Harden the shared query builders for canned-query and MCP callers - #55
Merged
Merged
Conversation
Addresses roborev findings from jobs 8, 9, 11, 12, 13, 19 and part of 20.
Datasette hands canned-query parameters to SQLite as strings, and a
parameter the user left blank arrives as ''. Bound directly that broke two
ways, both reproduced before fixing: LIMIT '' raises "datatype mismatch",
and COUNT(*) >= '' compares an affinity-less aggregate against text, which
is always false, so a HAVING clause silently discarded every row. Nothing
exercises the named form yet, so this was latent — but group 3 wires the
canned queries up and would have hit it immediately.
Numeric placeholders in the named form are now wrapped as
CAST(COALESCE(NULLIF(:x, ''), <default>) AS INTEGER), falling back to each
builder's own default. Two candidate builders multiplied the limit in
Python, which would have turned a supplied "20" into "202020"; the
arithmetic moved into SQL. Found while testing the blank case: json_each('')
raises "malformed JSON", so id-set parameters coalesce to an empty array,
which matches nothing rather than failing the statement.
Two parametrized tests now execute every builder's named form with all
parameters blank and with all parameters string-typed, so this class of bug
cannot return for a builder added later.
parse_when resolved per row, not per statement. SQLite called it 28 times
for a 28-row scan, so a query crossing a cache-generation boundary compared
early rows against one instant and later rows against another — a result
depending on scan order. Registering the functions deterministic=True pins
the value for the statement (28 invocations to 1) while still re-evaluating
between statements, which is what keeps relative expressions fresh. The
claim is accurate: within one statement these functions are pure in their
arguments.
Also: the DST assertion used strict 23<delta<25 bounds, which a daylight
saving transition legitimately hits exactly; now inclusive.
Documentation: the read-only layer table said temporary DDL needs no
authorizer, contradicting the same section's note that query_only is
resettable — corrected, since the deny list already includes the _TEMP_
variants. The proposal claimed return shapes were unchanged while album
aggregates gained album_ids; now stated as additive. Task 1.3 claimed a
wheel verification its config file could not satisfy; now records that it
was verified against a scaffold and that 6.1 re-verifies.
Album aggregation is deliberately untouched per operator decision, so the
filtered-attribution findings (jobs 18, 20) remain open.
447 tests pass; lint, type and audit clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses roborev job 21, which reviewed d1f8acf. All three findings were valid; the first is a bug that commit introduced. **SQL injection in the numeric fallback.** `_numeric` interpolates its COALESCE default rather than binding it, because a bound parameter cannot serve as a default in static SQL — and `_numeric_param` took that default from the caller's own `limit`/`min_plays`. Seven builders embedded an attacker-controlled payload into SQL text: LIMIT CAST(COALESCE(NULLIF(:limit, ''), 20; DROP TABLE plays) AS INTEGER) Reproduced across all seven before fixing. That is a worse defect than the blank-parameter bug d1f8acf was written to fix, and my own contract test missed it: it checked that a quote-heavy *artist* value is bound rather than embedded, but never exercised the numeric path. Fallbacks are now coerced with `_as_int`, which refuses anything non-integer. Digits-as-text still work, since Datasette binds even numeric parameters as strings. Two parametrized tests cover every numeric parameter on every builder that has one, against five payloads. The rollup builders compared `limit <= 0` before coercion and so raised TypeError on a string and rejected a legitimate "7"; they coerce first now. **parse_when was registered deterministic=True, which is untrue and does not do what I claimed.** It reads the wall clock, so asserting SQLite's determinism contract for it is false. And the flag is an optimizer permission, not a guarantee of single evaluation — measured, a single call site with a bound argument is hoisted (28 invocations to 1), but two call sites resolve independently and a column-valued argument is evaluated once per row. The previous commit's claim that the flag "pins the value for the statement" was wrong. The flag now applies only to the three functions that genuinely qualify. The real remedy belongs in SQL: design D5 and task 3.3 now require a canned query to resolve parse_when once in a `WITH ... AS MATERIALIZED` bound, verified to resolve exactly once. No builder emits parse_when today — they bind an already-converted UTC string — so nothing is broken in the meantime. **The statement-stability test proved nothing.** It registered its own function with the flag instead of going through prepare_connection, so it would pass even if production registration lost it. Replaced with one that patches SQL_FUNCTIONS and calls prepare_connection, asserting month_name is hoisted and parse_when is not, plus a test documenting the two call-site and column-argument cases and that a materialized CTE fixes them. 508 tests pass; lint, type and audit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses roborev job 22, which reviewed 6a90f57. Both findings valid, and both are gaps in that commit. fmt_ts was left marked deterministic while parse_when was corrected. It is not: it defers to dateutil.parser.parse, which fills missing components from today's date, so fmt_ts('12:00') returns today at noon and fmt_ts('March') returns today's day in March — both change from one day to the next. It is stable for the full ISO timestamps the schema stores, which is why this was easy to miss, but a SQL function accepts whatever an ad hoc query passes it and SQLite's contract covers every accepted input. Having just made this mistake with parse_when, I fixed that one and did not audit the other three. month_name and fuzz_partial_ratio were checked and do qualify; they keep the flag. A test now asserts each flag against what the function actually does, with the partial-input behavior as evidence, so the justification is recorded rather than assumed. Numeric validation was named-form only. _numeric_param returned before coercing for the positional form, and _limit_clause's positional branch bound directly, so the path the CLI and MCP actually execute passed a malformed value through to SQLite — surfacing as "datatype mismatch" from the database instead of an error naming the parameter. Not an injection route, since positional values are bound rather than interpolated, which the review scored correctly as Low. Both forms now coerce identically, and the refusal tests are parametrized over both forms; checking only the named one is what let this through. CLI-path SQL remains unchanged: of 25 builders rendered positionally, 23 are byte-identical to main and the two that differ moved a limit multiplication into SQL, verified to produce the same effective limit. 560 tests pass; lint, type and audit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
roborev job 23, one LOW finding on the test added in 07c6794. It captured date.today() once and then compared two later fmt_ts calls against it, so a midnight rollover between the capture and either call would fail the test. Each call now reads the date immediately before and after and accepts either, which is the narrower of the two suggested fixes and needs no clock freezing. Unrelated and pre-existing: tests/test_date_parsing.py::test_weekday_name fails today, and fails identically on main. parse_relative_time("Monday") resolves to the nearest Monday, which on a Sunday is tomorrow, so its `result <= datetime.now()` assertion cannot hold. Left alone as out of scope; the rest of the suite is green. 559 passed with that one test deselected; lint and type clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate test robustness and coverage findings remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Hardens shared SQL query builders for Datasette and MCP callers by safely handling numeric parameters and correcting SQLite function determinism metadata.
Changes:
- Added blank-safe numeric coercion and injection protections.
- Expanded regression tests for builders and SQL functions.
- Updated OpenSpec planning and design documentation.
File summaries
| File | Changes and review notes |
|---|---|
tests/test_query_builders.py |
Adds numeric safety and fallback tests. Moderate (3 votes): expand coverage to all normalized numeric call sites. Moderate (2 votes): use more than five rows to verify the default limit. |
tests/test_datasette_functions.py |
Tests function registration and stable time-bound behavior. Moderate (1 vote): avoid asserting that deterministic=True causes exactly one evaluation. |
src/scrobbledb/domain_queries.py |
Adds numeric validation and blank-tolerant SQL rendering. |
src/scrobbledb/datasette_plugin/functions.py |
Corrects SQLite function determinism registration. |
openspec/changes/add-datasette-web-server/tasks.md |
Updates task verification requirements. |
openspec/changes/add-datasette-web-server/proposal.md |
Corrects documented return-shape claims. |
openspec/changes/add-datasette-web-server/design.md |
Documents materialized bounds and read-only protections. |
Review details
Suppressed comments (1)
tests/test_datasette_functions.py:491
- The exact
len(calls["month_name"]) == 1assertion treatsdeterministic=Trueas a guarantee of single evaluation, but SQLite defines it only as permission for optimizer transformations (which the surrounding comments correctly describe as something that "may" happen). A different supported SQLite build can invoke this constant expression more than once while the registration is correct, making the test flaky; record thedeterministicargument passed tocreate_functionwith a test double instead of asserting an optimizer choice.
conn.execute("SELECT COUNT(*) FROM rows_ WHERE month_name(3) IS NOT NULL").fetchone()
assert len(calls["month_name"]) == 1, (
"month_name is deterministic and should be hoisted; "
f"got {len(calls['month_name'])} invocations"
)
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two PR review comments on the tests added in this branch; both correct. The hand-written matrix covered 10 of 18 (builder, parameter) pairs, so eight builders that route numerics through _numeric_param or _limit_clause were unprotected — build_albums_by_search_sql, build_tracks_by_search_sql, build_artist_fts_candidates_sql, build_artist_like_candidates_sql, build_top_tracks_sql, build_yearly_rollup_sql, and the limit parameters of build_artists_with_stats_sql and build_tracks_list_sql, the last two of which the review itself did not list. The matrix is now derived from the builders' signatures, so a builder is covered the moment it gains a numeric parameter, with an assertion that at least 18 pairs are found. Confirmed the coverage is real rather than nominal: reintroducing a direct LIMIT interpolation into build_yearly_rollup_sql — a builder the old list omitted — now fails 11 tests, where before it would have passed the suite. The blank-limit test was vacuous. It inserted four rows against a limit of five, so a correct fallback and an unbounded result both returned four. It now inserts nine and asserts exactly five come back, with an unbounded query alongside to show nine exist. 647 passed with the pre-existing Sunday test_weekday_name failure deselected; lint, type and audit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Addressed both review comments in 18fa56a. Both were about tests in this branch that didn't verify what they claimed, and both were worse than the comments suggested:
647 tests pass; lint, type and audit clean. The one red test, |
Four habits, each traceable to a review finding on this branch rather than general advice. The motivating case is not the six LOW findings about weak tests but a MEDIUM one: a test written to assert that user values are bound rather than embedded only exercised the string path, so it read as coverage while a SQL injection went in through the numeric path. Inserted without reflowing the rest of the file — flowmark rewraps the kata-managed block, which `kata init --with-agents` owns.
Owner
Author
|
LGTM! Fire in the ... Gates of Hell! |
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
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.
Makes the shared query builders safe for the consumers they were extracted for. The CLI screens its inputs with
click; the Datasette canned queries and MCP tools do not.Three defects, each reproduced before being fixed:
''for anything blank.LIMIT ''raiseddatatype mismatch, andCOUNT(*) >= ''is always false, so aHAVINGclause silently discarded every row.COALESCEdefault must be interpolated rather than bound, and it came from caller input — seven builders would embed arbitrary text into SQL. Fallbacks are now coerced to integers, in both SQL forms.parse_whenandfmt_tsboth read the clock, so neither is registereddeterministic=Trueany more. A canned query needing one stable bound must resolve it in a materialized CTE, now required by design D5 and task 3.3.No user-visible change: of 25 builders rendered in the positional form the CLI uses, 23 are byte-identical to
main, and the two that differ moved a limit multiplication into SQL with the same effective result.Album aggregation is deliberately untouched. Several review findings argued for changing it; per operator decision the current design stands, and those are recorded as declined rather than disproved.
Also corrects three stale planning claims: the read-only layer table on temporary DDL, the proposal's "return shapes unchanged", and task 1.3's wheel verification.
560 tests pass; lint, type and audit clean.
Note:
test_date_parsing.py::test_weekday_namefails on Sundays, on this branch and onmainalike —parse_relative_time("Monday")resolves to the nearest Monday, which on a Sunday is tomorrow. Pre-existing and untouched here.🤖 Generated with Claude Code
https://claude.ai/code/session_01HSVhPRjZjmqJNE81C4rhKv