Conversation
WalkthroughThis PR adds six filesystem operations and app container path lookup to the mobilecli module. Three new type interfaces define the response shapes for file listings and container paths. The Mobilecli class gains six new methods that execute mobilecli CLI commands and parse JSON responses for list and app-path queries. Six corresponding MCP tools expose these operations to clients with input validation for file transfer operations. Unit tests verify command argument construction, optional flag handling, and response parsing across all new operations. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server.ts`:
- Around line 490-496: The MCP tool annotation for mobile_pull_file is
incorrect: it is marked readOnlyHint: true but the handler calls
mobilecli.fsPull(device, remotePath, localPath) which writes to the local
filesystem; update the annotation for mobile_pull_file to readOnlyHint: false
and add destructiveHint: true if the operation can overwrite or delete existing
files (otherwise destructiveHint: false); keep the existing handler
(ensureMobilecliAvailable, validateOutputPath, mobilecli.fsPull) unchanged—only
modify the annotation object for the mobile_pull_file tool.
In `@test/mobilecli.test.ts`:
- Around line 248-254: The test calls mobilecli.fsList with three args including
a bundleId, but the current API for fsList is fsList(deviceId: string,
remotePath?: string); update the test to call mobilecli.fsList with the deviceId
and optional remotePath only (e.g., mobilecli.fsList("device1", "/Documents"))
and change the expected call assertion to remove the bundleId from the args
(expect ["fs","ls","/Documents","--device","device1"] or adjust for an undefined
remotePath if testing that case); update references in the test to
createMockMobilecli and the calls[0].args assertion accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97800ef8-f685-4270-a25d-f458f06be162
📒 Files selected for processing (3)
src/mobilecli.tssrc/server.tstest/mobilecli.test.ts
| { readOnlyHint: true }, | ||
| async ({ device, remotePath, localPath }) => { | ||
| ensureMobilecliAvailable(); | ||
| validateOutputPath(localPath); | ||
| mobilecli.fsPull(device, remotePath, localPath); | ||
| return `Pulled ${remotePath} to ${localPath}`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In @modelcontextprotocol/sdk (v1.26.0), should a tool that writes a local file be marked with readOnlyHint or destructiveHint?
💡 Result:
In the Model Context Protocol (MCP), a tool that writes to a local file should be marked with readOnlyHint: false [1][2][3]. Because the tool modifies the environment (i.e., it is not read-only), you must explicitly set readOnlyHint to false [2][3]. When readOnlyHint is set to false, you can further use destructiveHint to characterize the nature of the write operation [1][3]: - Set destructiveHint: true if the operation is destructive (e.g., overwriting an existing file, deleting data, or making irreversible changes) [4][2][3]. - Set destructiveHint: false if the operation is purely additive or safe (e.g., appending to a log file or creating a new, non-conflicting file) [1][3][5]. These annotations are hints used by MCP clients to decide whether to auto-approve a tool call or trigger a user confirmation dialog [2][3]. If you do not provide these annotations, clients may default to assuming a tool is non-read-only and destructive, which often leads to unnecessary confirmation prompts for safe operations, or conversely, may fail to protect against dangerous ones if the defaults are misinterpreted [2][3]. Always set these fields accurately to ensure the correct balance between security and agent autonomy [2][3][5].
Citations:
- 1: https://modelcontextprotocol.io/specification/2025-06-18/schema
- 2: https://auditbuffet.com/patterns/ab-001831
- 3: https://blog.modelcontextprotocol.io/posts/2026-03-16-tool-annotations/
- 4: https://github.com/cyanheads/model-context-protocol-resources/blob/main/guides/mcp-server-development-guide.md
- 5: https://blog.marcnuri.com/mcp-tool-annotations-introduction
src/server.ts: Mark mobile_pull_file MCP tool annotation correctly (not read-only)
mobile_pull_file is annotated with readOnlyHint: true but it writes to the local filesystem via mobilecli.fsPull(device, remotePath, localPath). Update the annotation to readOnlyHint: false (and set destructiveHint: true if this operation can overwrite/delete existing files).
Suggested fix
Change { readOnlyHint: true } to { readOnlyHint: false, destructiveHint: ... } (set destructiveHint to true only if overwriting/deleting is possible).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server.ts` around lines 490 - 496, The MCP tool annotation for
mobile_pull_file is incorrect: it is marked readOnlyHint: true but the handler
calls mobilecli.fsPull(device, remotePath, localPath) which writes to the local
filesystem; update the annotation for mobile_pull_file to readOnlyHint: false
and add destructiveHint: true if the operation can overwrite or delete existing
files (otherwise destructiveHint: false); keep the existing handler
(ensureMobilecliAvailable, validateOutputPath, mobilecli.fsPull) unchanged—only
modify the annotation object for the mobile_pull_file tool.
| it("should call fs ls with bundleId and path", () => { | ||
| const { mobilecli, calls } = createMockMobilecli(mockResponse); | ||
| mobilecli.fsList("device1", "com.example.app", "/Documents"); | ||
|
|
||
| assert.equal(calls.length, 1); | ||
| assert.deepEqual(calls[0].args, ["fs", "ls", "com.example.app", "/Documents", "--device", "device1"]); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify declaration vs call-site arity mismatch for fsList
rg -nP '^\s*fsList\s*\(.*\)' src/mobilecli.ts -n -C2
rg -nP '\bfsList\s*\(' test/mobilecli.test.ts -n -C2Repository: mobile-next/mobile-mcp
Length of output: 1096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect fsList implementation
sed -n '240,340p' src/mobilecli.ts
# Show the relevant test cases
sed -n '200,290p' test/mobilecli.test.tsRepository: mobile-next/mobile-mcp
Length of output: 3541
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for other fsList overloads/usages in the implementation file
rg -n "fsList\s*\(" src/mobilecli.ts
# Search the whole repo for fsList call-sites that pass 3 args
rg -n "\.fsList\s*\(\s*[^,]+,\s*[^,]+,\s*[^)]+\)" -S .
# If tsc is available/configured, run a no-emit typecheck to catch TS2554-style errors
ls -la
ls -la tsconfig*.json 2>/dev/null || true
cat package.json 2>/dev/null || true
if [ -f package.json ]; then
if rg -q '"typecheck"|tsc' package.json; then
npm -s run typecheck || npx -s tsc -p tsconfig.json --noEmit --pretty false || true
else
npx -s tsc -p tsconfig.json --noEmit --pretty false || true
fi
fiRepository: mobile-next/mobile-mcp
Length of output: 5522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== tsconfig.json ==="
cat tsconfig.json
echo "=== Running tests (ts-node will typecheck test compilation) ==="
npm test --silentRepository: mobile-next/mobile-mcp
Length of output: 435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
npx mocha --require ts-node/register test/*.tsRepository: mobile-next/mobile-mcp
Length of output: 2200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== tooling presence ==="
ls -la node_modules/ts-node 2>/dev/null || echo "missing node_modules/ts-node"
ls -la node_modules/nyc 2>/dev/null || echo "missing node_modules/nyc"
ls -la node_modules/mocha 2>/dev/null || echo "missing node_modules/mocha"
echo "=== test helper + fsList tests ==="
sed -n '1,220p' test/mobilecli.test.ts
sed -n '220,310p' test/mobilecli.test.tsRepository: mobile-next/mobile-mcp
Length of output: 10525
Fix fsList test to match current API (2-arg signature)
src/mobilecli.tsdefinesfsList(deviceId: string, remotePath?: string), but the test callsmobilecli.fsList("device1", "com.example.app", "/Documents")(unsupported 3rd argument and bundleId isn’t part of the API).
Suggested fix (align tests to current API)
- it("should call fs ls with bundleId and path", () => {
+ it("should call fs ls with remote path", () => {
const { mobilecli, calls } = createMockMobilecli(mockResponse);
- mobilecli.fsList("device1", "com.example.app", "/Documents");
+ mobilecli.fsList("device1", "/Documents");
assert.equal(calls.length, 1);
- assert.deepEqual(calls[0].args, ["fs", "ls", "com.example.app", "/Documents", "--device", "device1"]);
+ assert.deepEqual(calls[0].args, ["fs", "ls", "/Documents", "--device", "device1"]);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should call fs ls with bundleId and path", () => { | |
| const { mobilecli, calls } = createMockMobilecli(mockResponse); | |
| mobilecli.fsList("device1", "com.example.app", "/Documents"); | |
| assert.equal(calls.length, 1); | |
| assert.deepEqual(calls[0].args, ["fs", "ls", "com.example.app", "/Documents", "--device", "device1"]); | |
| }); | |
| it("should call fs ls with remote path", () => { | |
| const { mobilecli, calls } = createMockMobilecli(mockResponse); | |
| mobilecli.fsList("device1", "/Documents"); | |
| assert.equal(calls.length, 1); | |
| assert.deepEqual(calls[0].args, ["fs", "ls", "/Documents", "--device", "device1"]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/mobilecli.test.ts` around lines 248 - 254, The test calls
mobilecli.fsList with three args including a bundleId, but the current API for
fsList is fsList(deviceId: string, remotePath?: string); update the test to call
mobilecli.fsList with the deviceId and optional remotePath only (e.g.,
mobilecli.fsList("device1", "/Documents")) and change the expected call
assertion to remove the bundleId from the args (expect
["fs","ls","/Documents","--device","device1"] or adjust for an undefined
remotePath if testing that case); update references in the test to
createMockMobilecli and the calls[0].args assertion accordingly.
1 participant
Cannot be merged until all platforms move to mobilecli