feat: file system support for ls, mkdir, rm, push, pull and get app path

[gmegidish]

No description provided.

[coderabbitai]

Walkthrough

Adds a Filesystem capability and an apps path command across the codebase. CLI: new top-level fs command with subcommands push, pull, ls, mkdir, rm, plus apps path [bundle_id]. Server: registers JSON‑RPC methods device.fs.* and device.apps.path, implements base64 file transfer with a 1 MB limit. Devices: Android implements full FS ops and container path resolution; Simulator implements full FS ops; iOS implements container path and listing (push/pull/mkdir/rm stubbed); Remote stubs. Tests: emulator filesystem integration tests added.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 9.52% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	No pull request description was provided by the author, making it impossible to assess relevance to the changeset.	Add a pull request description that explains the motivation, implementation approach, and any testing performed for the filesystem feature additions.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title accurately summarizes the main changes: filesystem command support (ls, mkdir, rm, push, pull) and app path retrieval functionality.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat-working-with-filesystem

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

cli/root.go (1)

124-142: ⚡ Quick win

Add an apps path example in root help for discoverability.

Since app-container workflows are shown here, adding one mobilecli apps path <bundle-id> --device <device-id> example would make the new path capability easier to discover from --help.

🤖 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 `@cli/root.go` around lines 124 - 142, Add a short example for the new apps
path subcommand to the CLI root help text: locate the help/examples string that
contains the FILESYSTEM block in root.go (the same block showing fs
ls/pull/push/mkdir/rm) and insert a line demonstrating usage of the apps path
command, e.g. "mobilecli apps path <bundle-id> --device <device-id>", so the
apps path capability is discoverable from --help; update the help string near
the FILESYSTEM examples (the root command help definition) to include this new
example.

🤖 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 `@cli/apps.go`:
- Around line 181-184: Remove the stray blank line between the
appsCmd.AddCommand(appsPathCmd) call and the
appsLaunchCmd.Flags().StringVar(...) call to satisfy gofmt; open the function
containing appsCmd, locate the block where appsCmd.AddCommand(appsPathCmd) and
appsLaunchCmd.Flags().StringVar(&deviceId, "device", "", "ID of the device to
launch app on") appear and delete the extra empty line so the two statements are
adjacent.

In `@devices/ios_fs.go`:
- Around line 108-158: The ListFiles method in IOSDevice never sets
FileEntry.ModTime, causing inconsistency with Android/Simulator; update
IOSDevice.ListFiles to populate ModTime from the AFC file info returned by
client.Stat (and the single-file stat path) — when you call
client.Stat(fullPath) or client.Stat(remotePath) extract the modification time
(e.g., info.ModTime or the appropriate timestamp field on the returned info) and
assign it to FileEntry.ModTime for both the single-file branch and the directory
listing loop so ModTime is consistently populated.

In `@README.md`:
- Line 279: Update the example heading text that currently reads "Example output
for `apps fs ls`" to the correct command label "Example output for `fs ls`" so
the README example matches the actual command; locate and edit the heading
string in the README where the example output for the file-system listing is
shown and replace `apps fs ls` with `fs ls`.
- Around line 227-305: Update the wording in the "Access files on the device..."
section to remove the contradiction by changing the phrase "Pulling binary files
(images, databases, DEX files) is fully supported and binary-safe on all
platforms." to either "…on all supported platforms." or explicitly "…on Android
and iOS Simulator." Locate and edit the notes block (the paragraph starting with
"Pulling binary files...") in the README's file-system section so the platform
claim matches the earlier sentence that support is currently limited to Android
and iOS Simulator.

---

Nitpick comments:
In `@cli/root.go`:
- Around line 124-142: Add a short example for the new apps path subcommand to
the CLI root help text: locate the help/examples string that contains the
FILESYSTEM block in root.go (the same block showing fs ls/pull/push/mkdir/rm)
and insert a line demonstrating usage of the apps path command, e.g. "mobilecli
apps path <bundle-id> --device <device-id>", so the apps path capability is
discoverable from --help; update the help string near the FILESYSTEM examples
(the root command help definition) to include this new example.

🪄 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: 18213652-dcd5-4af8-8273-195639792576

📥 Commits

Reviewing files that changed from the base of the PR and between 732c64c and e178dc4.

📒 Files selected for processing (12)

• README.md
• cli/apps.go
• cli/fs.go
• cli/root.go
• commands/apps.go
• commands/fs.go
• devices/android.go
• devices/android_fs.go
• devices/common.go
• devices/ios_fs.go
• devices/remote_fs.go
• devices/simulator_fs.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fix gofmt violation blocking CI.

Line 183 has an extra blank line in this block; CI is already failing on go fmt ./... for this area.

Proposed fix

 	appsCmd.AddCommand(appsForegroundCmd)
 	appsCmd.AddCommand(appsPathCmd)
-
 
 	appsLaunchCmd.Flags().StringVar(&deviceId, "device", "", "ID of the device to launch app on")

🤖 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 `@cli/apps.go` around lines 181 - 184, Remove the stray blank line between the
appsCmd.AddCommand(appsPathCmd) call and the
appsLaunchCmd.Flags().StringVar(...) call to satisfy gofmt; open the function
containing appsCmd, locate the block where appsCmd.AddCommand(appsPathCmd) and
appsLaunchCmd.Flags().StringVar(&deviceId, "device", "", "ID of the device to
launch app on") appear and delete the extra empty line so the two statements are
adjacent.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Inconsistent ModTime population across platforms.

The iOS ListFiles implementation never sets the ModTime field in FileEntry results (both for single files at lines 137-142 and directory listings at lines 145-156). This creates an inconsistency with Android (devices/android_fs.go:144) and Simulator (devices/simulator_fs.go:92), which both populate ModTime.

Consider populating ModTime from AFC file info if available, or documenting why iOS cannot provide modification times.

🤖 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 `@devices/ios_fs.go` around lines 108 - 158, The ListFiles method in IOSDevice
never sets FileEntry.ModTime, causing inconsistency with Android/Simulator;
update IOSDevice.ListFiles to populate ModTime from the AFC file info returned
by client.Stat (and the single-file stat path) — when you call
client.Stat(fullPath) or client.Stat(remotePath) extract the modification time
(e.g., info.ModTime or the appropriate timestamp field on the returned info) and
assign it to FileEntry.ModTime for both the single-file branch and the directory
listing loop so ModTime is consistently populated.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify platform-support wording to avoid contradiction.

Line 227 limits support to Android/iOS Simulator, but Line 305 says binary-safe pull works on “all platforms.” Please align this phrasing (for example, “all supported platforms”) so expectations are clear.

🤖 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 `@README.md` around lines 227 - 305, Update the wording in the "Access files on
the device..." section to remove the contradiction by changing the phrase
"Pulling binary files (images, databases, DEX files) is fully supported and
binary-safe on all platforms." to either "…on all supported platforms." or
explicitly "…on Android and iOS Simulator." Locate and edit the notes block (the
paragraph starting with "Pulling binary files...") in the README's file-system
section so the platform claim matches the earlier sentence that support is
currently limited to Android and iOS Simulator.

[coderabbitai]

Actionable comments posted: 3

🤖 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 `@server/server.go`:
- Around line 1589-1595: The code currently reads the entire file with
os.ReadFile(tmpPath) before checking fsSizeLimit which can spike memory; change
the flow in the function handling the pulled file so you stat the file first
(use os.Stat on tmpPath), compare fi.Size() to fsSizeLimit and return an error
if too large, and only then read the file into memory; reference tmpPath,
fsSizeLimit and the existing os.ReadFile call so you replace the pre-check with
a size check using os.Stat (or open the file and use io.CopyN/LimitReader)
before loading.
- Around line 1615-1621: The code currently calls
base64.StdEncoding.DecodeString(p.Content) before checking size, so huge base64
input can allocate large memory; instead compute the decoded length using
base64.StdEncoding.DecodedLen(len(p.Content)) and if that decoded length exceeds
fsSizeLimit return the existing "file too large" error before calling
DecodeString; update the block around p.Content, fsSizeLimit and DecodeString to
perform this pre-check and only decode when the decoded size is within limits.

In `@test/emulator.ts`:
- Around line 264-303: mobilecliJson and the related mobilecli call are
constructing shell strings and are vulnerable to injection and breaking on
spaces; replace execSync string-based calls with execFileSync and pass the
mobilecli binary plus each argument as an argv array (e.g., split the command
into explicit args for subcommands like 'apps','path', packageName, '--device',
deviceId or 'fs','ls', remotePath, etc.), preserve options (encoding, timeout,
stdio, env) and keep JSON.parse(result) in mobilecliJson; update fsPush, fsPull,
fsMkdir, fsRm, getAppPath and fsList callers to build and pass proper argv
arrays to mobilecli/ mobilecliJson so all dynamic values (deviceId, remotePath,
packageName, localPath) are passed as arguments rather than interpolated into a
shell string.

🪄 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: 52a136ee-a514-46ae-b554-120c4e8b2167

📥 Commits

Reviewing files that changed from the base of the PR and between e178dc4 and 35fcbbd.

📒 Files selected for processing (5)

• devices/android_fs.go
• devices/ios_fs.go
• server/dispatch.go
• server/server.go
• test/emulator.ts

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

• devices/ios_fs.go
• devices/android_fs.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Check pulled file size before loading it into memory.

Line 1589 reads the full file before enforcing fsSizeLimit; oversized pulls can spike memory before rejection.

Suggested fix

-	data, err := os.ReadFile(tmpPath)
+	info, err := os.Stat(tmpPath)
+	if err != nil {
+		return nil, fmt.Errorf("failed to stat pulled file: %w", err)
+	}
+	if info.Size() > int64(fsSizeLimit) {
+		return nil, fmt.Errorf("file too large (%d bytes); maximum allowed size for JSON-RPC transfer is 1 MB", info.Size())
+	}
+
+	data, err := os.ReadFile(tmpPath)
 	if err != nil {
 		return nil, fmt.Errorf("failed to read pulled file: %w", err)
 	}
-	if len(data) > fsSizeLimit {
-		return nil, fmt.Errorf("file too large (%d bytes); maximum allowed size for JSON-RPC transfer is 1 MB", len(data))
-	}

📝 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.

Suggested change

	data, err := os.ReadFile(tmpPath)
	if err != nil {
	return nil, fmt.Errorf("failed to read pulled file: %w", err)
	}
	if len(data) > fsSizeLimit {
	return nil, fmt.Errorf("file too large (%d bytes); maximum allowed size for JSON-RPC transfer is 1 MB", len(data))
	}
	info, err := os.Stat(tmpPath)
	if err != nil {
	return nil, fmt.Errorf("failed to stat pulled file: %w", err)
	}
	if info.Size() > int64(fsSizeLimit) {
	return nil, fmt.Errorf("file too large (%d bytes); maximum allowed size for JSON-RPC transfer is 1 MB", info.Size())
	}
	data, err := os.ReadFile(tmpPath)
	if err != nil {
	return nil, fmt.Errorf("failed to read pulled file: %w", err)
	}

🤖 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 `@server/server.go` around lines 1589 - 1595, The code currently reads the
entire file with os.ReadFile(tmpPath) before checking fsSizeLimit which can
spike memory; change the flow in the function handling the pulled file so you
stat the file first (use os.Stat on tmpPath), compare fi.Size() to fsSizeLimit
and return an error if too large, and only then read the file into memory;
reference tmpPath, fsSizeLimit and the existing os.ReadFile call so you replace
the pre-check with a size check using os.Stat (or open the file and use
io.CopyN/LimitReader) before loading.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject oversized base64 payloads before decode.

Line 1615 decodes first and checks size second; a large content can allocate heavily before being rejected.

Suggested fix

+	maxEncoded := base64.StdEncoding.EncodedLen(fsSizeLimit)
+	if len(p.Content) > maxEncoded {
+		return nil, fmt.Errorf("file too large; maximum allowed size for JSON-RPC transfer is 1 MB")
+	}
+
 	data, err := base64.StdEncoding.DecodeString(p.Content)
 	if err != nil {
 		return nil, fmt.Errorf("'content' is not valid base64: %w", err)
 	}

📝 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.

Suggested change

	data, err := base64.StdEncoding.DecodeString(p.Content)
	if err != nil {
	return nil, fmt.Errorf("'content' is not valid base64: %w", err)
	}
	if len(data) > fsSizeLimit {
	return nil, fmt.Errorf("file too large (%d bytes); maximum allowed size for JSON-RPC transfer is 1 MB", len(data))
	}
	maxEncoded := base64.StdEncoding.EncodedLen(fsSizeLimit)
	if len(p.Content) > maxEncoded {
	return nil, fmt.Errorf("file too large; maximum allowed size for JSON-RPC transfer is 1 MB")
	}
	data, err := base64.StdEncoding.DecodeString(p.Content)
	if err != nil {
	return nil, fmt.Errorf("'content' is not valid base64: %w", err)
	}
	if len(data) > fsSizeLimit {
	return nil, fmt.Errorf("file too large (%d bytes); maximum allowed size for JSON-RPC transfer is 1 MB", len(data))
	}

🤖 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 `@server/server.go` around lines 1615 - 1621, The code currently calls
base64.StdEncoding.DecodeString(p.Content) before checking size, so huge base64
input can allocate large memory; instead compute the decoded length using
base64.StdEncoding.DecodedLen(len(p.Content)) and if that decoded length exceeds
fsSizeLimit return the existing "file too large" error before calling
DecodeString; update the block around p.Content, fsSizeLimit and DecodeString to
perform this pre-check and only decode when the decoded size is within limits.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify remaining dynamic shell-command execSync usage in this file.
rg -nP --type=ts 'execSync\(\s*`|execSync\(\s*["'\''].*\$\{.*["'\'']\s*\)' test/emulator.ts

Repository: mobile-next/mobilecli

Length of output: 126

---

🏁 Script executed:

# Find the mobilecli function definition (not mobilecliJson)
rg -n -A 10 'function mobilecli\(' test/emulator.ts

Repository: mobile-next/mobilecli

Length of output: 445

---

🏁 Script executed:

# Check for all execSync calls in the file
rg -n 'execSync' test/emulator.ts

Repository: mobile-next/mobilecli

Length of output: 272

---

🏁 Script executed:

# Get file length to understand if we can read the entire file
wc -l test/emulator.ts

Repository: mobile-next/mobilecli

Length of output: 86

---

🏁 Script executed:

cat -n test/emulator.ts | head -250 | tail -100

Repository: mobile-next/mobilecli

Length of output: 4220

---

🏁 Script executed:

cat -n test/emulator.ts | tail -70

Repository: mobile-next/mobilecli

Length of output: 3001

---

Fix shell injection vulnerability in both mobilecli() and mobilecliJson() by using argv-based execution.

Both mobilecli() (line 211) and mobilecliJson() (line 264) build shell commands through string interpolation with dynamic values like deviceId, remotePath, packageName, and localPath. This enables command injection and breaks on paths with spaces or special characters. Switch both functions to execFileSync() with argument arrays.

🧰 Tools

🪛 OpenGrep (1.20.0)

[ERROR] 266-271: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.

(coderabbit.command-injection.exec-js)

🤖 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/emulator.ts` around lines 264 - 303, mobilecliJson and the related
mobilecli call are constructing shell strings and are vulnerable to injection
and breaking on spaces; replace execSync string-based calls with execFileSync
and pass the mobilecli binary plus each argument as an argv array (e.g., split
the command into explicit args for subcommands like 'apps','path', packageName,
'--device', deviceId or 'fs','ls', remotePath, etc.), preserve options
(encoding, timeout, stdio, env) and keep JSON.parse(result) in mobilecliJson;
update fsPush, fsPull, fsMkdir, fsRm, getAppPath and fsList callers to build and
pass proper argv arrays to mobilecli/ mobilecliJson so all dynamic values
(deviceId, remotePath, packageName, localPath) are passed as arguments rather
than interpolated into a shell string.

[coderabbitai]

♻️ Duplicate comments (1)

test/emulator.ts (1)

264-303: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replace shell-string execution with argv-based execution in CLI helpers.

mobilecliJson and the FS/app helper callers still construct shell strings from dynamic values. This is injection-prone and also brittle for paths containing spaces/quotes.

Suggested fix (execFileSync + argument arrays)

-import { execSync } from 'child_process';
+import { execFileSync, execSync } from 'child_process';

-function mobilecli(args: string): void {
+function mobilecli(args: string[]): void {
 	const mobilecliBinary = path.join(__dirname, '..', 'mobilecli');
-	const command = `${mobilecliBinary} ${args}`;

 	try {
-		const result = execSync(command, {
+		execFileSync(mobilecliBinary, args, {
 			encoding: 'utf8',
 			timeout: 180000,
 			stdio: ['pipe', 'pipe', 'pipe'],
 			env: {
+				...process.env,
 				ANDROID_HOME: process.env.ANDROID_HOME || "",
 			}
 		});
 	} catch (error: any) {
-		console.log(`Command failed: ${command}`);
+		console.log(`Command failed: ${mobilecliBinary} ${args.map((a) => JSON.stringify(a)).join(' ')}`);
 		if (error.stderr) {
 			console.log(`Error stderr: ${error.stderr}`);
 		}
@@
-function mobilecliJson(args: string): any {
+function mobilecliJson(args: string[]): any {
 	const mobilecliBinary = path.join(__dirname, '..', 'mobilecli');
-	const result = execSync(`${mobilecliBinary} ${args}`, {
+	const result = execFileSync(mobilecliBinary, args, {
 		encoding: 'utf8',
 		timeout: 60000,
 		stdio: ['pipe', 'pipe', 'pipe'],
-		env: { ANDROID_HOME: process.env.ANDROID_HOME || '' },
+		env: { ...process.env, ANDROID_HOME: process.env.ANDROID_HOME || '' },
 	});
 	return JSON.parse(result);
 }

 function getAppContainerPath(deviceId: string, packageName: string): string {
-	const response = mobilecliJson(`apps path ${packageName} --device ${deviceId}`);
+	const response = mobilecliJson(['apps', 'path', packageName, '--device', deviceId]);
 	expect(response.status).to.equal('ok');
 	return response.data.path;
 }

 function fsList(deviceId: string, remotePath: string): any[] {
-	const response = mobilecliJson(`fs ls --device ${deviceId} "${remotePath}"`);
+	const response = mobilecliJson(['fs', 'ls', '--device', deviceId, remotePath]);
 	expect(response.status).to.equal('ok');
 	return response.data;
 }

 function fsPush(deviceId: string, localPath: string, remotePath: string): void {
-	mobilecli(`fs push --device ${deviceId} "${localPath}" "${remotePath}"`);
+	mobilecli(['fs', 'push', '--device', deviceId, localPath, remotePath]);
 }

 function fsPull(deviceId: string, remotePath: string, localPath: string): void {
-	mobilecli(`fs pull --device ${deviceId} "${remotePath}" "${localPath}"`);
+	mobilecli(['fs', 'pull', '--device', deviceId, remotePath, localPath]);
 }

 function fsMkdir(deviceId: string, remotePath: string, parents: boolean): void {
-	const flag = parents ? '-p ' : '';
-	mobilecli(`fs mkdir --device ${deviceId} ${flag}"${remotePath}"`);
+	const args = ['fs', 'mkdir', '--device', deviceId];
+	if (parents) args.push('-p');
+	args.push(remotePath);
+	mobilecli(args);
 }

 function fsRm(deviceId: string, remotePath: string, recursive: boolean): void {
-	const flag = recursive ? '-r ' : '';
-	mobilecli(`fs rm --device ${deviceId} ${flag}"${remotePath}"`);
+	const args = ['fs', 'rm', '--device', deviceId];
+	if (recursive) args.push('-r');
+	args.push(remotePath);
+	mobilecli(args);
 }

#!/bin/bash
# Verify remaining string-based shell execution patterns in this file.
rg -nP --type=ts 'execSync\(\s*`|execSync\(\s*["'\''].*\$\{.*["'\'']\s*\)' test/emulator.ts
rg -nP --type=ts 'mobilecli(Json)?\(\s*`' test/emulator.ts
# Expected after fix: no matches for dynamic/interpolated shell commands.

🤖 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/emulator.ts` around lines 264 - 303, mobilecliJson and the helper
callers (mobilecli, getAppContainerPath, fsList, fsPush, fsPull, fsMkdir, fsRm)
currently build shell-interpolated command strings which is injection-prone;
change mobilecliJson to use execFileSync (or spawnSync) with an argv array (no
shell=true) and update all callers to pass arguments as separate array entries
(e.g., ['apps','path', packageName, '--device', deviceId] or
['fs','push','--device', deviceId, localPath, remotePath]) instead of string
interpolation, ensuring paths with spaces/quotes are passed as single argv
elements and retaining existing timeout/encoding/env options.

🧹 Nitpick comments (1)

test/emulator.ts (1)

105-111: ⚡ Quick win

Ensure temp files are always cleaned up on failing test paths.

Cleanup currently runs only on success. If fsPush, fsPull, or assertions fail, temp files remain and can accumulate across CI retries.

Suggested fix (use try/finally)

 const localFile = writeTempFile('hello from mobilecli');
-fsPush(deviceId, localFile, remoteFile);
-fs.unlinkSync(localFile);
+try {
+	fsPush(deviceId, localFile, remoteFile);
+} finally {
+	if (fs.existsSync(localFile)) fs.unlinkSync(localFile);
+}

 const localDest = path.join(os.tmpdir(), `mobilecli-pull-${Date.now()}.txt`);
-fsPull(deviceId, remoteFile, localDest);
-const contents = fs.readFileSync(localDest, 'utf8');
-expect(contents.trim()).to.equal('hello from mobilecli');
-fs.unlinkSync(localDest);
+try {
+	fsPull(deviceId, remoteFile, localDest);
+	const contents = fs.readFileSync(localDest, 'utf8');
+	expect(contents.trim()).to.equal('hello from mobilecli');
+} finally {
+	if (fs.existsSync(localDest)) fs.unlinkSync(localDest);
+}

Also applies to: 121-129, 172-178, 188-196

🤖 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/emulator.ts` around lines 105 - 111, The temp files created by
writeTempFile (localFile) must be removed even if fsPush/fsPull or assertions
throw; wrap the test body that uses localFile in a try/finally so
fs.unlinkSync(localFile) runs in finally (guarded by fs.existsSync(localFile) if
needed) and move fs.unlinkSync out of the success path; apply the same
try/finally pattern to the other tests that create localFile and call
fsPush/fsPull (the blocks around fsPush/fsPull and the tests referenced at
121-129, 172-178, 188-196) so temp files are always cleaned up on failure.

🤖 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.

Duplicate comments:
In `@test/emulator.ts`:
- Around line 264-303: mobilecliJson and the helper callers (mobilecli,
getAppContainerPath, fsList, fsPush, fsPull, fsMkdir, fsRm) currently build
shell-interpolated command strings which is injection-prone; change
mobilecliJson to use execFileSync (or spawnSync) with an argv array (no
shell=true) and update all callers to pass arguments as separate array entries
(e.g., ['apps','path', packageName, '--device', deviceId] or
['fs','push','--device', deviceId, localPath, remotePath]) instead of string
interpolation, ensuring paths with spaces/quotes are passed as single argv
elements and retaining existing timeout/encoding/env options.

---

Nitpick comments:
In `@test/emulator.ts`:
- Around line 105-111: The temp files created by writeTempFile (localFile) must
be removed even if fsPush/fsPull or assertions throw; wrap the test body that
uses localFile in a try/finally so fs.unlinkSync(localFile) runs in finally
(guarded by fs.existsSync(localFile) if needed) and move fs.unlinkSync out of
the success path; apply the same try/finally pattern to the other tests that
create localFile and call fsPush/fsPull (the blocks around fsPush/fsPull and the
tests referenced at 121-129, 172-178, 188-196) so temp files are always cleaned
up on failure.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6ca1cf66-30b7-42e8-bc7a-8c3e75af3dba

📥 Commits

Reviewing files that changed from the base of the PR and between 35fcbbd and 6f4f4a5.

📒 Files selected for processing (5)

• README.md
• cli/fs.go
• commands/apps.go
• devices/android_fs.go
• test/emulator.ts

✅ Files skipped from review due to trivial changes (1)

• README.md

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

• commands/apps.go
• cli/fs.go
• devices/android_fs.go