Skip to content

Bind function arguments with CPython semantics in LocalPythonExecutor - #2755

Open
ShayanMuhammad-CS wants to merge 1 commit into
huggingface:mainfrom
ShayanMuhammad-CS:fix/executor-argument-binding
Open

ShayanMuhammad-CS wants to merge 1 commit into
huggingface:mainfrom
ShayanMuhammad-CS:fix/executor-argument-binding

Conversation

@ShayanMuhammad-CS

@ShayanMuhammad-CS ShayanMuhammad-CS commented Sep 5, 2026 •

Copy link
Copy Markdown

Closes #2754.

What

create_function in local_python_executor.py binds call arguments to parameters by hand, and the binding diverges from CPython in six ways. Most of them produce a silently wrong value rather than an error, so an agent's code action keeps running on bad data.

evaluate_lambda had the same defects independently.

# Case CPython main
1 def f(a, b, *rest) → f(1, 2, 3, 4) rest == (3, 4) rest == (1, 2, 3, 4)
2 A().m(1, 2, 3) for def m(self, a, *rest) rest == (2, 3) rest == (<A object>, 1, 2, 3)
3 def f(a, *, d=2, **kw) → f(1, d=4, z=5) kw == {'z': 5} kw == {'d': 4, 'z': 5}
4 def f(a, /, b) → f(1, 2) (1, 2) InterpreterError: The variable a is not defined.
5 def f(a, *, b=7) → f(1) (1, 7) InterpreterError: The variable b is not defined.
6 f = lambda x, y=10: (x, y) → f(1) (1, 10) InterpreterError: The variable y is not defined.

Two further consequences of the same code:

Defaults were re-evaluated on every call instead of once at definition time, so a mutable default never accumulated. The practical effect is that the standard memoisation idiom silently degraded from linear to exponential — the cache was empty on every call:

def fib(n, memo={}):
    if n in memo: return memo[n]
    v = n if n < 2 else fib(n-1) + fib(n-2)
    memo[n] = v
    return v
fib(22)

main: 2.0s. This branch: 0.02s (and fib(30) returns instantly rather than taking minutes).

Argument mismatches went unreported. Against def f(a): ..., CPython raises TypeError for f(1, 2), f(1, a=2), f(1, z=2) and f(); on main all four ran, leaving parameters unbound and typically failing later and elsewhere with a confusing "The variable b is not defined".

How

Build an inspect.Signature from the ast.arguments node once, at definition time (so defaults are evaluated once, as CPython does), then bind each call through Signature.bind() + apply_defaults(). That reproduces CPython's rules for positional-only, keyword-only, *args, **kwargs and defaults, and surfaces CPython's own TypeError messages on a mismatch — rather than re-deriving those rules by hand.

The two new helpers (build_signature, bind_arguments) are shared with evaluate_lambda, so lambdas now support defaults, keyword arguments, *args and **kwargs too.

super() still works: __class__ is injected as before, and self is now bound by name through the signature.

Two existing tests asserted the old behaviour

Both are corrected in this PR, and I want to flag them explicitly for review rather than bury them:

  • test_variable_args asserted 15 for var_args_method(1, 2, 3, x=4, y=5) against def var_args_method(self, *args, **kwargs). CPython returns 14 — self takes 1, so args == (2, 3) and kwargs == {"x": 4, "y": 5}, giving 5 + 9. The old 15 was exactly the bug in case 1 above. Only the expected value changed.
  • test_exceptions defined def method_that_raises(self) and called it with no arguments. In CPython that is a TypeError, not the ValueError the test means to catch. The stray self parameter is removed, preserving what the test is actually about (an exception raised inside a function being catchable).

Tests

14 regression tests added, covering every case in the table plus self-exclusion, full-signature binding (def f(p, /, a, b=1, *args, d, e=5, **kwargs)), lambda defaults/keywords, definition-time default evaluation, and the four TypeError mismatches.

  • tests/test_local_python_executor.py: 409 passed, 3 skipped.
  • The one remaining failure, test_vulnerability_for_all_dangerous_functions[os.system], also fails on clean main in this environment (Windows) and is unrelated to this change.
  • ruff check and ruff format --check clean over src and tests.
  • Verified no regressions in test_agents.py, test_tools.py, test_default_tools.py, test_types.py, test_memory.py, test_utils.py, test_monitoring.py, test_final_answer.py, test_serialization.py and test_tool_validation.py by running them against main and against this branch and comparing: this branch fails no test that main passes.

I found these by differential-testing the executor against CPython over a corpus of snippets; 16 of 20 argument-binding cases diverged before this change and 0 diverge after.


Disclosure, per CONTRIBUTING: I used an AI assistant (Claude Code) to help find and write this change. I have read the diff, run the tests, and will stand behind it in review.

`create_function` bound call arguments by hand, which diverged from CPython
in several ways that silently produced wrong values rather than errors:

- `*args` captured every positional argument instead of only the extras,
  so `def f(a, b, *rest)` called as `f(1, 2, 3, 4)` gave `rest == (1, 2, 3, 4)`.
  For methods this also leaked `self` into `*args`.
- `**kwargs` captured every keyword argument, including ones already bound to
  named or keyword-only parameters.
- Positional-only parameters were never bound, raising "The variable `a` is
  not defined".
- Keyword-only defaults (`kw_defaults`) were ignored.
- Defaults were re-evaluated on every call instead of once at definition time,
  so mutable defaults never accumulated (memoisation caches stayed empty).
- Argument-count and duplicate/unexpected-keyword mismatches were not detected,
  so calls that should raise `TypeError` silently ran with missing bindings.

Build an `inspect.Signature` from the `ast.arguments` node once at definition
time and bind through `Signature.bind()`, which reproduces CPython's rules and
its `TypeError` messages. `evaluate_lambda` shared the same defects and now
uses the same helpers, so lambdas support defaults, keyword arguments,
`*args` and `**kwargs`.

Two existing tests asserted the old behaviour and are corrected: the expected
value in `test_variable_args` (CPython returns 14, not 15, because `self`
consumes the first positional argument), and the stray `self` parameter in
`test_exceptions`, whose function was called with no arguments and so would
raise `TypeError` in CPython.
@ShayanMuhammad-CS
ShayanMuhammad-CS force-pushed the fix/executor-argument-binding branch from e5c65aa to e27da9d Compare September 5, 2026 21:16
@ShayanMuhammad-CS

Copy link
Copy Markdown
Author

Notes for the reviewer

A few things I'd rather surface myself than have you find.

Suggested reading order. build_signature is the whole change; bind_arguments and both call sites are thin. The one piece worth checking carefully is the default alignment in build_signature: ast.arguments.defaults covers the last N of posonlyargs + args, which is why the index is computed from the right (first_default_index). kw_defaults is different — it is positionally aligned with kwonlyargs and uses None for "no default", which is why it is zipped directly and None maps to Parameter.empty.

The intentional behaviour change to weigh. Calls that CPython rejects now raise TypeError instead of silently running with unbound parameters. Some existing agent runs will therefore start erroring where they previously "worked". I think that is the right trade for this library specifically: the executor is what agents get feedback from, and a TypeError is a signal the model can see and correct on the next step, whereas the old behaviour let a wrong value propagate into the final answer. But it is a real behaviour change and worth a deliberate call, not just my judgement — happy to gate it if you'd rather.

The two test edits are the part I'd scrutinise if I were reviewing. Both are described in the PR body. Short version: test_variable_args asserted 15 where CPython returns 14, and test_exceptions called a one-parameter function with zero arguments. Neither test was testing the thing its edit touches, and I kept both edits as small as possible — one expected value, one stray parameter. If you'd prefer test_variable_args rewritten to assert the components (args/kwargs separately) rather than the sum, say the word; I avoided it only to keep the diff honest about what changed.

Deliberately out of scope. I found these by differential-testing the executor against CPython, which turned up other divergences I did not touch here, to keep this reviewable:

Happy to file the decorator one separately if it's wanted; I didn't want to bundle unrelated semantics into an argument-binding PR.

Naming. build_signature and bind_arguments are module-level and public, matching the surrounding helpers (create_function, evaluate_lambda). If you'd rather they were private, an underscore prefix is a one-line change.

On the environment caveats in the PR body. I ran the suite on Windows/Python 3.11, so the os.system security test fails for me on clean main too, and pytest needs PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 because the arize-phoenix plugin fails to import there. Neither is related to this change, but it does mean CI on Linux is a better signal than my local run for those specific tests — worth approving the workflow run rather than taking my word for it.

@VANDRANKI VANDRANKI 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.

Community review, not a merge-gate approval.

This replaces the sandboxed interpreter's hand-rolled positional/keyword argument binding in create_function/evaluate_lambda (src/smolagents/local_python_executor.py) with inspect.Signature.bind(), and I think this is the right approach: rather than re-deriving CPython's argument-binding rules by hand (which is exactly what the old zip(arg_names, args) + separate vararg/kwarg/defaults handling was doing, incompletely), it builds a real inspect.Signature from the ast.arguments node once at function-definition time and delegates the actual binding to the standard library on every call.

I checked build_signature maps each AST argument kind correctly: posonlyargs before the positional-or-keyword args list get POSITIONAL_ONLY, vararg gets VAR_POSITIONAL, each kwonlyarg gets KEYWORD_ONLY with its default pulled from the parallel kw_defaults list (correctly handling that kw_defaults can contain None at a given index when that particular kwonly arg has no default, which is how the AST represents it), and kwarg gets VAR_KEYWORD. Defaults are evaluated once, at signature-build time, matching real CPython (and covered directly by test_default_values_are_evaluated_once_at_definition, which checks the classic mutable-default-argument sharing behavior across two calls).

I traced test_full_signature_binding (def func(p, /, a, b=1, *args, d, e=5, **kwargs) called as func(0, 1, 2, 3, 4, d=9, z=8)) by hand against real Python semantics and it matches the asserted (0, 1, 2, (3, 4), 9, 5, {"z": 8}), and the four test_argument_mismatch_raises_type_error cases (too many positional args, a missing required arg, a duplicate value for a named param, an unexpected keyword) all rely on Signature.bind() raising TypeError itself rather than the sandbox silently doing something wrong, which is a meaningfully different failure mode than a naive zip()-based binder (which mostly just silently drops or misassigns extra/missing arguments instead of raising).

The var_args_method test result changing from 15 to 14 is a real behavior fix, not a regression: the old code set func_state[vararg_name] = args using the entire positional-args tuple passed to new_func, which for a bound method call includes self as the first element, so *args inside the method incorrectly contained self too. The new code excludes self from *args because self is just the first ordinary positional_args entry that the signature already binds by name, and only the genuinely leftover positional arguments land in *args. test_method_star_args_excludes_self and test_star_args_only_receives_the_extra_positional_arguments both target this directly.

One thing worth a second look from a maintainer, not a bug I found: create_function no longer explicitly sets func_state["self"], relying entirely on the signature binding to place it (since self is just the first positional_args name), while still keeping a separate explicit func_state["__class__"] = args[0].__class__ line for super() support. That split is correct as far as I traced it (the self binding happens through the normal path now, __class__ genuinely needs special-casing since it isn't a real parameter), but it's a subtle enough split that I'd flag it for a second pair of eyes rather than assert it's airtight without running the full existing method/super() test suite myself.

Overall this reads as a correct, well-motivated rewrite with tests that actually probe CPython-specific edge cases (positional-only params, keyword-only defaults, **kwargs exclusion of named/keyword-only params, mismatch errors) rather than just re-testing the happy path.

@ShayanMuhammad-CS

Copy link
Copy Markdown
Author

@VANDRANKI thanks for the careful trace. On the self / __class__ split: bind_arguments runs before the __class__ line, so self is already in func_state by the time __class__ is set, and a zero-argument call raises TypeError from Signature.bind() before args[0] is ever touched. The existing super() tests in test_local_python_executor.py (e.g. super().__init__(...), super().__getattr__(...)) are among the 409 passing on this branch.

@albertvillanova would you be able to take a look when you have time, and approve the CI workflow run? Local tests pass on Windows, but Linux CI is the better signal for the os.system security test, as noted above. Happy to make any changes.

(For reference, the decorator divergence I mentioned as out of scope is now tracked separately in #2771.)

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.

BUG: LocalPythonExecutor binds function arguments incorrectly (*args, **kwargs, positional-only, keyword-only, defaults)

2 participants