fix: move_files newPath 参数计算规则 #725

wangle201210 · 2025-08-12T01:58:26Z

fix: move_files newPath 参数计算规则

Summary by CodeRabbit

New Features
- Enhanced move/rename behavior: you can move files to an explicit destination path (including renaming) or into a destination folder as before.
Improvements
- More accurate detection of whether the destination is a file or directory for correct move semantics.
- Clearer per-item error messages and maintained parallel processing for faster operations.

coderabbitai · 2025-08-12T01:58:34Z

Walkthrough

move_files now prefetches destination metadata and, per source, chooses validation path based on whether the destination is a file or directory (using destInfo.isFile). Each source is validated and renamed accordingly; errors and success messages are reported per-source.

Changes

Cohort / File(s)	Summary
Filesystem move logic `src/main/presenter/mcpPresenter/inMemoryServers/filesystem.ts`	Prefetch destination metadata (`destInfo`) once. For each source, pick validation path based on `destInfo.isFile` (use destination as-is for files, or destination + basename for directories), validate that path, then `fs.rename` to the computed `validDestPath`. Per-source error formatting updated; operations run in parallel via `Promise.all`. No public API/signature changes.

Sequence Diagram(s)

sequenceDiagram
  actor Client
  participant Presenter as mcpPresenter
  participant FS as FileSystem

  Client->>Presenter: move_files(sources[], destination)
  Presenter->>FS: getFileStats(destination) --> destInfo
  alt destInfo.isFile == true
    Presenter->>Presenter: for each source -> validDest = validate(destination)
  else destInfo.isFile == false
    Presenter->>Presenter: for each source -> validDest = validate(destination + "/" + basename(source))
  end
  loop for each source (parallel)
    Presenter->>FS: rename(source, validDest)
    FS-->>Presenter: success / error
    Presenter-->>Client: per-source success/error message
  end

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

I hopped along a single trail,
Peeked at the goal to set the tale,
No stitching names, one path to trust—
I moved each file with tiny thrust.
A tidy warren, neat and hale. 🐇

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 16bd66f and 8a839a0.

📒 Files selected for processing (1)

src/main/presenter/mcpPresenter/inMemoryServers/filesystem.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/main/presenter/mcpPresenter/inMemoryServers/filesystem.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: build-check (x64)

✨ Finishing Touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

Visit our Status Page to check the current availability of CodeRabbit.
Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 0

🔭 Outside diff range comments (1)

src/main/presenter/mcpPresenter/inMemoryServers/filesystem.ts (1)

1031-1040: Critical: destination handling now breaks multi-source moves and directory semantics

Using parsed.data.destination as the exact target for every source means:

All sources are renamed to the same path (data loss or immediate failures).
Moving into an existing directory no longer works, since fs.rename requires the final target path (directory + basename), not the directory path itself.

This regresses the previously correct behavior of moving multiple sources into a directory. Also, validating the same destination inside the loop is redundant.

Proposed fix: validate destination once, detect whether it is an existing directory, enforce directory requirement for multi-source moves, and compute a per-source target path accordingly. Also return meaningful error messages instead of JSON.stringify(e) (which yields {} for Error).

Apply this diff to the move_files case:

           case 'move_files': {
             const parsed = MoveFilesArgsSchema.safeParse(args)
             if (!parsed.success) {
               throw new Error(`Invalid arguments for move_files: ${parsed.error}`)
             }
-            const results = await Promise.all(
-              parsed.data.sources.map(async (source) => {
-                const validSourcePath = await this.validatePath(source)
-                const validDestPath = await this.validatePath(parsed.data.destination)
-                try {
-                  await fs.rename(validSourcePath, validDestPath)
-                  return `Successfully moved ${source} to ${parsed.data.destination}`
-                } catch (e) {
-                  return `Move ${source} to ${parsed.data.destination} failed: ${JSON.stringify(e)}`
-                }
-              })
-            )
+            const destinationInput = parsed.data.destination
+            // Validate destination once and determine whether it's an existing directory
+            const validatedDestination = await this.validatePath(destinationInput)
+            let destinationIsDirectory = false
+            try {
+              const destStat = await fs.stat(validatedDestination)
+              destinationIsDirectory = destStat.isDirectory()
+            } catch {
+              // Destination does not exist yet (allowed when moving a single source only)
+              destinationIsDirectory = false
+            }
+
+            // If multiple sources, destination must be an existing directory
+            if (parsed.data.sources.length > 1 && !destinationIsDirectory) {
+              return {
+                content: [
+                  {
+                    type: 'text',
+                    text: `Destination must be an existing directory when moving multiple sources: ${destinationInput}`
+                  }
+                ],
+                isError: true
+              }
+            }
+
+            const results = await Promise.all(
+              parsed.data.sources.map(async (source) => {
+                try {
+                  const validSourcePath = await this.validatePath(source)
+                  const destPathForSource = destinationIsDirectory
+                    ? path.join(validatedDestination, path.basename(validSourcePath))
+                    : validatedDestination
+                  await fs.rename(validSourcePath, destPathForSource)
+                  const renderedDest = destinationIsDirectory
+                    ? path.join(destinationInput, path.basename(source))
+                    : destinationInput
+                  return `Successfully moved ${source} to ${renderedDest}`
+                } catch (e) {
+                  const errMsg = e instanceof Error ? e.message : String(e)
+                  return `Move ${source} to ${destinationInput} failed: ${errMsg}`
+                }
+              })
+            )
             return {
               content: [
                 {
                   type: 'text',
                   text: results.join('\n')
                 }
               ]
             }
           }

🧹 Nitpick comments (1)

src/main/presenter/mcpPresenter/inMemoryServers/filesystem.ts (1)

1025-1030: Confirm intended semantics and update schema/description accordingly

If the goal of this PR is to allow specifying a full target path for a single-source rename (without implicit basename join), we should explicitly document and enforce:

When sources.length > 1: destination MUST be an existing directory.

When sources.length === 1: destination may be a directory (auto-join basename) or a full target file path (rename).

Consider updating the MoveFilesArgsSchema description and the tool description to reflect this rule for clarity.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3feafbc and 16bd66f.

📒 Files selected for processing (1)

src/main/presenter/mcpPresenter/inMemoryServers/filesystem.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (8)

**/*.{ts,tsx,js,jsx,vue}