Repository navigation
feat(mcp): add upload_file tool - #3258
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new MCP upload_file tool to Arthas core that accepts a basename + Base64 payload (and optional SHA-256), writes the decoded content into a content-addressed upload root, and returns stable metadata (artifactId, sha256, targetPath, etc.) for downstream tools (mc, redefine, retransform, profiler) to consume.
Changes:
- Introduces
UploadFileToolimplementing validation (filename/type/Base64/size/checksum/quota) and atomic commit into a content-addressed directory layout. - Adds
UploadFileToolTestcovering success, idempotent reuse, and rejection cases (unsafe names, unsupported types, invalid Base64/checksum, quota, symlinked digest dir, schema/scanner discovery).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/src/main/java/com/taobao/arthas/core/mcp/tool/function/basic1000/UploadFileTool.java | New upload_file MCP tool implementation: validation, quota enforcement, and atomic commit to a content-addressed upload root. |
| core/src/test/java/com/taobao/arthas/core/mcp/tool/function/basic1000/UploadFileToolTest.java | New unit tests for upload success, reuse/idempotency, and various validation/safety failure modes. |
Suppressed comments (1)
core/src/main/java/com/taobao/arthas/core/mcp/tool/function/basic1000/UploadFileTool.java:277
commitAtomically(...)verifies the existing artifact onFileAlreadyExistsException, but it doesn't surface to the caller whether the commit actually wrote a new file or reused an existing one. Returning a boolean here allows the caller to correctly setreusedand message fields.
private void commitAtomically(Path target, byte[] content, String expectedSha256) {
Path temp = null;
try {
temp = Files.createTempFile(target.getParent(), ".upload-", ".part");
setFilePermissions(temp);
try (FileChannel channel = FileChannel.open(temp, StandardOpenOption.WRITE,
StandardOpenOption.TRUNCATE_EXISTING)) {
ByteBuffer buffer = ByteBuffer.wrap(content);
while (buffer.hasRemaining()) {
channel.write(buffer);
}
channel.force(true);
}
try {
Files.move(temp, target, StandardCopyOption.ATOMIC_MOVE);
temp = null;
} catch (FileAlreadyExistsException e) {
verifyExistingArtifact(target, expectedSha256, content.length);
} catch (AtomicMoveNotSupportedException e) {
throw uploadError("IO_ERROR", "The upload directory does not support atomic file commits", e);
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } else { | ||
| assertQuotaAvailable(content.length); | ||
| ensureArtifactDirectory(artifactDirectory); | ||
| commitAtomically(target, content, sha256); | ||
| reused = false; | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 817fccd6d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /** | ||
| * Upload a small file to the target JVM filesystem for use by other Arthas MCP tools. | ||
| */ |
There was a problem hiding this comment.
This class introduces English Javadoc here and additional English inline comments in ensureArtifactDirectory, setDirectoryPermissions, and setFilePermissions, contrary to the repository requirement that all code comments be written in Chinese; translate these comments to keep the new code compliant.
AGENTS.md reference: AGENTS.md:L243-L243
Useful? React with 👍 / 👎.
| String lowerCaseName = fileName.toLowerCase(Locale.ROOT); | ||
| if (lowerCaseName.endsWith(".class")) { | ||
| return "CLASS"; | ||
| } | ||
| if (lowerCaseName.endsWith(".java")) { | ||
| return "JAVA_SOURCE"; |
There was a problem hiding this comment.
.java suffix
When fileName is Demo.JAVA, validation lowercases only the comparison while preserving the uppercase target name, so the upload reports a usable JAVA_SOURCE; however, MemoryCompilerCommand.java:130-133 strips only the literal lowercase .java, passes Demo.JAVA as the source class name, and produces a compiler URI ending in /Demo/JAVA.java, causing a normal public class Demo source to fail compilation. Reject non-lowercase Java suffixes or normalize the stored filename.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
core/src/main/java/com/taobao/arthas/core/mcp/tool/function/basic1000/UploadFileTool.java:104
- upload_file 在校验失败时通过抛出 UploadFileException 走 MCP 的异常通道返回错误，这会导致客户端拿不到与成功响应一致的 JSON 结构（例如 status/stage），也不便于程序化处理 code。建议在 uploadFile() 中捕获 UploadFileException/IllegalArgumentException，并返回一个结构化的 error 响应（至少包含 status="error"、code、message）。
if (!uploadPermit.tryAcquire()) {
throw uploadError("UPLOAD_BUSY", "Another file upload is in progress");
}
try {
return upload(fileName, contentBase64, expectedSha256);
} finally {
uploadPermit.release();
}
}
core/src/main/java/com/taobao/arthas/core/mcp/tool/function/basic1000/UploadFileTool.java:376
- 默认上传目录通过 Files.createTempDirectory("arthas-mcp-uploads-") 创建，但当前没有任何清理策略（例如 JVM 退出清理或复用固定目录）。如果 Arthas MCP 频繁重启，系统临时目录下可能累积多个遗留 uploads 目录并占用磁盘，和“temporary directory”的预期不一致。建议补充生命周期清理或改为可复用的稳定目录策略。
private static Path createDefaultUploadRoot() {
try {
return Files.createTempDirectory("arthas-mcp-uploads-");
} catch (IOException e) {
throw new IllegalStateException("Failed to create MCP upload directory", e);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
core/src/main/java/com/taobao/arthas/core/mcp/tool/function/basic1000/UploadFileTool.java:368
Files.createTempDirectorydoes not remove the directory automatically, and the MCP server shutdown path has no upload cleanup. Consequently, each target JVM can leave its entire upload quota behind after termination, causing persistent disk growth across JVM churn. Attach this root to an ownership-aware shutdown/cleanup lifecycle (while retaining it for the JVM lifetime).
private static Path createDefaultUploadRoot() {
try {
return Files.createTempDirectory("arthas-mcp-uploads-");
core/src/main/java/com/taobao/arthas/core/mcp/tool/function/basic1000/UploadFileTool.java:40
- The default quota contradicts the PR contract: the description specifies 64 MiB per JVM, but this permits 200 MiB and the new test explicitly expects that value. Please align the implementation and test with the documented 64 MiB limit (or update the PR contract if 200 MiB is intentional).
This issue also appears on line 366 of the same file.
static final long DEFAULT_MAX_TOTAL_BYTES = 200L * 1024 * 1024;
core/src/test/java/com/taobao/arthas/core/mcp/tool/function/basic1000/UploadFileToolTest.java:188
- This test codifies the opposite of the stated 64-artifact per-JVM quota. The implementation has no artifact-count check, so arbitrarily many tiny uploads can consume inodes and make every quota scan increasingly expensive. Enforce the 64-artifact limit and assert that the 65th distinct artifact is rejected, while identical retries remain reusable.
@Test
public void shouldNotLimitArtifactCount() {
for (int i = 0; i < 65; i++) {
tool.uploadFile("Artifact" + i + ".class",
Base64.getEncoder().encodeToString(new byte[] {(byte) i}), null);
}
Summary
upload_filetool withfileName, Base64 content, and optional SHA-256 inputs.class,.java, and.jfcfiles in a private, content-addressed temporary directory while preserving the original basename formctargetPaththat existingmc,redefine,retransform, andprofilertools can consumeWhy
MCP does not currently provide a dedicated client-to-server binary upload primitive. A bounded Base64 tool keeps the first version compatible with the existing Streamable and Stateless transports without adding a proprietary HTTP endpoint.
Behavior
.class,.java, and.jfcValidation
./mvnw -pl core -am -Dtest=UploadFileToolTest -Dsurefire.failIfNoSpecifiedTests=false test./mvnw -pl arthas-mcp-integration-test -am -Dtest=ArthasMcpJavaSdkIT -Dsurefire.failIfNoSpecifiedTests=false verifytools/listcoverage and a realupload_filecallviewfile, andvmtoolThe unit test run emits the repository's existing Logback test-appender compatibility warning; it does not fail the build.