RJ Coleman (feced1d6) at 17 Mar 17:53

RJ Coleman (94784853) at 17 Mar 17:44

@romaneisner

I've made these changes in */experimental/flows but I'm not sure how to test these changes. Can you point me in the direction so I may test these changes locally before I re-request reviews from you and @tmrrss

RJ Coleman (7cd1b8c9) at 17 Mar 17:21

RJ Coleman (a95b4b10) at 17 Mar 17:14

@tmrrss

I addressed the lint errors and have created the MR in canonical gitlab-org/gitlab!227310 - have requested review from you on that MR.

Hey @panoskanell I've requested review from @tmrrss who is a domain maintainer.

@romaneisner I've added this but in toolset.py lines 152 - 164

@romaneisner & @tmrrss

I've adjust this MR to do this. No more force_internal and for any parameter that exists, it should be configurable in the flow yaml under toolset.

I'm closing this as I think this is unnecessary.

Based on this documentation and my own testing, I think our solution is here in the flow yaml:

ui_log_events:
      - "on_tool_execution_success"
      - "on_tool_execution_failed"
      - "on_agent_final_answer" <-- REMOVING THIS FIXES THE ISSUE IN AGENT SESSION PAGE + JOB LOGS

@elwyn-gitlab I think this is unnecessary but need confirmation if at all possible.

Based on this documentation and my own testing, I think our solution is here in the flow yaml:

ui_log_events:
      - "on_tool_execution_success"
      - "on_tool_execution_failed"
      - "on_agent_final_answer" <-- REMOVING THIS FIXES THE ISSUE IN AGENT SESSION PAGE AND IN JOB LOG

RJ Coleman (8060b8de) at 16 Mar 17:17

RJ Coleman (1e40a231) at 16 Mar 16:47

RJ Coleman (d4397e29) at 16 Mar 13:55

RJ Coleman (89045273) at 16 Mar 13:18

Ah very good catch @tmrrss - breaking change is no good. Let me update this MR with this instead. I'll re-request review when I'm done.

Hey @romaneisner (cc @tmrrss)

Good question. We can't restrict values to primitives only, since we already anticipate list values in future use cases (e.g., default_labels: ["security", "auto-generated"]). However, I agree we should guard against unbounded nesting.

We could enforce a max depth limit (e.g., 1 level deep) - values can be primitives or flat lists/dicts, but not nested structures like {"a": {"b": {"c": ...}}}. Something like:

def _validate_tool_options(options: dict[str, Any]) -> None:
    for key, value in options.items():
        if isinstance(value, dict):
            if any(isinstance(v, (dict, list)) for v in value.values()):
                raise ValueError(
                    f"Tool option '{key}' contains nested structures. "
                    "Only flat key-value mappings are supported."
                )
        elif isinstance(value, list):
            if any(isinstance(v, (dict, list)) for v in value):
                raise ValueError(
                    f"Tool option '{key}' contains nested structures. "
                    "Only flat lists are supported."
                )

This keeps things flexible enough for lists and simple dicts while preventing arbitrarily deep nesting.

WDYT?

@romaneisner & @tmrrss

I 100% agree. Better option. We currently have this follow up issue:

#2009

Wondering if we can add this to that Issue as part of validating tool_options.

WDYT?

Hey @romaneisner

I added you to the documentation update MR related to this MR as well. LMK if you have any questions.