Repository navigation
Conversation
| mapOf( | ||
| "artifact_names" to Schema(type = Type.ARRAY, items = Schema(type = Type.STRING)), | ||
| ), |
There was a problem hiding this comment.
Please revert the unrelated formatting change in declaration() (and the trailing-comma edits on untouched tests in LoadArtifactsToolTest.kt lines 97–386) so the diff stays focused on processLoadArtifactsFunctionCall and the new tests.
There was a problem hiding this comment.
Done in b4d321b: reverted the declaration() rewrap and the trailing-comma edits to the existing tests. The diff now only touches processLoadArtifactsFunctionCall, which keeps its original Content(...) layout inside the new loop, plus the imports and new tests in LoadArtifactsToolTest.kt.
|
|
||
| private suspend fun assertArtifactReachesAnsweringModel(loadArtifactsFirst: Boolean) { | ||
| val artifacts = InMemoryArtifactService() | ||
| val filename = "invoice.txt" | ||
| val payload = "Invoice total is 1234.56 USD" | ||
| artifacts.saveArtifact( | ||
| SessionKey("artifact_app", "user1", "session1"), | ||
| filename, | ||
| Part(text = payload), | ||
| ) | ||
| val currencyCall = Part(functionCall = FunctionCall("get_currency", id = "currency-1")) | ||
| val artifactCall = | ||
| Part( | ||
| functionCall = | ||
| FunctionCall( | ||
| "load_artifacts", | ||
| args = mapOf("artifact_names" to listOf(filename)), | ||
| id = "artifact-1", | ||
| ), | ||
| ) | ||
| val orderedCalls = | ||
| if (loadArtifactsFirst) listOf(artifactCall, currencyCall) | ||
| else listOf(currencyCall, artifactCall) | ||
| var calls = 0 | ||
| var answerRequest: LlmRequest? = null | ||
| val model = | ||
| DummyModel("artifact-model") { request -> | ||
| calls++ | ||
| if (calls == 1) { | ||
| flowOf(LlmResponse(content = Content(role = Role.MODEL, parts = orderedCalls))) | ||
| } else { | ||
| answerRequest = request | ||
| flowOf(LlmResponse(content = modelMessage("Finished"))) | ||
| } | ||
| } | ||
| val agent = | ||
| LlmAgent( | ||
| name = "invoice_agent", | ||
| model = model, | ||
| tools = | ||
| listOf( | ||
| DummyTool(name = "get_currency", declares = true) { _, _ -> | ||
| mapOf("currency" to "USD") | ||
| }, | ||
| LoadArtifactsTool(), | ||
| ), | ||
| ) | ||
| val runner = | ||
| InMemoryRunner(agent = agent, appName = "artifact_app", artifactService = artifacts) | ||
| try { | ||
| val events = | ||
| runner.runAsync("user1", "session1", newMessage = userMessage("Read the invoice")).toList() | ||
| val responses = events.flatMap { it.functionResponses() } | ||
| assertEquals(orderedCalls.map { it.functionCall!!.name }, responses.map { it.name }) | ||
| val artifactResponse = responses.single { it.name == "load_artifacts" } | ||
| assertEquals(listOf(filename), artifactResponse.response["artifact_names"]) | ||
| val nextRequest = assertNotNull(answerRequest) | ||
| assertEquals(2, calls) | ||
| assertTrue( | ||
| nextRequest.contents.flatMap { it.parts }.any { it.text == payload }, | ||
| "A successful load_artifacts response must supply the file to the answering model; " + | ||
| "loadArtifactsFirst=$loadArtifactsFirst", | ||
| ) | ||
| } finally { | ||
| runner.close() | ||
| } | ||
| } |
There was a problem hiding this comment.
In assertArtifactReachesAnsweringModel, please also assert that nextRequest.contents.last() is the expected Content(role = Role.USER, parts = listOf(Part(text = "Artifact $filename is:"), Part(text = payload))) (and add a case or assertion for a "user:"-scoped cross-session artifact in a batched response).
There was a problem hiding this comment.
Done in b4d321b: assertArtifactReachesAnsweringModel now asserts that nextRequest.contents.last() is Content(role = Role.USER, parts = listOf(Part(text = "Artifact $filename is:"), Part(text = payload))). Two new batched cases save the artifact as user:invoice.txt in another session and call get_currency before load_artifacts: one requests user:invoice.txt, the other requests invoice.txt and relies on the user: fallback in loadArtifact. With LoadArtifactsTool.kt from main, both fail, as do the existing after-another-tool and multiple-responses tests; the before-another-tool case still passes.
There was a problem hiding this comment.
Thanks for fixing batched load_artifacts responses and adding the regression tests, @GCSimba! Left two small comments on reverting the unrelated trailing-comma formatting churn and tightening the assertion in LoadArtifactsToolTest, please also rebase onto the latest main.
433fbd2 to
b4d321b
Compare
|
Thanks for the review, @aravindmurali22! Rebased onto the latest |
|
Thank you for the quick response @GCSimba , could you please rebase your branch onto the latest main once again? Thanks! |
b4d321b to
54a7108
Compare
|
@GCSimba Here as well
Appreciate your help with this! |
54a7108 to
a963450
Compare
|
Rebased onto e665796 again; the diff is unchanged. About the failing check: the last
Failed runs
If the new run hits one of these again, could you re-run the failed job? Thanks! |
When another tool precedes
load_artifactsin the same parallel call batch, the loader returns the requested filenames but omits their contents from the next model request because it only inspects the first response part.Scan every part of the latest response message and load artifacts from all matching responses, preserving their order and skipping missing files. This matches Python ADK's handling of batched responses.
Adds five regressions to the existing
LoadArtifactsToolTest:user:-scoped artifact saved in another session, requested with and without theuser:prefix;The runner flows check that the answering model's last content is exactly the loaded artifact. The request-processing test also checks that the original request remains unchanged.
Validation on JDK 21, at b4d321b (rebased onto b961c9e):
LoadArtifactsToolTest: 19/19 JVM and 19/19 Android host tests passed, with no skips. WithLoadArtifactsTool.ktfrommain, 4 of the 19 fail: the after-another-tool flow, bothuser:flows, and the multiple-responses test. The before-another-tool flow still passes, since the old loader read only the first response part.buildpassed on Ubuntu 24.04 (WSL2): 500 tasks (458 executed; the other 42 were up to date from the targeted runs in the same checkout). 10,584 tests passed with no failures; 18 skipped XML records are live Gemini/Firebase or local-model tests without credentials/models. No test exclusions or test-disable flags were used, integration retries remained at repository defaults, and the build left the working tree clean.git diff --checkpassed.ktfmt --google-style(0.65) would only add trailing commas to the new lines, as it would throughout the existing files.After rebasing onto e863fac (now 54a7108, same diff),
LoadArtifactsToolTestpassed again on the new base: 19/19 JVM and 19/19 Android host. WithLoadArtifactsTool.ktfrom the newmain, the same 4 tests fail.The Linux runs used
-PjdkVersion=21,-Dorg.gradle.jvmargs="-Xmx3g -XX:MaxMetaspaceSize=1g",-Dorg.gradle.parallel=false,--max-workers=2,--no-daemon,--stacktrace, and--continue. On-device tests, real backend/model execution, and the JDK 17/25 matrix were not run locally.This change was drafted with AI assistance (GitHub Copilot); I reviewed the diff and the validation results above.