Skip to content

Add core snapshot provider proof#94694

Closed
giodl73-repo wants to merge 3 commits into
mainfrom
giodl/snapshot-provider-proof
Closed

Add core snapshot provider proof#94694
giodl73-repo wants to merge 3 commits into
mainfrom
giodl/snapshot-provider-proof

Conversation

@giodl73-repo

@giodl73-repo giodl73-repo commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Adds a core SQLite snapshot provider proof under src/snapshot, not an extension/plugin.
  • Proves the first step of the RFC's safe-sync-file story: turn a live SQLite database into a verified artifact + manifest that a file-syncing host can persist.
  • Adds a local SqliteSnapshotProvider backed by a manifest + compact database.sqlite artifact repository.
  • Builds on refactor(sqlite): land database-first memory and proxy alignment #94646's database-first layout: global/control-plane SQLite and per-agent/data-plane SQLite are the long-term targets; this PR proves the mechanism against one explicit DB reference.
  • Shares the SQLite VACUUM INTO primitive with the existing backup path through packages/memory-host-sdk, including sqlite-vec loading and compact-snapshot behavior.
  • Proves committed WAL-mode state is captured without asking hosts to sync live *.sqlite-wal, *.sqlite-shm, or *.sqlite-journal sidecars.

Why this matters

Scout/Lobster-style hosts persist OpenClaw by syncing files when they are saved. A live SQLite directory is the wrong durability boundary: the main *.sqlite file can be incomplete without WAL state, and unmanaged file deltas over live SQLite can observe DB pages, WAL frames, and sidecars at different moments.

This PR proves the safer core boundary: SQLite materializes a clean artifact first, the provider verifies it, and the host can sync the completed artifact set instead of the live DB working set. WAL bundles remain a metrics-gated Phase 2 after verified snapshot artifacts and cursors exist.

Scope

This is PR 1 from the snapshot RFC roadmap: core provider proof only. It intentionally does not add openclaw snapshot ... commands, backup command integration, remote storage, leases, scheduling, WAL bundles, or failover orchestration.

RFC context: openclaw/rfcs#20
Database-first state layout: #94646
Related storage discussion: #90370

Verification

  • node scripts/run-vitest.mjs run src/snapshot/snapshot-provider.test.ts - passed, 1 file / 6 tests
  • node scripts/run-vitest.mjs run packages/memory-host-sdk/src/host/sqlite-vec.test.ts - passed, 1 file / 9 tests
  • node_modules\.bin\oxlint src\snapshot packages\memory-host-sdk\src\host\sqlite-snapshot.ts packages\memory-host-sdk\src\engine-storage.ts src\infra\backup-create.ts - passed
  • git diff --check - passed

Real behavior proof

Behavior or issue addressed:
Core SQLite snapshot provider can create, verify, list, and internally restore compact snapshot artifacts from SQLite databases, including WAL-mode state. This proves the safe sync artifact boundary for the database-first SQLite target model landed in #94646.

Real environment tested:
Windows source checkout at C:\src\openclaw-snapshot-pr1, Node/Vitest source harness against node:sqlite.

Exact steps or command run after this patch:

  1. node scripts/run-vitest.mjs run src/snapshot/snapshot-provider.test.ts
  2. node scripts/run-vitest.mjs run packages/memory-host-sdk/src/host/sqlite-vec.test.ts
  3. node_modules\.bin\oxlint src\snapshot packages\memory-host-sdk\src\host\sqlite-snapshot.ts packages\memory-host-sdk\src\engine-storage.ts src\infra\backup-create.ts
  4. git diff --check

Evidence after fix:

  • Snapshot provider test passed: 1 file / 6 tests.
  • SQLite vec/shared primitive test passed: 1 file / 9 tests.
  • Oxlint completed cleanly for changed provider/shared files.
  • git diff --check completed with no whitespace errors.

Observed result after fix:
The source harness creates a WAL-mode source database, snapshots via the core local provider, verifies the manifest and artifact, restores to a fresh path, and reads both pre-WAL and committed-in-WAL rows from the restored database. It also covers failed create cleanup, artifact tamper detection, signed user_version, same-millisecond snapshot IDs, and stale restore-target WAL sidecar removal.

What was not tested:
Public openclaw snapshot CLI, backup command integration, object storage providers, scheduled snapshots, WAL bundles, leases, and fresh OpenClaw state-dir boot proof; those are intentionally later PRs.

@openclaw-barnacle openclaw-barnacle Bot added size: L maintainer Maintainer-authored PR labels Jun 18, 2026
@clawsweeper

clawsweeper Bot commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 19, 2026, 12:00 AM ET / 04:00 UTC.

Summary
The PR adds a core SQLite snapshot provider, manifest/artifact format, shared VACUUM INTO helper, and provider tests, while rewiring the existing backup SQLite snapshot loop to use the helper.

PR surface: Source +388, Tests +242. Total +630 across 7 files.

Reproducibility: no. live repro was run in this read-only review. Source inspection shows the verifier omits sqlite-vec loading while current memory code creates vec0 virtual tables that require the sqlite-vec extension path.

Review metrics: 2 noteworthy metrics.

  • Snapshot Artifact Contract: 1 manifest schema, 1 fixed database artifact filename. These fields can become the durability contract consumed by follow-on CLI and host-sync tooling.
  • Existing Backup Path Rewired: 1 backup snapshot loop changed. The PR changes shipped backup behavior, so validation must cover both the new provider and existing backup archives.

Stored data model
Persistent data-model change detected: database schema: packages/memory-host-sdk/src/host/sqlite-snapshot.ts, database schema: src/snapshot/manifest.ts, database schema: src/snapshot/snapshot-provider.test.ts, database schema: src/snapshot/snapshot-provider.ts, serialized state: src/snapshot/manifest.ts, unknown-data-model-change: packages/memory-host-sdk/src/host/sqlite-snapshot.ts, and 1 more. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted terminal output, copied live output, logs, a linked artifact, or a recording from the after-patch snapshot provider run.
  • [P1] Load sqlite-vec, or otherwise prove it is not required, before running snapshot integrity checks on artifacts and restored targets.
  • [P1] Get maintainer approval or narrowing for the core snapshot manifest/provider contract before merge.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body only summarizes Vitest/lint source-harness runs; the contributor should add redacted terminal output, copied live output, logs, a linked artifact, or a recording, then update the PR body to trigger re-review.

Risk before merge

  • [P1] Snapshot verification opens artifacts without loading sqlite-vec, so vec0-backed OpenClaw memory snapshots may fail verify or restore after create succeeds.
  • [P1] The manifest/artifact/provider shape is a new core durability contract that follow-on CLI and host-sync work can depend on, so maintainer approval is needed before merge.
  • [P1] The contributor proof remains Vitest/lint summary only; this external PR still needs redacted real behavior proof or a maintainer override before merge.

Maintainer options:

  1. Fix Verifier And Approve Contract (recommended)
    Load sqlite-vec or prove it is unnecessary during integrity checks, add focused coverage, and keep the PR open until maintainers accept the manifest/provider boundary and proof is added.
  2. Narrow To Backup Helper
    If the provider contract is premature, remove the src/snapshot provider and keep only the shared VACUUM INTO helper extraction with backup validation.
  3. Pause Behind RFC Or CLI
    If maintainers want the API and user workflow decided together, pause this draft until the RFC and dependent CLI PR settle.

Next step before merge

  • [P1] Manual review is needed because missing contributor proof and core contract approval remain merge gates that automation cannot satisfy for this protected draft.

Security
Cleared: No concrete security or supply-chain concern was found in the final 7-file diff; the current PR no longer changes lockfiles or CI/package execution surfaces.

Review findings

  • [P2] Load sqlite-vec before integrity checks — packages/memory-host-sdk/src/host/sqlite-snapshot.ts:37-39
Review details

Best possible solution:

Fix sqlite-vec-aware verification, add real after-patch proof, and land only if maintainers approve the core snapshot manifest/provider contract; otherwise narrow the PR to the shared backup helper extraction.

Do we have a high-confidence way to reproduce the issue?

No live repro was run in this read-only review. Source inspection shows the verifier omits sqlite-vec loading while current memory code creates vec0 virtual tables that require the sqlite-vec extension path.

Is this the best way to solve the issue?

No, not merge-ready as written. The provider shape is plausible, but the verifier should handle sqlite-vec-backed artifacts and maintainers still need to accept the core durability contract.

Full review comments:

  • [P2] Load sqlite-vec before integrity checks — packages/memory-host-sdk/src/host/sqlite-snapshot.ts:37-39
    The create path loads sqlite-vec before VACUUM INTO, and current memory code can create memory_index_chunks_vec USING vec0. verifySqliteDatabaseIntegrity reopens the artifact read-only without extension loading before PRAGMA integrity_check, so snapshots containing OpenClaw's vec0 table can fail verification or restore even though creation succeeded.
    Confidence: 0.82

Overall correctness: patch is incorrect
Overall confidence: 0.82

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 845ad1cf71b9.

Label changes

Label justifications:

  • P2: This is a normal-priority storage feature PR with bounded backup/snapshot blast radius and no urgent live outage.
  • merge-risk: 🚨 compatibility: The PR introduces a manifest/artifact shape and exported SQLite helper that follow-on snapshot CLI and host-sync work can rely on as a durability contract.
  • merge-risk: 🚨 other: The unresolved risk is maintainer acceptance of a new core snapshot boundary, which normal CI cannot decide.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body only summarizes Vitest/lint source-harness runs; the contributor should add redacted terminal output, copied live output, logs, a linked artifact, or a recording, then update the PR body to trigger re-review.
Evidence reviewed

PR surface:

Source +388, Tests +242. Total +630 across 7 files.

View PR surface stats
Area Files Added Removed Net
Source 6 404 16 +388
Tests 1 242 0 +242
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 7 646 16 +630

What I checked:

  • Repository policy read: Root AGENTS.md was read fully; its PR review, proof, storage, and compatibility guidance applies because this PR touches SQLite state snapshots, backup behavior, and a new durability artifact shape. (AGENTS.md:21, 845ad1cf71b9)
  • Final PR surface: Live PR metadata shows head 0175bf3 with 7 changed files under packages/memory-host-sdk, src/infra, and src/snapshot; the PR is draft, mergeable clean, and still carries maintainer/proof/merge-risk labels. (0175bf36aaf3)
  • Shared SQLite helper added: The new helper loads sqlite-vec before VACUUM INTO on create, but the verify helper opens the database read-only and runs PRAGMA integrity_check without allowExtension or loadSqliteVecExtension. (packages/memory-host-sdk/src/host/sqlite-snapshot.ts:35, 0175bf36aaf3)
  • Backup path rewired: The existing backup SQLite snapshot loop now calls createVacuumedSqliteSnapshot, so the PR affects shipped backup behavior as well as the new provider. (src/infra/backup-create.ts:657, 0175bf36aaf3)
  • Current main lacks provider: Current main only has the existing backup path and memory-host storage barrel; no src/snapshot provider or packages/memory-host-sdk/src/host/sqlite-snapshot.ts exists on main. (845ad1cf71b9)
  • OpenClaw memory uses sqlite-vec tables: Current memory code loads sqlite-vec and creates memory_index_chunks_vec USING vec0, which is the database shape the snapshot verifier needs to handle. (extensions/memory-core/src/memory/manager-sync-ops.ts:646, 845ad1cf71b9)

Likely related people:

  • vincentkoc: Current main blame points to this author for the SQLite backup snapshot path, memory-host storage barrel, sqlite-vec loader, and the merged database-first SQLite alignment stack this PR builds on. (role: recent area contributor; confidence: high; commits: 2ef0589b760d, 65eb2ccd4ae2, 1b819e6551fd; files: src/infra/backup-create.ts, packages/memory-host-sdk/src/engine-storage.ts, packages/memory-host-sdk/src/host/sqlite-vec.ts)
  • jalehman: Related merged database-first storage work includes memory indexing and source-key commits touching memory-host and sqlite-vec-adjacent behavior. (role: adjacent storage contributor; confidence: medium; commits: c1ac5eb9e728, d53e5c32e72f, 9df9ee294b5d; files: packages/memory-host-sdk/src/host/memory-schema.ts, extensions/memory-core/src/memory/manager-sync-ops.ts, extensions/memory-core/src/memory/manager-search.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jun 18, 2026
@giodl73-repo
giodl73-repo force-pushed the giodl/snapshot-provider-proof branch 2 times, most recently from 3362721 to 9c45304 Compare June 18, 2026 23:40
@giodl73-repo giodl73-repo mentioned this pull request Jun 19, 2026
Closed
@giodl73-repo

giodl73-repo commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@giodl73-repo
giodl73-repo force-pushed the giodl/snapshot-provider-proof branch from 592e0c5 to 3d1a406 Compare June 19, 2026 02:10
@giodl73-repo

giodl73-repo commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Rebased on current origin/main after #94646 landed and updated the PR body to call out the database-first SQLite target model explicitly. Latest head: 3d1a4064ab.

@clawsweeper

clawsweeper Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@giodl73-repo giodl73-repo mentioned this pull request Jun 19, 2026
Closed
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 19, 2026
@giodl73-repo giodl73-repo changed the title Add snapshot provider proof plugin Add core snapshot provider proof Jun 19, 2026
This was referenced Jun 19, 2026
Closed
Closed
Closed
@giodl73-repo

giodl73-repo commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Closing this stack slice in favor of the consolidated core snapshot PR: #94805.

We collapsed the previous PR1-4 chain into one reviewable feature PR so reviewers can evaluate the provider, CLI, named targets, and safe-sync artifact proof together. The stress harness remains a separate follow-up in #94967.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: L status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@giodl73-repo