Sxnan commented on PR #854:
URL: https://github.com/apache/flink-agents/pull/854#issuecomment-4776889177
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.
--
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]
