Decouple DecoratorManager from eval.py TRIGGER_KWARGS allowlist

[dmamelin]

Motivation

DecoratorManager's function calls relied on eval.py's global TRIGGER_KWARGS allowlist to decide which unconsumed dispatch kwargs to silently ignore — a fixed, decorator-agnostic list that doesn't generalize to decorators with arbitrary argument names. This is a prerequisite for #869, which needs exactly that and was blocked by this dependency. Removing the legacy trigger.py subsystem entirely (and TRIGGER_KWARGS with it) is planned as a separate follow-up PR.

Behavior change

Previously, extra kwargs passed via @state_trigger(..., kwargs=...) (and other trigger decorators) had to match a parameter in the decorated function's signature, or the call raised TypeError. Now this isn't checked — the function simply receives whichever parameters it declares, and everything else is silently ignored, same as built-in trigger context kwargs (value, context, etc.) already behaved.

[ALERTua]

I have a few spare tokens, and there can be no such thing as an unneeded review, so here's what Opus said:

---

Reviewed at 0402db0. Line numbers below are from that commit. I read the code and the docs, but I did not run the test suite.

The change itself reads correctly, and the motivation is sound: a signature check generalizes to arbitrary argument names, and a fixed name list does not. Five points below, roughly in order of value.

1. The documentation now contradicts the code

docs/reference.rst:574 states the old rule directly:

In contrast, if you specify additional keyword parameters via kwargs, you will get an excepton if the function doesn't have matching keyword arguments (unless you use the **kwargs catch-all in the function definition).

This PR removes that exception, but the file is unchanged. A user who reads the reference will still expect an error that no longer happens. This is the cheapest item to fix and the one most likely to confuse people.

2. The decoupling is partial: @service still depends on TRIGGER_KWARGS

decorators/service.py:114 calls the function directly with func.call(ast_ctx, **func_args), so it never passes through FunctionDecoratorManager._call(). The new filter does not apply there, and the check at eval.py:751 stays live for that path.

The result is two different behaviors inside the same decorator subsystem: a trigger now ignores an unknown name silently, but a service raises TypeError for the same name. If PR #869 needs arbitrary argument names anywhere near the service path, the allowlist will block it again.

3. The new branch has no test coverage

The only test change is the stub at tests/test_decorator_manager.py:310:

self.func_def = ast.parse("def _stub(**kwargs): pass").body[0]

Because the stub declares **kwargs, every existing test takes the first branch and passes everything through. The else branch, which holds the whole new filter, is never executed. There is no test that an unknown kwarg is dropped, none for kwonlyargs, and none for posonlyargs. The stub makes the old tests pass; it does not exercise the new behavior.

4. posonlyargs in the accepted set can only produce an error

The filter builds the accepted set from posonlyargs + args + kwonlyargs. There are no positional arguments in this call: call_func(self.eval_func, None, **call_kwargs) passes None as the function name, not as arguments. So every kept positional-only name arrives as a keyword, eval.py:723 puts it into bad_kwargs, and eval.py:736 raises:

f() got some positional-only arguments passed as keyword arguments: 'value'

A trigger function declared as def f(value=None, /) therefore fails for certain. If posonlyargs were left out of the accepted set, the name would be dropped and the default value would apply, which is the behavior a reader expects from this filter.

5. The set is rebuilt on every dispatch, and duplicates logic EvalFunc already has

EvalFunc.__init__ already stores num_posonly_arg (eval.py:337), and get_positional_args() (eval.py:701) already collects posonlyargs + args. The new code reaches into self.eval_func.func_def.args from outside and rebuilds the set on every trigger fire.

A cached property or a small method on EvalFunc (for example accepted_kwarg_names, or filter_kwargs(kwargs)) computes it once, keeps _call() out of AST internals, and can be reused by decorators/service.py for point 2 and by the new decorator in PR #869.

Minor

• The new comment says "Trigger dispatch offers a fixed set of context kwargs". The set is not fixed: decorators/event.py:41 merges all of event.data, and decorator_abc.py:251 merges the user's own kwargs= dict. That variable part is the actual reason for this change, so the comment describes the opposite of the case.

• A question rather than a finding: a typo in the user's own kwargs={...} is always a user mistake, because those names are written by hand right next to the function. The strict check was useful there. One option is to keep raising for names that came from kwargs=, and to drop silently only the dispatch context names and pattern wildcards. The cost is that the decorator must tell _call() which keys it declared, which may not be worth it. Your call.

[dmamelin]

For context: this PR is one step in migrating off the legacy trigger.py subsystem — #869 needs decorators with arbitrary argument names, which was blocked by DecoratorManager depending on eval.py's TRIGGER_KWARGS allowlist. Removing trigger.py entirely (and TRIGGER_KWARGS with it) is a separate, larger follow-up PR. That's relevant to a couple of points below, where I'm deliberately deferring rather than fixing here.

Thanks for the thorough review — pushed 3 commits addressing this.

1. Docs mismatch — fixed in 8e28116: docs/reference.rst no longer claims a TypeError is raised for kwargs the function doesn't declare; it now describes the actual (new) behavior.

2. @service inconsistency — confirmed, @service still goes through the old TRIGGER_KWARGS-based path (func.call(ast_ctx, **data) unfiltered), so it's inconsistent with trigger dispatch right now. This doesn't affect #869. I'd rather fold the fix into the follow-up PR that removes the legacy trigger.py subsystem, since it raises a broader question:
@craigbarratt, I'd like @service to go through the same DecoratorManager pipeline as triggers (including running through @*_active) instead of its own bespoke call path — that's a breaking change, but seems more consistent. Happy to discuss there.

3. Positional-only params — good catch, and confirmed not a regression: before this PR, data.func_args was passed to EvalFunc.call() unfiltered, and dispatch never supplied positional args either, so a positional-only parameter whose name collided with a dispatch kwarg already hit TypeError: got some positional-only arguments passed as keyword arguments in the old code path (the bad_kwargs check runs before and independently of TRIGGER_KWARGS, so that list never protected against this case). I confirmed this by running the new test with NODM=1 (forcing the legacy trigger.py path): same TypeError, raised from trigger.py:1386 instead.

Fixed in e874fb9: posonlyargs is now excluded from the accepted set, so such a parameter falls back to its default instead of erroring. This only changes behavior for the new DecoratorManager path — trigger.py is untouched and keeps the old behavior, as intended for this PR's scope.

4. Missing test coverage — added in e874fb9 / af9e27e:

• a dispatch kwarg the function doesn't declare (and has no **kwargs) is dropped before the call
• a positional-only parameter is excluded from the accepted set (falls back to its default)
• a keyword-only parameter is correctly kept in the accepted set (it can be filled by keyword,
  unlike posonly)
• an end-to-end test (test_state_trigger_positional_only_param) exercising the real EvalFunc
  through a @state_trigger with a /-only parameter

5. Caching/duplication — EvalFunc.call() itself does the same kind of per-call work right after this: self.func_def.args.posonlyargs + self.func_def.args.args is rebuilt and looped over on every call (eval.py:715), same for kwonlyargs (eval.py:739) and set(kwargs.keys()) (eval.py:751). Only derived counts (num_posonly_arg, num_posn_arg) are cached in __init__, not the argument lists/sets themselves. So this filtering step is consistent with the existing style of the method it feeds into, and trigger dispatch isn't a hot path either — leaving it as-is for now.

Also, EvalFunc.get_positional_args() (eval.py:701) isn't directly reusable here — it returns posonlyargs + args, i.e. it includes posonlyargs, which is exactly what we need to exclude from the accepted set (see point 3). So even if we cached, we couldn't just call that existing method as-is.

Minor: "fixed set" wording — fixed, the comment no longer implies the kwarg set is static (it varies per trigger/service type, including arbitrary event/service data fields).

Stricter validation for kwargs= — I considered validating kwargs={...} against the function's signature at decorator-validate() time, but decided against it: trigger dispatch already passes plenty of optional context arguments a function isn't required to declare, so a mismatched name being silently ignored is consistent with that existing pattern, not a new class of footgun. It's also not fully silent in practice: a typo landing on a required parameter still fails the call, just with a different error. The only truly silent case is a typo landing on a parameter that already has a default.