Reject empty allowed_tables in SQLToolset instead of allowing all tables - #73381
Conversation
justinpakzad
left a comment
There was a problem hiding this comment.
Thanks for the PR. The changes look good to me. This is technically a breaking change for anyone who previously passed an empty list (e.g. from a filtered list comprehension). Maybe worth a note in the change log. Will leave that up to a maintainer though since the commit subject that gets baked into the change log might be sufficient.
|
One note on the description, since it has no inline anchor: the summary gives "for example from a template or configuration" as how an empty allow-list arrives. Jinja cannot be the source here. The configuration half holds, though: a |
…ngelog Rejecting an empty allowed_tables breaks construction that 0.8.0 and 0.9.0 accepted, so an upgrading Dag that builds the list dynamically now fails at import and needs to be told that None is the way to ask for allow-all. The toolsets page carries the same parameter list as the docstring and is where users of this toolset land, so it has to describe the guard as well.
|
All of the points are addressed in the pushed update. Thanks for reviewing! |
Yeah, I have checked them again and fixed the PR's description Thanks for pointing that out! |
Summary
SQLToolset(allowed_tables=[])silently exposed every table in the schema to the agent. The constructor usedfrozenset(allowed_tables) if allowed_tables else None, so an empty list collapsed toNone, which means "no restriction". An allow-list that resolves to nothing — aVariable.get, a YAML or config read, or a filtered comprehension in the Dag file — therefore disabled the guardrail entirely:list_tablesreturned every table,get_schemaaccepted any table, andquery/check_queryskippedallowed_tablesenforcement. The toolset now fails fast with a clearValueErrorinstead.Nonekeeps its documented meaning of "allow every table".Changes
SQLToolset.__init__raisesValueErrorwhenallowed_tablesis an empty list.allowed_tablesdocstring and in the matching parameter list indocs/toolsets.rst.Changelogheader ofproviders/common/ai/docs/changelog.rst:allowed_tables=[]is accepted by 0.8.0 and 0.9.0, so a Dag that builds the list dynamically now fails at import instead of quietly allowing every table.ValueError.Was generative AI tooling used to co-author this PR?