Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Deterministic, content-addressed Base64 media temp files — the previously random Files.createTempFile path made tool-result formatting non-reproducible and spawned a new file per call. The rewrite is correct and carefully hardened: SHA-256 content identity, striped locking, staging file + ATOMIC_MOVE publication with fallback, NOFOLLOW_LINKS checks that reject symlink/irregular targets, MIME subtype sanitisation, and existing-file validation instead of silent overwrite. Error handling preserves the primary failure and suppresses cleanup errors. Shared by both AbstractBaseFormatter and GeminiMessageConverter, so the fix lands consistently across model providers. Test coverage is excellent (content identity incl. padding, concurrency ×8, atomic and non-atomic moves, symlink rejection, cleanup-failure and SecurityException paths, plus an end-to-end DashScope determinism check). CLA signed, CI green on all platforms. Two non-blocking performance/operability notes are pinned inline; neither affects correctness. Approved.
Automated review by github-manager-bot
|
|
||
| synchronized (materializationLock(target)) { | ||
| if (Files.exists(target, LinkOption.NOFOLLOW_LINKS)) { | ||
| validateMaterializedFile(target, decodedData); |
There was a problem hiding this comment.
Non-blocking: on the reuse path the full payload is re-read and compared byte-by-byte on every format call. For large media that is re-formatted repeatedly in an agent loop (image/audio tool results), this turns the "reuse" fast path into a repeated O(size) disk read. Consider caching verified targets in-process (e.g. a small set of already-validated absolute paths, or verifying only when Files.size(target) != decodedData.length differs), keeping the full validation for the publication path where it is genuinely needed.
| synchronized (MediaUtils.class) { | ||
| directory = materializedMediaDirectory; | ||
| if (directory == null) { | ||
| directory = Files.createTempDirectory("agentscope_media_"); |
There was a problem hiding this comment.
Non-blocking: the per-process agentscope_media_* directory is never evicted or cleaned (Runtime.deleteOnExit is explicitly out of scope here and would not help for long-running servers). This is no worse than the previous per-call Files.createTempFile behaviour, but for long-lived harness/distribution processes streaming many distinct tool results it is unbounded growth in the OS temp dir. Worth a follow-up issue for a size/count-bounded eviction policy, or documenting that operators should prune java.io.tmpdir/agentscope_media_*.
AgentScope-Java Version
2.0.3-SNAPSHOT, based onmain@594343c6ef29f26eea3cbbb2d6f92b1e5b9696deDescription
Fixes #3199
Formatting the same unchanged message history could previously produce different tool-result text when Base64-backed media was converted to a temporary-file reference.
The previous implementations in
AbstractBaseFormatterandGeminiMessageConverterusedFiles.createTempFile(...), so every formatting pass embedded a new random path into the provider request.Impact before this change
Changes
This change:
MediaUtils;AbstractBaseFormatterandGeminiMessageConverter.Impact after this change
Within the same JVM, identical decoded media content and the same media type reuse the same materialized path. The serialized tool-result text therefore remains byte-stable when the unchanged message history is formatted repeatedly.
This preserves the opportunity for providers with prompt or prefix caching to reuse the same historical prefix. It avoids actively invalidating cache reuse merely because the formatter created a new random temporary path.
The change does not guarantee cache hits. Actual cache behavior remains provider-specific and depends on cache policy, cache boundaries, minimum prefix lengths, expiration, token accounting, and billing rules.
Different decoded media content produces different content-addressed targets. The formatter remains stateless, and existing formatter method signatures and serialized text structure remain unchanged.
The deterministic guarantee is process-local. Cross-process path determinism, temporary-file TTL, global cleanup scheduling, and broader multimodal protocol changes are intentionally outside this PR.
Invalid existing targets fail closed rather than being overwritten.
Compatibility
Verification
mvn spotless:check: passed.mvn -DskipTests ... compilefor the affected modules and reactor dependencies: passed.git show --check: passed.MediaUtilsTest: 41/41;DashScopeMultiAgentFormatterTest: 37/37;GeminiMessageConverterTest: 34/34.A subsequent local rerun was blocked by an environment-level Mockito/Byte Buddy inline-agent attachment failure in the test JVM. The affected failure is in the local test runtime's agent-attachment mechanism; CI verification remains required before claiming the checks are green.
The tests cover:
Non-goals
Checklist