Skip to content

fix(tool_validation): tuple literals wrongly rejected as complex#2527

Open
chuenchen309 wants to merge 1 commit into
huggingface:mainfrom
chuenchen309:fix/tool-validation-tuple-literal
Open

fix(tool_validation): tuple literals wrongly rejected as complex#2527
chuenchen309 wants to merge 1 commit into
huggingface:mainfrom
chuenchen309:fix/tool-validation-tuple-literal

Conversation

@chuenchen309

Copy link
Copy Markdown

What

validate_tool_attributes()'s "is this attribute a simple literal" check whitelisted (ast.Constant, ast.Dict, ast.List, ast.Set) but not ast.Tuple, so any Tool subclass using an ordinary tuple class attribute (e.g. allowed_exts = (".png", ".jpg")) failed validation with "Complex attributes should be defined in __init__, not as class attributes" — blocking Tool.save()/push_to_hub() for a completely standard pattern.

Adding ast.Tuple alone wasn't sufficient: the check uses ast.walk() over the whole assignment value, and for Tuple/List nodes that also yields their ast.Load() context node (a Tuple's ctx field, always Load for a value expression) — previously unnoticed because Dict/Set don't have a ctx field. This meant list literal attributes had the identical latent bug (untested until now), just never reported since tuples were the more common case people hit:

>>> ast.dump(ast.parse('x = (".png", ".jpg")').body[0].value)
"Tuple(elts=[...], ctx=Load())"   # ast.walk() yields the Load() node too

The non-literal-default-value check (__init__ parameter defaults) only needed ast.Tuple added — it inspects the top-level default node directly, not via ast.walk(), so it doesn't have the ctx-node issue.

Fix

Add ast.Tuple (both checks) and ast.Load (the ast.walk()-based check only) to the whitelists.

Testing

Extended ValidTool in tests/test_tool_validation.py with tuple_attr, list_attr class attributes and a tuple_default __init__ parameter default, covered by the existing test_validate_tool_attributes_valid parametrized test. Confirmed via git stash that all three new cases fail against the pre-fix code and pass after; InvalidToolComplexAttrs's genuinely-complex list-comprehension case is still correctly rejected (no false negative introduced).

  • pytest tests/test_tool_validation.py — 16 passed.
  • pytest tests/test_utils.py — 44 passed, no regressions.
  • ruff check and ruff format --check both clean on the changed files.

AI disclosure

Developed with AI assistance (Claude Code), which located the bug via a targeted search for AST-node-type whitelists that looked incomplete. I reviewed the diff, independently reproduced the crash and the fix by executing both against the actual function (including diagnosing the secondary ast.Load issue after the first fix attempt was incomplete), and ran the tests/linters myself before opening this PR.

validate_tool_attributes()'s "is this attribute a simple literal"
check whitelisted (ast.Constant, ast.Dict, ast.List, ast.Set) but not
ast.Tuple, so any Tool subclass using an ordinary tuple class attribute
(e.g. allowed_exts = (".png", ".jpg")) failed validation with "Complex
attributes should be defined in __init__, not as class attributes" -
blocking Tool.save()/push_to_hub() for a completely standard pattern.

Adding ast.Tuple alone wasn't sufficient: the check uses ast.walk() over
the whole assignment value, and for Tuple/List nodes that also yields
their ast.Load() context node (a Tuple's `ctx` field, always Load for a
value expression) - previously unnoticed because Dict/Set don't have a
ctx field. This meant *list* literal attributes had the identical latent
bug (untested until now), just never reported since tuples were the more
common case people hit. Added ast.Load to the whitelist to cover both.

The non-literal-default-value check (__init__ parameter defaults) only
needed ast.Tuple added - it inspects the top-level default node directly,
not via ast.walk(), so it doesn't have the ctx-node issue.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@ErenAta16 ErenAta16 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the root cause directly: ast.walk() on a List or Tuple node yields a Load node for its ctx field in addition to the element Constants (Set and Dict don't have a ctx field, so they were unaffected). Ran the old vs new allowlist against list_attr, tuple_attr, dict_attr, set_attr directly:

old check passes: list_attr=False, tuple_attr=False, dict_attr=True, set_attr=True
new check passes: list_attr=True, tuple_attr=True, dict_attr=True, set_attr=True

So this fix is actually broader than the title suggests, list-literal class attributes were just as broken as tuples before this (despite ast.List already being in the old allowlist, the uncaught Load child still tripped the all(...) check), and adding ast.Load fixes both. Good that list_attr got added to the test alongside tuple_attr, that's pinning a real second fix, not just a sanity check. Looks correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants