GreatEugenius commented on PR #854:
URL: https://github.com/apache/flink-agents/pull/854#issuecomment-4784435218
> Thanks for the work — the schema-hiding part (`injected_args` /
`@ToolParam(injected=true)`) is clean and useful.
>
> A step back on the API shape before line details: #853 asks for one narrow
thing — inject a model-hidden, framework-owned arg at execution time. This PR
does it via a general **before/after hook mechanism** (hook slots on `Agent`,
plan fields, serializers, YAML keys, new `ToolCallContext`/`ToolCallResult`).
That's a lot of machinery, and the generality makes injection harder, not
easier:
>
> * Two decoupled declarations (`injected_args` + a separate global hook
matching on `tool_name`), no plan-time link — forget/typo the hook → silent
crash.
> * One hook per agent → a manual `if tool_name == ...` dispatch table as
tools grow.
> * `after_tool_call` (result rewriting) is unrelated to injection — a
separate capability bundled in.
>
> Suggestion:
>
> 1. **Keep** `injected_args` for schema hiding.
> 2. **Make the value source declarative and co-located with the param**,
e.g. `@tool(inject={"tenant_id": lambda ctx: ctx.tenant_id})`, resolved right
before the call — drops the decoupling, the dispatch table, and the "declared
but never filled" gap.
> 3. **Defer** `before_tool_call` / `after_tool_call` to a separate issue if
a real cross-cutting need shows up — and there, consider the existing
event/action model over a parallel mechanism.
>
> If there's a concrete need behind the general hooks that isn't in #853,
worth surfacing here.
Thanks, agreed with the direction here. Stepping back, #853 should stay
focused on model-hidden framework-owned argument injection, and the generic
before/after hook mechanism was too broad for that scope.
I’ve updated the PR accordingly:
- Removed the before/after tool-call hook API and the related
Agent/Plan/YAML/serializer fields.
- Kept `injected_args` / `@ToolParam(injected = true)` for schema hiding.
- Made the injection source declarative and co-located with the parameter
instead of relying on a separate global hook. The current API supports explicit
sources such as config, sensory memory, and short-term memory.
- Kept resolution in the built-in `tool_call_action`, right before invoking
the tool, so the injected value remains hidden from the model and is not
written back to the tool request.
So the PR now covers only the narrow injection use case from #853. Any
general `before_tool_call` / `after_tool_call` capability can be discussed
separately if a concrete cross-cutting need shows up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
