fix(curation): narrow SELECT gate to actual SELECT/WITH text (#450)

[k0shir0]

Fixes #450

What was broken
POST /curation/preview and POST /curation/pin both advertise "exactly one read-only SELECT" but disagreed on PRAGMA/DESCRIBE/SHOW/SUMMARIZE. DuckDB labels those as StatementType.SELECT (table functions), so the shared gate accepted them. Pin runs SQL as-is → 200. Preview wraps as DESCRIBE SELECT * FROM (<sql>) → parser error syntax error at or near ")" with LINE 1: DESCRIBE SELECT * FROM (PRAGMA database_list) – our wrapper blamed as caller's syntax error.

Why
Gate checked only StatementType.SELECT. PRAGMA database_list is a legal statement but an illegal subquery, so the wrapper fails to parse. The 400 detail leaked internal rewrite. Preview→pin flow allowed pinning a cut that preview refused.

What changed
Narrow the shared gate in src/hflow/curation.py:reject_non_single_select – after type check, require stripped SQL to start with SELECT or WITH (CTE) after leading --//* */ comments ((?i)^(SELECT|WITH)\b). Both endpoints now reject PRAGMA/DESCRIBE/etc with the existing "sql must be exactly one read-only SELECT statement" and no wrapper leak. SELECT * FROM pragma_version() remains allowed (SELECT text).

Why this choice
Issue offered: narrow gate vs stop wrapping. Chose narrow gate – simple, consistent with advertised rule, minimal diff, no risk of materializing large results just to name columns. Pinning PRAGMA-based cuts is not a real use case; a SELECT that needs pragma data can still SELECT * FROM pragma_version() as SELECT text. Keeps preview's LIMIT/count/SUMMARIZE wrapping unchanged; after gate, wrapper only sees SELECT/WITH which is always wrappable, so misleading parse error disappears.

How tested

• Before: PRAGMA database_list → preview 400 (wrapper leak) vs pin 200 (divergence). After: both 400 with fixed message.
• New tests: tests/test_catalog_curation.py – gate rejects PRAGMA/DESCRIBE/SHOW/SUMMARIZE (incl. comment-prefixed) and accepts --//* */+SELECT and WITH; packages/hflow-server/tests/test_server_curation_preview.py and test_server_curation_pin.py – preview/pin agree and no DESCRIBE SELECT * FROM leak.
• Suites: pytest tests/test_catalog_curation.py 97 passed, pytest packages/hflow-server/tests 234 passed, ruff check/format/ty check clean.

Not a security fix – constrained=True already blocks file access regardless of statement type.

[kstonekuan]

The reasoning is good and the write-up is the best-argued PR body I have read on this repo this week: you picked one of the two options, said why, kept the existing message, handled leading -- and /* */ comments, and stated plainly that this is not a security fix. The gate is clean, 1612 passed.

The problem is what the text check refuses. (?i)^(SELECT|WITH)\b is narrower than "one read-only SELECT", and DuckDB has legal read-only queries that start with neither:

sql                                type                  runs?   starts SELECT/WITH
FROM range(3)                      StatementType.SELECT  yes     False
FROM range(3) SELECT *             StatementType.SELECT  yes     False
FROM range(3) WHERE range > 1      StatementType.SELECT  yes     False
(SELECT 1)                         StatementType.SELECT  yes     False
VALUES (1), (2)                    StatementType.SELECT  yes     False

FROM-first is the one that matters. FROM episodes WHERE status = 'ok' is idiomatic modern DuckDB and a perfectly ordinary cut for a curation surface, and after this it is refused with "sql must be exactly one read-only SELECT statement". That sentence would be false: it is exactly one read-only SELECT.

Which lands back on what #450 was actually about. The complaint was not that PRAGMA is accepted, it was that we blame the caller for our own wrapper. Refusing a valid SELECT with a message telling the user it is not a SELECT is the same defect wearing different clothes.

To be fair about the size of it: nothing in the repo writes FROM-first today, and no existing test or doc breaks. This is a narrowing of what the public surface accepts, not a broken build.

Where to go

Whatever you pick, the constraint I would hold to is that the gate should not decide read-only-ness by matching text. DuckDB's grammar will keep growing and an allowlist of opening keywords will keep being wrong in the direction of refusing valid queries.

Two directions that do not have that property:

Ask DuckDB whether the wrap is legal, instead of guessing from the prefix. The real precondition is "this SQL survives being used as a subquery", which is a question DuckDB can answer at parse time without executing anything, the same way extract_statements already does. Then the refusal is about the actual constraint and PRAGMA is refused for the true reason.

Stop wrapping in preview, which is the other option the issue offered. Column names and types are available from the executed statement's own description, so preview would not need DESCRIBE SELECT * FROM (<sql>) at all. More work, and worth checking what else that wrapper is carrying (the LIMIT and count paths) before committing.

If you conclude the text check is right after all and FROM-first should be refused, argue that instead: say what a user loses and why the message should still say what it says. I would rather be talked out of this than have you rewrite it because I said so.

Either way, add FROM-first, (SELECT ...) and VALUES to whatever test you land, in the direction you decide. They are the cases this round did not consider, and they should not be silent again.