weiqingy commented on PR #718:
URL: https://github.com/apache/flink-agents/pull/718#issuecomment-4598676976
@section9-lab Thanks for resolving each comment.
Confirming each:
1. **Tool-name resolution** — `buildToolCallIdToNameMap` +
`resolveToolFunctionName` recover the name from `externalId`, with explicit
`name` as a precedence override and a clear throw only when it's genuinely
unresolvable. `testRuntimeShapeMultiTurn` exercises the full ASSISTANT→TOOL
round-trip against the runtime shape — that's the contract locked in.
2. **Test contract** — dropping `testConvertToolMessageWithoutName` and
keeping `testConvertToolMessageThrowsWhenUnresolvable` for the
genuinely-missing case is exactly right; the suite now fails if the runtime
path regresses.
3. **`additional_kwargs`** — `top_k`/`top_p`/`stop_sequences` forwarded, and
warning on dropped unknown keys is a nice touch beyond what I'd asked for. Good
call flagging the `Float` `topK` — surprising, but it matches the upstream
protocol.
4. **IAE contract** — validating `model` before the `try` plus the explicit
`catch (IllegalArgumentException) { throw; }` makes both entry points
symmetric; nice that `checkFinishReason()`'s IAE surfaces unwrapped too.
5. **Vertex** — smoke test + javadoc note is the right scope. Accepting both
ADC outcomes while asserting the flag isn't dropped is a reasonable bound for a
path that can't run e2e in CI.
6. **`dist/pom.xml`** — gemini is now bundled alongside the siblings, and CI
staying green says google-genai's heavier transitive set
(protobuf/guava/auth/httpclient) shaded cleanly without duplicate-class
fallout. That was my main worry, so 👍.
LGTM from my side.
--
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]
