Add portable ClientSQL runtime and debugger - #181
Conversation
github-actions
Bot
added
area/runtime
Sensitive Files Detected📦 Dependency change — Modifies the Bazel module graph — needs runtime team review after import. 📎 Prebuilt binary — Changes a prebuilt binary — requires verification of provenance. 🔧 Build rules — Affects build rules for all Valdi consumers. This is an automated notice. A maintainer will review after import. |
09cec35 to
475a3aa
Compare
|
| Test Suite | Result |
|---|---|
| API Surface Check | ✅ success |
| Test Coverage Delta | ✅ success |
| Linux: Registry Validation | ✅ success |
| valdi_web Integration Test | ✅ success |
| macOS: C++ & Platform Tests | ❌ failure |
| Snapshot Tests | ✅ success |
| Valdi Smoke Tests | ❌ failure |
| Linux: Build Compiler | ✅ success |
| Linux: Build & Export | ✅ success |
| Linux: Hotreload Smoke | ✅ success |
| Linux: C++ Tests | ❌ failure |
| Linux: Module Tests | ✅ success |
Some tests failed. Please check the workflow logs for details.
🚀 Bazel remote cache is now enabled - future builds will be faster!
Workflow: Valdi CI
f8f1449 to
406e3c4
Compare
github-actions
Bot
removed
area/runtime
clholgat
left a comment
clholgat
left a comment
There was a problem hiding this comment.
Really nice work — this is careful and thorough. Generated code is injection-safe throughout (values bound via ?, identifiers escaped, literals json.dumps'd), RAII is correct in the native layer, the SQLite provenance is documented and hash-pinned, and the tests genuinely compile + execute generated output rather than snapshotting strings. Agreed it should stay in draft until the @Version(__PLACEHOLDER__) annotations are finalized.
I've left inline comments. Grouping and priority:
Blocking / please confirm
- Debugger auto-registration exposes read access to open DBs — is it gated to debug builds / authenticated? (comment on
ClientSQLDebug.ts) - Native: unbounded result set (High), and two transaction-liveness gaps (Med) that can wedge the writer or
std::terminate.
Generator — silently-wrong generated types, not crashes (worth fixing since they hit common SQL): parameterized column types, SELECT * across a JOIN, WITHOUT ROWID tables, and the await reserved word.
Two items I couldn't anchor cleanly inline:
- Teardown/GC path (Med). On the drop-without-explicit-close path, the last queued task dropping the final coordinator reference destroys the dispatch queue (and runs
sqlite3_close) from within a task running on that same queue — a potential self-join deadlock/crash. The explicit-close path looks fine; this is the GC path, and it depends on the dispatch-queue destructor semantics, so worth a look. - Non-transactional
query()during an open write transaction reads the pre-transaction WAL snapshot (dispatched to the reader pool, not deferred likeexecute()). Defensible reader-pool semantics, but an easy caller foot-gun — worth documenting.
Suggested tests: three fixtures — a parameterized column type, SELECT * across a JOIN, and a WITHOUT ROWID table — would catch the top three generator findings directly.
Nothing here is a redesign; the structure is solid. Happy to dig deeper on any of these.
| ) | ||
|
|
||
| http_archive( | ||
| name = "sqlite", |
There was a problem hiding this comment.
Nit: this http_archive block is duplicated in bzl/dependencies.bzl for the WORKSPACE path. The consistency test helps catch drift, but a single shared definition would remove the hazard entirely.
There was a problem hiding this comment.
I looked at consolidating this. MODULE.bazel and the legacy WORKSPACE entry point cannot directly reuse the same repository-setup macro; moving the Bzlmod side behind a module extension would broaden this change considerably. I kept the declarations in their required entry points, added comments documenting why, and retained test_native_sqlite_dependency_shape_is_explicit_for_apple_and_default to pin both URLs and hashes and catch drift. Happy to pursue a module-extension cleanup separately if preferred.
406e3c4 to
8e1b957
Compare
|
Addressed both unanchored findings as well:
I also added the suggested parameterized-type, joined- |
clholgat
left a comment
clholgat
left a comment
There was a problem hiding this comment.
Thanks for the fast turnaround — this addresses the review well. Confirmed on 8e1b957: result-set caps (row + byte, with a clear "add a LIMIT" error), the transaction watchdog plus broadened Exception& / std::exception& / ... catches, the isDebugEnabled trust-boundary comment, and on the generator the parameterized column types, WITHOUT ROWID, JOIN handling, and reserved-word set all look fixed. 👍
Left inline comments for the two small remaining items (teardown/GC path, NUL nit) and a couple of portability/gating requests (overridable SQLite labels, folding the new tests into //valdi:test).
One design question that informs how this slots into existing setups: is the emitted TypeScript intended to be output-compatible with SQLDelight v1, or is it a fresh contract? i.e. would an existing SQLDelight-v1 .sq consumer regenerate to equivalent bindings, or should they expect behavioral differences to revalidate? Not a blocker — just want to understand the migration story.
Nice work overall.
| ] + select({ | ||
| "//bzl/conditions:ios": [], | ||
| "//bzl/conditions:macos": [], | ||
| "//conditions:default": ["@sqlite//:sqlite"], |
There was a problem hiding this comment.
Portability request: consider exposing the SQLite dependency as an overridable label (e.g. a label_flag defaulting to @sqlite//:sqlite) rather than hardcoding it here. That lets an embedder that already vendors SQLite point this at their copy and avoid linking a second amalgamation with duplicate sqlite3_* symbols — without patching this BUILD file. Same idea would help for the validator's @sqlite_316.
There was a problem hiding this comment.
Agreed, and the internal ClientSQL context makes this broader than SQLite alone. I will add overridable runtime and validation SQLite labels, preserve the existing internal compiler seam, and make the whole native implementation selectable so an embedder links exactly one factory/runtime. I am leaving this open until the merged internal graph proves that the OSS SQLite and factory are absent when internal providers are selected.
|
Answering the compatibility question: this generator is a fresh Valdi-specific SQLDelight-style contract. It is not currently demonstrated to be output-compatible with SQLDelight v1 or Snap’s internal ClientSQL, so existing consumers should not regenerate against it without revalidation. The current
Those changes prevent the open-source import from implicitly replacing Snap’s compiler or linking two native factories. They do not by themselves prove generated-binding, native-name, debugger-ID, database-path, or migration compatibility. The remaining integration gate is:
Generated artifacts are sufficient; internal source is not needed. The useful inputs are the current ClientSQL |
Consolidate the hermetic ClientSQL generator, pinned SQLite toolchain, native/web runtime, integration coverage, debugger provider, and smoke fixtures into one dependency-complete change.\n\nCarries six native API version annotations plus one matching generator expectation; the commit must remain draft until Snap allocates one concrete version and completes the sensitive-file/import review.
8e1b957 to
f3ce8a0
Compare
![semgrep-code-snapchat[bot]](https://avatars.githubusercontent.com/u/40505220?s=60&v=4){kind=small}
| [ | ||
| str(CLIENTSQL_TOOLCHAIN_EXECUTABLE), | ||
| "-s", str(sql_dir), | ||
| "-p", "SharedDb", | ||
| "-c", "SharedDb", | ||
| "-m", "SharedDb", | ||
| "-o", str(output), | ||
| "-l", "typescript", | ||
| ], |
There was a problem hiding this comment.
Semgrep identified an issue in your code:
Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'.
Dataflow graph
flowchart LR
classDef invis fill:white, stroke: none
classDef default fill:#e7f5ff, color:#1c7fd6, stroke: none
subgraph File0["<b>compiler/clientsql/test_clientsql.py</b>"]
direction LR
%% Source
subgraph Source
direction LR
v0["<a href=https://github.com/Snapchat/Valdi/blob/f3ce8a09d972dc54aeca945cb35d325d0727dc36/compiler/clientsql/test_clientsql.py#L25 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 25] os.environ</a>"]
end
%% Intermediate
subgraph Traces0[Traces]
direction TB
v2["<a href=https://github.com/Snapchat/Valdi/blob/f3ce8a09d972dc54aeca945cb35d325d0727dc36/compiler/clientsql/test_clientsql.py#L28 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 28] path</a>"]
v3["<a href=https://github.com/Snapchat/Valdi/blob/f3ce8a09d972dc54aeca945cb35d325d0727dc36/compiler/clientsql/test_clientsql.py#L25 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 25] value</a>"]
v4["<a href=https://github.com/Snapchat/Valdi/blob/f3ce8a09d972dc54aeca945cb35d325d0727dc36/compiler/clientsql/test_clientsql.py#L34 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 34] environment_tool_path</a>"]
v5["<a href=https://github.com/Snapchat/Valdi/blob/f3ce8a09d972dc54aeca945cb35d325d0727dc36/compiler/clientsql/test_clientsql.py#L34 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 34] CLIENTSQL_TOOLCHAIN_EXECUTABLE</a>"]
v6["<a href=https://github.com/Snapchat/Valdi/blob/f3ce8a09d972dc54aeca945cb35d325d0727dc36/compiler/clientsql/test_clientsql.py#L34 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 34] CLIENTSQL_TOOLCHAIN_EXECUTABLE</a>"]
end
v2 --> v3
v3 --> v4
v4 --> v5
v5 --> v6
%% Sink
subgraph Sink
direction LR
v1["<a href=https://github.com/Snapchat/Valdi/blob/f3ce8a09d972dc54aeca945cb35d325d0727dc36/compiler/clientsql/test_clientsql.py#L249 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 249] [<br> str(CLIENTSQL_TOOLCHAIN_EXECUTABLE),<br> "-s", str(sql_dir),<br> "-p", "SharedDb",<br> "-c", "SharedDb",<br> "-m", "SharedDb",<br> "-o", str(output),<br> "-l", "typescript",<br> ]</a>"]
end
end
%% Class Assignment
Source:::invis
Sink:::invis
Traces0:::invis
File0:::invis
%% Connections
Source --> Traces0
Traces0 --> Sink
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by dangerous-subprocess-use-tainted-env-args.
You can view more details about this finding in the Semgrep AppSec Platform.
There was a problem hiding this comment.
/fp False positive: CLIENTSQL_TEST_TOOLCHAIN_EXECUTABLE is supplied by the Bazel py_test via $(rootpath :clientsql_toolchain), and subprocess.run receives a structured argv list with shell=False (the default). No value is interpreted by a shell.
2 participants
Description
Adds a dependency-complete portable ClientSQL implementation on top of the debugger capabilities landed from #180.
The single commit includes the hermetic ClientSQL generator, pinned SQLite toolchain, native and web runtimes, integration coverage, debugger provider, and a cross-platform Ledger SQL demo. The demo exercises reactive account and ledger queries, atomic transfers, reset behavior, and deterministic stress batches on macOS, iOS, and Android.
This revision also incorporates the review hardening requested on this PR: debug-database registration is inert unless runtime debugging is enabled; debugger query results are bounded; native transactions have timeout and exception-safe rollback paths; database teardown is safe from the coordinator queue; and the SQL generator now covers parameterized types, joins, nested constraints,
WITHOUT ROWID, reserved identifiers, quoted identifiers and aliases, and scalar subqueries.The SQLite source-archive provenance and global build-rule changes require the existing sensitive-file/import review.
This PR supersedes #176 and #177; those PRs retain the incremental review history.
Type of Change
Testing
bazel test //...)Testing Details
bazel test //compiler/clientsql:test_clientsql— 24 tests passedbazel test //src/valdi_modules/src/valdi/client_sql:client_sql_native_tests //valdi:test_client_sql_runtime_integration— passedbazel build //src/valdi_modules/src/valdi/client_sql:client_sql_web //apps/ledger_sql_demo:ledger_sql_demo_ios— passedbazel build //apps/ledger_sql_demo:ledger_sql_demo_android --snap_flavor=platform_development --copt=-DANDROID_WITH_JNI --repo_env=VALDI_PLATFORM_DEPENDENCIES=android --define=client_repo_arm64=true --android_platforms=@snap_platforms//os:android_arm64— passedbazel query //...— passedgit diff --check— cleanThe full repository-wide
bazel test //...matrix was not run locally; CI remains the final platform matrix.Checklist
Related Issues
Builds on the capabilities landing from #180.
Supersedes #176 and #177 while preserving their review history.
Additional Context
The single head commit is directly parented on the imported #180 capabilities landing at
b83dc83ee3d3587c3a94a3634fed560df3b0ccdd. Review the ClientSQL-only rangeb83dc83ee3d3587c3a94a3634fed560df3b0ccdd..f3ce8a09d972dc54aeca945cb35d325d0727dc36.