[JoeyPatricio]

Overview

Fixes #31057
The Functionary v3.2 chat template fails with selectattr: unknown test 'type' when a tool parameter uses oneOf. The shared filter implementation interprets arguments by count, so selectattr('type', 'defined') treats the attribute name as the test name.

Each registration now declares whether it reads an attr through the lookup_attr template parameter. With the input array at position 0, the test name is at position 1 for select/reject and position 2 for selectattr/rejectattr. One loop replaces the three count-based branches and forwards all remaining template args to the test. selectattr('type', 'defined') now selects items with a defined type attr, allowing the template to render oneOf parameters.

This has two side effects:

• select('nonexistent') used to return an empty list silently. It now raises select: unknown test 'nonexistent', matching Jinja2.
• Bare select/reject used to error. They now keep truthy or falsy items (respectively).

Additional information

N/A

Requirements

• I have read and agree with the contributing guidelines
• AI usage disclosure: Yes, was used for bug reproduction and code review.

[CISC]

/bot review

[ggml-gh-bot]

Automated code review

Review of PR #31059 - jinja: fix select and selectattr argument handling

I reviewed the diff statically, tracing selectattr/select/reject/rejectattr through func_args (common/jinja/value.h:648), apply_filter/try_builtin_func (common/jinja/runtime.cpp:302), and the built-in test registry in common/jinja/value.cpp, plus the existing tests in tests/test-jinja.cpp and template usages in tests/test-chat-template.cpp and common/chat-diff-analyzer.cpp.

Verdict: the fix is correct and matches Jinja2 semantics. The old count-based branches conflated select (test name at pos 1) with selectattr (attr at pos 1, test at pos 2); the lookup_attr template parameter plus test_pos resolves that cleanly, and forwarding all trailing args to the test matches Jinja2's prepare_select_or_reject. I verified:

• func_args::func_name is populated before builtin invocation (value_func_t::invoke, common/jinja/value.h:713), so the new error message args.func_name + ": unknown test ..." resolves to the real filter name ("select"/"selectattr"/...), no regression there.
• The bare select/selectattr truthiness path (test_it == builtins.end()) matches Jinja2's func = None -> bool(item) fallback, and the undefined-attr default keeps non-object items out of the selection.
• Existing in-repo usages are unaffected: tests/test-chat-template.cpp and common/chat-diff-analyzer.cpp only use the 4-arg form selectattr("role", "equalto", "user"); existing tests select('in', keep) / reject('in', skip) (extra-arg forwarding) and the 2-arg/4-arg selectattr tests exercise the new paths.
• Lifetime is safe: get_attribute returns a reference to the loop-scoped val_default or into the item, and it is copied into val immediately.

Minor points, none blocking:

(point 1) common/jinja/value.cpp:331 - args.get_pos(1) is called inside the per-item loop. The old code hoisted it (auto attribute = args.get_pos(1);). Each call re-runs the bounds check and copies the shared_ptr per item. Hoist it before the loop for clarity and to match the previous structure.

(point 2) The static function is still named selectattr, but with the lookup_attr parameter it now implements all four filters. Jinja2 calls the shared helper prepare_select_or_reject; a rename along those lines (e.g. select_or_reject) would make the dual role obvious. Optional.

(point 3) The tests cover the new happy paths well, but not the new error path. Consider adding one test asserting that items|selectattr('x', 'nosuchtest') raises (there is an existing pattern for expected failures in tests/test-jinja.cpp:1211), since the "unknown test" behavior for previously-silent cases is the most behavior-visible side effect of this PR.

(point 4) Worth being aware (no change requested): the old 3-arg form selectattr('equalto', 'text') - test applied to the whole item - now means attr='equalto', test='text' and raises "unknown test 'text'". That is Jinja2-correct and no in-repo template relies on the old form, but templates out in the wild that accidentally depended on it will now hard-fail instead of silently rendering an empty list. If that is intended, the PR description already covers it; just confirm it is.

Style checks pass: ASCII only, comments concise and useful, tests added to the existing file/function following existing patterns, no unrelated changes, commit message fine (no Co-authored-by). Linked issue (#31057) present, AI usage disclosed.

This review was generated automatically by pi coding agent using zai-org/GLM-5.3. It may contain mistakes. Maintainers make the final call.

[CISC]

Nice catch, thank you!

Please address bot points 1-3.

[JoeyPatricio]

Thanks for the quick review! I implemented all three, but slightly differed on the test suggestion.

I kept the selectattr case and added select('nosuchtest'), since that's the case that used to return an empty list silently. Selectattr already raised before this PR, so the test also checks that the error names 'nosuchtest' (the old code reported 'x').

[CISC]

I kept the selectattr case and added select('nosuchtest'), since that's the case that used to return an empty list silently. Selectattr already raised before this PR, so the test also checks that the error names 'nosuchtest' (the old code reported 'x').

It's too elaborate and specific, can I suggest that you instead just use an attr with the same name as a test? The concern isn't really the error message, but that is uses the right arg.

[JoeyPatricio]

Oooooh I see now. I simplified the test a lot and should be a lot closer to what your suggesting / neater. Thanks so much for the help! Let me know if you need anything else from me.