fix(mcp): classify compound routines as ddl#385
Conversation
debba
left a comment
There was a problem hiding this comment.
Took a look at this. The core approach is sound — moving the routine check ahead of the multi-statement guard so the ; inside a BEGIN ... END body don't get the whole thing flagged as unknown. I traced the consumers in mcp/mod.rs (the read-only gate at line 913 and the approval gate at line 933) and both key off kind != "select", so ddl and unknown are treated the same way for enforcement. That means the new path only ever returns ddl for a CREATE, never downgrades anything to select, and stays fail-closed. No security concern here.
Two small things, both about classification accuracy rather than safety:
-
is_create_compound_routineruns on the raw uppercased SQL (trimmed_upper), while the rest ofclassify_query_kindworks offstrip_strings_and_comments. Sosplit("BEGIN"),rfind("END")and the trailing-statement scan all see string literals and comments as if they were code. A legit routine whose body has something likeSELECT 'END; foo'will get read as a trailing statement and fall back tounknown. It fails closed, so nothing breaks, but the auditquery_kindends up wrong. Worth running this on the already-stripped string instead. -
without_terminal_semicolon.ends_with("END")isn't word-boundary aware, unlikecontains_keyword. A malformedCREATE FUNCTION ... BEGIN RETURN total_spend(no realEND, butSPENDends in "END") would slip past that check. Edge case and it's still gated as a write, so cosmetic, but the boundary check would be more consistent with the rest of the file.
The nested BEGIN ... END limitation you already called out is fine — it resolves to unknown, which is the safe direction.
Nothing blocking. Good to merge as is; the two notes above are optional cleanups.
Closes #384
Summary
Review notes
Test plan