Conversation
- Add serial node capability with BLE (Nordic UART) and USB serial support - Add ISO 8601 timestamps to all gateway log messages (Kotlin + Dart) - Add android:allowBackup="true" for ADB backup support - Add Bluetooth and USB host permissions to AndroidManifest - Bump version to 1.8.4+15 Closes #21, closes #54, closes #55
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Serial node capability (USB + BLE) with five commands, timestamps gateway logs (ISO 8601 UTC), enables backup/USB/Bluetooth manifest entries, adds an UpdateService and Settings UI to check releases, enhances node capability advertisement and foregrounding, and adds a proot fallback to write openclaw.json. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Flutter App
participant NodeSvc as NodeService
participant Serial as SerialCapability
participant BLE as BLE Stack
participant USB as USB Host
participant Device as Remote Device
rect rgba(100,150,200,0.5)
Note over App,Device: Discovery
App->>NodeSvc: invoke "serial.list"
NodeSvc->>Serial: handle("list")
Serial->>BLE: scan()
Serial->>USB: listDevices()
BLE-->>Serial: ble device list
USB-->>Serial: usb device list
Serial-->>NodeSvc: NodeFrame(device list)
NodeSvc-->>App: NodeFrame
end
rect rgba(150,100,200,0.5)
Note over App,Device: Connect
App->>NodeSvc: invoke "serial.connect" (deviceId)
NodeSvc->>Serial: handle("connect", deviceId)
alt deviceId starts with "ble:"
Serial->>BLE: connect(deviceId)
BLE->>Device: establish NUS
Device-->>BLE: connected
else deviceId starts with "usb:"
Serial->>USB: open(deviceId)
USB->>Device: claim interface
Device-->>USB: opened
end
Serial-->>NodeSvc: NodeFrame(success)
NodeSvc-->>App: NodeFrame
end
rect rgba(100,200,150,0.5)
Note over App,Device: Data transfer
App->>NodeSvc: "serial.write" / "serial.read"
NodeSvc->>Serial: handle("write"/"read")
Serial->>BLE: write/read TX/RX char
Serial->>USB: write/read endpoint
Device-->>Serial: data
Serial-->>NodeSvc: NodeFrame(data/ack)
NodeSvc-->>App: NodeFrame
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="flutter_app/lib/services/capabilities/serial_capability.dart">
<violation number="1" location="flutter_app/lib/services/capabilities/serial_capability.dart:296">
P2: `conn.close()` is async (`Future<void>`) but is called without `await` inside a synchronous `dispose()`. The `try/catch` won't catch async errors, potentially causing unhandled Future exceptions. At minimum, chain `.catchError((_) {})` on the Future, or make `dispose()` return `Future<void>`.</violation>
<violation number="2" location="flutter_app/lib/services/capabilities/serial_capability.dart:366">
P1: BLE `read()` creates a second subscription to `onValueReceived` while `_bleSubscription` (from the constructor) is still active and buffering the same data. Since this is a broadcast stream, both listeners receive identical data — the constructor listener buffers it, and the `read()` listener returns it. The next `read()` call then returns the buffered copy, causing data duplication.
Fix: wait for the existing `_bleSubscription` to populate `_bleBuffer` instead of creating a competing listener.</violation>
</file>
<file name="flutter_app/android/app/src/main/res/xml/usb_device_filter.xml">
<violation number="1" location="flutter_app/android/app/src/main/res/xml/usb_device_filter.xml:3">
P2: Overly permissive USB device filter — `<usb-device />` with no attributes matches **every** USB device. This causes Android to prompt the user to open this app for any USB device attachment (keyboards, storage devices, cameras, etc.), not just serial adapters.
Consider restricting to common USB-serial adapter vendor/product IDs (FTDI, CP210x, CH340, PL2303) or at minimum filtering by device class. For example:</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| final completer = Completer<Uint8List?>(); | ||
| late StreamSubscription sub; | ||
| Timer? timer; | ||
| sub = bleTxChar!.onValueReceived.listen((data) { |
There was a problem hiding this comment.
P1: BLE read() creates a second subscription to onValueReceived while _bleSubscription (from the constructor) is still active and buffering the same data. Since this is a broadcast stream, both listeners receive identical data — the constructor listener buffers it, and the read() listener returns it. The next read() call then returns the buffered copy, causing data duplication.
Fix: wait for the existing _bleSubscription to populate _bleBuffer instead of creating a competing listener.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flutter_app/lib/services/capabilities/serial_capability.dart, line 366:
<comment>BLE `read()` creates a second subscription to `onValueReceived` while `_bleSubscription` (from the constructor) is still active and buffering the same data. Since this is a broadcast stream, both listeners receive identical data — the constructor listener buffers it, and the `read()` listener returns it. The next `read()` call then returns the buffered copy, causing data duplication.
Fix: wait for the existing `_bleSubscription` to populate `_bleBuffer` instead of creating a competing listener.</comment>
<file context>
@@ -0,0 +1,389 @@
+ final completer = Completer<Uint8List?>();
+ late StreamSubscription sub;
+ Timer? timer;
+ sub = bleTxChar!.onValueReceived.listen((data) {
+ timer?.cancel();
+ sub.cancel();
</file context>
|
|
||
| void dispose() { | ||
| for (final conn in _connections.values) { | ||
| try { conn.close(); } catch (_) {} |
There was a problem hiding this comment.
P2: conn.close() is async (Future<void>) but is called without await inside a synchronous dispose(). The try/catch won't catch async errors, potentially causing unhandled Future exceptions. At minimum, chain .catchError((_) {}) on the Future, or make dispose() return Future<void>.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flutter_app/lib/services/capabilities/serial_capability.dart, line 296:
<comment>`conn.close()` is async (`Future<void>`) but is called without `await` inside a synchronous `dispose()`. The `try/catch` won't catch async errors, potentially causing unhandled Future exceptions. At minimum, chain `.catchError((_) {})` on the Future, or make `dispose()` return `Future<void>`.</comment>
<file context>
@@ -0,0 +1,389 @@
+
+ void dispose() {
+ for (final conn in _connections.values) {
+ try { conn.close(); } catch (_) {}
+ }
+ _connections.clear();
</file context>
Suggested change
| try { conn.close(); } catch (_) {} | |
| conn.close().catchError((_) {}); |
| @@ -0,0 +1,4 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <resources> | |||
| <usb-device /> | |||
There was a problem hiding this comment.
P2: Overly permissive USB device filter — <usb-device /> with no attributes matches every USB device. This causes Android to prompt the user to open this app for any USB device attachment (keyboards, storage devices, cameras, etc.), not just serial adapters.
Consider restricting to common USB-serial adapter vendor/product IDs (FTDI, CP210x, CH340, PL2303) or at minimum filtering by device class. For example:
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flutter_app/android/app/src/main/res/xml/usb_device_filter.xml, line 3:
<comment>Overly permissive USB device filter — `<usb-device />` with no attributes matches **every** USB device. This causes Android to prompt the user to open this app for any USB device attachment (keyboards, storage devices, cameras, etc.), not just serial adapters.
Consider restricting to common USB-serial adapter vendor/product IDs (FTDI, CP210x, CH340, PL2303) or at minimum filtering by device class. For example:</comment>
<file context>
@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="utf-8"?>
+<resources>
+ <usb-device />
+</resources>
</file context>
Build SuccessfulVersion: Download APKs
Built from 208076b by GitHub Actions |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flutter_app/android/app/src/main/AndroidManifest.xml`:
- Line 35: The manifest currently sets android:allowBackup="true" which will
include app data in backups; create res/xml/backup_rules.xml and
res/xml/data_extraction_rules.xml and reference them from the AndroidManifest
via android:fullBackupContent="@xml/backup_rules" and
android:dataExtractionRules="@xml/data_extraction_rules" (keeping
android:allowBackup as needed) so that sensitive keys are excluded; in
backup_rules.xml exclude SharedPreferences keys "nodeGatewayToken",
"dashboardUrl" (and any token fragments), "nodeDeviceToken" and the file
"openclaw.json", and in data_extraction_rules.xml exclude the same files/keys to
prevent extraction during Android 10+ data migrations. Ensure filenames match
backup_rules.xml and data_extraction_rules.xml and that the manifest uses the
exact attributes fullBackupContent and dataExtractionRules to point to them.
In `@flutter_app/lib/services/capabilities/serial_capability.dart`:
- Around line 126-197: The connect path can leak open ports/devices when an
exception occurs after port.open() or device.connect(); modify the connect logic
in the method that uses UsbSerial.listDevices()/device.create()/port.open() and
BluetoothDevice.fromId()/device.connect() to track the local resources (e.g., a
local UsbPort? port and BluetoothDevice? device and flags like bool portOpened,
deviceConnected) and in the catch block close/cleanup any opened resource (call
port.close() if portOpened and the port was not stored in _connections, and call
device.disconnect() or cancel connection if deviceConnected and not stored)
before returning the error; ensure that when you successfully store a
_SerialConnection in _connections[deviceId] you do not close the resource there.
- Around line 175-185: The code currently accepts BLE connections even when NUS
TX/RX characteristics are missing; update the connect flow in
serial_capability.dart to validate that both txChar and rxChar are non-null
before marking the device connected: if either is null, cancel/close the BLE
connection (use the device disconnect/cancel method), do not add an entry to
_connections or call _SerialConnection.ble, and return a NodeFrame response
indicating failure (e.g. status: 'error' and an explanatory message). Only
create _SerialConnection.ble, call txChar.setNotifyValue(true), and return a
successful 'connected' NodeFrame when both characteristics are present.
- Around line 25-43: The current implementation in serial_capability.dart
exposes Bluetooth permissions globally via requiredPermissions and always
checks/request them in checkPermission and requestPermission, causing USB-only
commands (like serial.list or USB operations) to incorrectly require BLE access;
update the capability so requiredPermissions returns only USB/general
permissions by default and move Bluetooth permission checks into the
BLE-specific operations (e.g., the methods that handle ble:* connect/scan flows
such as the connect/scan handlers or a connectToBleDevice method), modifying
checkPermission/requestPermission to accept a context/flag or creating dedicated
checkBluetoothPermission/requestBluetoothPermission helpers used only by
BLE-scoped code paths; ensure symbol references include requiredPermissions,
checkPermission, requestPermission and any BLE-specific methods you add so BLE
permissions are only enforced for BLE commands.
- Around line 339-375: The read() method creates its own listener on
bleTxChar.onValueReceived while _SerialConnection.ble already sets
_bleSubscription to buffer notifications into _bleBuffer, causing a race;
instead remove the second listener in read() and implement a single-listener
polling mechanism: keep the existing _bleSubscription that pushes bytes into
_bleBuffer, and in read() if _bleBuffer is empty create a Completer<Uint8List?>
field (e.g., _bleReadCompleter) and a Timer for the timeout, then wait on that
completer; modify the single subscription callback (the one referenced by
_bleSubscription / bleTxChar.onValueReceived) to, when data arrives, append to
_bleBuffer and if _bleReadCompleter != null complete it with the buffered data
and clear the buffer and cancel its timer; ensure you cancel/clear
_bleReadCompleter and timer on completion or timeout to avoid leaks and never
call bleTxChar.onValueReceived.listen from read().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc371bae-2597-43fc-a3e1-d54a02d792cc
📒 Files selected for processing (9)
CHANGELOG.mdflutter_app/android/app/src/main/AndroidManifest.xmlflutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/GatewayService.ktflutter_app/android/app/src/main/res/xml/usb_device_filter.xmlflutter_app/lib/providers/node_provider.dartflutter_app/lib/screens/node_screen.dartflutter_app/lib/services/capabilities/serial_capability.dartflutter_app/lib/services/gateway_service.dartflutter_app/pubspec.yaml
| android:requestLegacyExternalStorage="true" | ||
| android:extractNativeLibs="true"> | ||
| android:extractNativeLibs="true" | ||
| android:allowBackup="true"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify sensitive persisted keys/paths that could be included in backup payloads
echo "=== Searching for sensitive keys/paths in flutter_app/lib ==="
rg -n "nodeGatewayToken|dashboardUrl|openclaw\.json|api.?key|token|secret" flutter_app/lib -C2 2>/dev/null || echo "No sensitive keys found"
echo -e "\n=== Checking for backup rule resources ==="
# Verify whether backup rule resources already exist
fd -a "backup_rules.xml|data_extraction_rules.xml" flutter_app/android/app/src/main/res/xml 2>/dev/null || echo "No backup rule resources found"Repository: mithun50/openclaw-termux
Length of output: 28665
Exclude sensitive tokens from backups.
android:allowBackup="true" on Line 35 can expose persisted authentication tokens (nodeGatewayToken, dashboardUrl containing token fragments, nodeDeviceToken) and configuration (openclaw.json) unless backup rules explicitly exclude them. Currently, no backup rule resources exist.
🔐 Suggested manifest hardening
<application
android:label="OpenClaw"
android:name="${applicationName}"
android:icon="@mipmap/ic_launcher"
android:usesCleartextTraffic="true"
android:requestLegacyExternalStorage="true"
android:extractNativeLibs="true"
- android:allowBackup="true">
+ android:allowBackup="true"
+ android:fullBackupContent="@xml/backup_rules"
+ android:dataExtractionRules="@xml/data_extraction_rules">Create flutter_app/android/app/src/main/res/xml/backup_rules.xml and flutter_app/android/app/src/main/res/xml/data_extraction_rules.xml to exclude SharedPreferences keys containing tokens and sensitive configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flutter_app/android/app/src/main/AndroidManifest.xml` at line 35, The
manifest currently sets android:allowBackup="true" which will include app data
in backups; create res/xml/backup_rules.xml and
res/xml/data_extraction_rules.xml and reference them from the AndroidManifest
via android:fullBackupContent="@xml/backup_rules" and
android:dataExtractionRules="@xml/data_extraction_rules" (keeping
android:allowBackup as needed) so that sensitive keys are excluded; in
backup_rules.xml exclude SharedPreferences keys "nodeGatewayToken",
"dashboardUrl" (and any token fragments), "nodeDeviceToken" and the file
"openclaw.json", and in data_extraction_rules.xml exclude the same files/keys to
prevent extraction during Android 10+ data migrations. Ensure filenames match
backup_rules.xml and data_extraction_rules.xml and that the manifest uses the
exact attributes fullBackupContent and dataExtractionRules to point to them.
Comment on lines
+25
to
+43
| List<Permission> get requiredPermissions => [ | ||
| Permission.bluetoothConnect, | ||
| Permission.bluetoothScan, | ||
| ]; | ||
|
|
||
| @override | ||
| Future<bool> checkPermission() async { | ||
| return await Permission.bluetoothConnect.isGranted && | ||
| await Permission.bluetoothScan.isGranted; | ||
| } | ||
|
|
||
| @override | ||
| Future<bool> requestPermission() async { | ||
| final statuses = await [ | ||
| Permission.bluetoothConnect, | ||
| Permission.bluetoothScan, | ||
| ].request(); | ||
| return statuses.values.every((s) => s.isGranted); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the capability handler to see how requiredPermissions is used
cd flutter_app && find . -name "capability_handler.dart" -o -name "node_provider.dart" | head -20Repository: mithun50/openclaw-termux
Length of output: 154
🏁 Script executed:
# Check if the files exist and their locations
git ls-files | grep -E "(capability_handler|serial_capability|node_provider)" | head -20Repository: mithun50/openclaw-termux
Length of output: 235
🏁 Script executed:
# Let's look at the serial_capability.dart file directly
cat -n flutter_app/lib/services/capabilities/serial_capability.dart | head -100Repository: mithun50/openclaw-termux
Length of output: 3769
🏁 Script executed:
# Check how requiredPermissions is used in the base capability class
git ls-files flutter_app | grep -i capability | head -20Repository: mithun50/openclaw-termux
Length of output: 621
🏁 Script executed:
# Check how requiredPermissions is used in capability_handler
cat -n flutter_app/lib/services/capabilities/capability_handler.dartRepository: mithun50/openclaw-termux
Length of output: 1796
🏁 Script executed:
# Check how node_provider invokes capabilities and their permission checking
cat -n flutter_app/lib/providers/node_provider.dart | head -150Repository: mithun50/openclaw-termux
Length of output: 6326
🏁 Script executed:
# Continue viewing node_provider.dart to see how SerialCapability is registered
sed -n '150,180p' flutter_app/lib/providers/node_provider.dartRepository: mithun50/openclaw-termux
Length of output: 1138
🏁 Script executed:
# Search for where _serialCapability.handle is called in node_provider
rg -n "_serialCapability" flutter_app/lib/providers/node_provider.dart -A 2 -B 2Repository: mithun50/openclaw-termux
Length of output: 737
USB commands are gated by BLE permissions.
requiredPermissions returns Bluetooth permissions globally, so serial.list and USB-only operations require Bluetooth permission even when BLE is not used. Permission checks should be scoped to commands that actually require BLE (e.g., connect to ble:* devices) to allow USB-only users to function without Bluetooth permissions.
🛠️ Suggested command-scoped permission handling
- List<Permission> get requiredPermissions => [
- Permission.bluetoothConnect,
- Permission.bluetoothScan,
- ];
+ List<Permission> get requiredPermissions => const [];
+
+ Future<bool> _ensureBlePermissions() async {
+ final statuses = await [
+ Permission.bluetoothConnect,
+ Permission.bluetoothScan,
+ ].request();
+ return statuses.values.every((s) => s.isGranted);
+ } Future<NodeFrame> handle(String command, Map<String, dynamic> params) async {
switch (command) {
case 'serial.list':
return _list();
case 'serial.connect':
+ final deviceId = params['deviceId'] as String?;
+ if (deviceId != null && deviceId.startsWith('ble:')) {
+ final ok = await _ensureBlePermissions();
+ if (!ok) {
+ return NodeFrame.response('', error: {
+ 'code': 'PERMISSION_DENIED',
+ 'message': 'Bluetooth permissions are required for BLE serial',
+ });
+ }
+ }
return _connect(params);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flutter_app/lib/services/capabilities/serial_capability.dart` around lines 25
- 43, The current implementation in serial_capability.dart exposes Bluetooth
permissions globally via requiredPermissions and always checks/request them in
checkPermission and requestPermission, causing USB-only commands (like
serial.list or USB operations) to incorrectly require BLE access; update the
capability so requiredPermissions returns only USB/general permissions by
default and move Bluetooth permission checks into the BLE-specific operations
(e.g., the methods that handle ble:* connect/scan flows such as the connect/scan
handlers or a connectToBleDevice method), modifying
checkPermission/requestPermission to accept a context/flag or creating dedicated
checkBluetoothPermission/requestBluetoothPermission helpers used only by
BLE-scoped code paths; ensure symbol references include requiredPermissions,
checkPermission, requestPermission and any BLE-specific methods you add so BLE
permissions are only enforced for BLE commands.
Comment on lines
+126
to
+197
| try { | ||
| if (deviceId.startsWith('usb:')) { | ||
| final usbId = int.tryParse(deviceId.substring(4)); | ||
| final usbDevices = await UsbSerial.listDevices(); | ||
| final device = usbDevices.firstWhere( | ||
| (d) => d.deviceId == usbId, | ||
| orElse: () => throw Exception('USB device not found'), | ||
| ); | ||
| final port = await device.create(); | ||
| if (port == null) throw Exception('Failed to create USB port'); | ||
| final opened = await port.open(); | ||
| if (!opened) throw Exception('Failed to open USB port'); | ||
|
|
||
| final baudRate = params['baudRate'] as int? ?? 115200; | ||
| await port.setDTR(true); | ||
| await port.setRTS(true); | ||
| await port.setPortParameters( | ||
| baudRate, | ||
| UsbPort.DATABITS_8, | ||
| UsbPort.STOPBITS_1, | ||
| UsbPort.PARITY_NONE, | ||
| ); | ||
|
|
||
| _connections[deviceId] = _SerialConnection.usb(port); | ||
| return NodeFrame.response('', payload: { | ||
| 'status': 'connected', | ||
| 'deviceId': deviceId, | ||
| 'type': 'usb', | ||
| 'baudRate': baudRate, | ||
| }); | ||
| } else if (deviceId.startsWith('ble:')) { | ||
| final remoteId = deviceId.substring(4); | ||
| final device = BluetoothDevice.fromId(remoteId); | ||
| await device.connect(timeout: const Duration(seconds: 10)); | ||
| final services = await device.discoverServices(); | ||
|
|
||
| BluetoothCharacteristic? txChar; | ||
| BluetoothCharacteristic? rxChar; | ||
|
|
||
| for (final service in services) { | ||
| if (service.uuid.toString().toLowerCase() == _nusServiceUuid) { | ||
| for (final c in service.characteristics) { | ||
| final uuid = c.uuid.toString().toLowerCase(); | ||
| if (uuid == _nusTxCharUuid) txChar = c; | ||
| if (uuid == _nusRxCharUuid) rxChar = c; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (txChar != null) { | ||
| await txChar.setNotifyValue(true); | ||
| } | ||
|
|
||
| _connections[deviceId] = _SerialConnection.ble(device, txChar, rxChar); | ||
| return NodeFrame.response('', payload: { | ||
| 'status': 'connected', | ||
| 'deviceId': deviceId, | ||
| 'type': 'ble', | ||
| 'hasNus': txChar != null && rxChar != null, | ||
| }); | ||
| } | ||
|
|
||
| return NodeFrame.response('', error: { | ||
| 'code': 'INVALID_DEVICE_ID', | ||
| 'message': 'deviceId must start with usb: or ble:', | ||
| }); | ||
| } catch (e) { | ||
| return NodeFrame.response('', error: { | ||
| 'code': 'CONNECT_ERROR', | ||
| 'message': '$e', | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the serial_capability.dart file
find . -name "serial_capability.dart" -type fRepository: mithun50/openclaw-termux
Length of output: 130
🏁 Script executed:
# Get the file size to understand how much we need to read
wc -l flutter_app/lib/services/capabilities/serial_capability.dartRepository: mithun50/openclaw-termux
Length of output: 132
🏁 Script executed:
# Read the relevant section with more context around lines 126-197
head -250 flutter_app/lib/services/capabilities/serial_capability.dart | tail -150Repository: mithun50/openclaw-termux
Length of output: 4677
🏁 Script executed:
# Read the entire file to find _SerialConnection class definition
cat -n flutter_app/lib/services/capabilities/serial_capability.dart | grep -A 50 "class _SerialConnection"Repository: mithun50/openclaw-termux
Length of output: 2005
🏁 Script executed:
# Also check for imports to understand what libraries are being used
head -50 flutter_app/lib/services/capabilities/serial_capability.dartRepository: mithun50/openclaw-termux
Length of output: 1592
🏁 Script executed:
# Find the close() method in _SerialConnection class
cat -n flutter_app/lib/services/capabilities/serial_capability.dart | grep -A 15 "Future<void> close"Repository: mithun50/openclaw-termux
Length of output: 341
🏁 Script executed:
# Also check what happens if there's any exception handling in the constructor or around port.open()/device.connect()
sed -n '302,389p' flutter_app/lib/services/capabilities/serial_capability.dartRepository: mithun50/openclaw-termux
Length of output: 2581
Clean up partially-open transports on connect failure.
If exceptions occur after port.open() (USB) or device.connect() (BLE), the resources remain open because the _SerialConnection object is never created and stored in _connections. The catch block returns an error without closing the port or disconnecting the device, leaving them leaked.
Track the opened/connected resources and close them in the catch block before returning the error:
🧹 Suggested cleanup on failed connect attempts
- try {
+ UsbPort? openedPort;
+ BluetoothDevice? connectedBle;
+ try {
if (deviceId.startsWith('usb:')) {
@@
final opened = await port.open();
if (!opened) throw Exception('Failed to open USB port');
+ openedPort = port;
@@
} else if (deviceId.startsWith('ble:')) {
@@
final device = BluetoothDevice.fromId(remoteId);
await device.connect(timeout: const Duration(seconds: 10));
+ connectedBle = device;
@@
} catch (e) {
+ try { await openedPort?.close(); } catch (_) {}
+ try { if (connectedBle != null) await connectedBle!.disconnect(); } catch (_) {}
return NodeFrame.response('', error: {
'code': 'CONNECT_ERROR',
'message': '$e',
});
}📝 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
| try { | |
| if (deviceId.startsWith('usb:')) { | |
| final usbId = int.tryParse(deviceId.substring(4)); | |
| final usbDevices = await UsbSerial.listDevices(); | |
| final device = usbDevices.firstWhere( | |
| (d) => d.deviceId == usbId, | |
| orElse: () => throw Exception('USB device not found'), | |
| ); | |
| final port = await device.create(); | |
| if (port == null) throw Exception('Failed to create USB port'); | |
| final opened = await port.open(); | |
| if (!opened) throw Exception('Failed to open USB port'); | |
| final baudRate = params['baudRate'] as int? ?? 115200; | |
| await port.setDTR(true); | |
| await port.setRTS(true); | |
| await port.setPortParameters( | |
| baudRate, | |
| UsbPort.DATABITS_8, | |
| UsbPort.STOPBITS_1, | |
| UsbPort.PARITY_NONE, | |
| ); | |
| _connections[deviceId] = _SerialConnection.usb(port); | |
| return NodeFrame.response('', payload: { | |
| 'status': 'connected', | |
| 'deviceId': deviceId, | |
| 'type': 'usb', | |
| 'baudRate': baudRate, | |
| }); | |
| } else if (deviceId.startsWith('ble:')) { | |
| final remoteId = deviceId.substring(4); | |
| final device = BluetoothDevice.fromId(remoteId); | |
| await device.connect(timeout: const Duration(seconds: 10)); | |
| final services = await device.discoverServices(); | |
| BluetoothCharacteristic? txChar; | |
| BluetoothCharacteristic? rxChar; | |
| for (final service in services) { | |
| if (service.uuid.toString().toLowerCase() == _nusServiceUuid) { | |
| for (final c in service.characteristics) { | |
| final uuid = c.uuid.toString().toLowerCase(); | |
| if (uuid == _nusTxCharUuid) txChar = c; | |
| if (uuid == _nusRxCharUuid) rxChar = c; | |
| } | |
| } | |
| } | |
| if (txChar != null) { | |
| await txChar.setNotifyValue(true); | |
| } | |
| _connections[deviceId] = _SerialConnection.ble(device, txChar, rxChar); | |
| return NodeFrame.response('', payload: { | |
| 'status': 'connected', | |
| 'deviceId': deviceId, | |
| 'type': 'ble', | |
| 'hasNus': txChar != null && rxChar != null, | |
| }); | |
| } | |
| return NodeFrame.response('', error: { | |
| 'code': 'INVALID_DEVICE_ID', | |
| 'message': 'deviceId must start with usb: or ble:', | |
| }); | |
| } catch (e) { | |
| return NodeFrame.response('', error: { | |
| 'code': 'CONNECT_ERROR', | |
| 'message': '$e', | |
| }); | |
| } | |
| UsbPort? openedPort; | |
| BluetoothDevice? connectedBle; | |
| try { | |
| if (deviceId.startsWith('usb:')) { | |
| final usbId = int.tryParse(deviceId.substring(4)); | |
| final usbDevices = await UsbSerial.listDevices(); | |
| final device = usbDevices.firstWhere( | |
| (d) => d.deviceId == usbId, | |
| orElse: () => throw Exception('USB device not found'), | |
| ); | |
| final port = await device.create(); | |
| if (port == null) throw Exception('Failed to create USB port'); | |
| final opened = await port.open(); | |
| if (!opened) throw Exception('Failed to open USB port'); | |
| openedPort = port; | |
| final baudRate = params['baudRate'] as int? ?? 115200; | |
| await port.setDTR(true); | |
| await port.setRTS(true); | |
| await port.setPortParameters( | |
| baudRate, | |
| UsbPort.DATABITS_8, | |
| UsbPort.STOPBITS_1, | |
| UsbPort.PARITY_NONE, | |
| ); | |
| _connections[deviceId] = _SerialConnection.usb(port); | |
| return NodeFrame.response('', payload: { | |
| 'status': 'connected', | |
| 'deviceId': deviceId, | |
| 'type': 'usb', | |
| 'baudRate': baudRate, | |
| }); | |
| } else if (deviceId.startsWith('ble:')) { | |
| final remoteId = deviceId.substring(4); | |
| final device = BluetoothDevice.fromId(remoteId); | |
| await device.connect(timeout: const Duration(seconds: 10)); | |
| connectedBle = device; | |
| final services = await device.discoverServices(); | |
| BluetoothCharacteristic? txChar; | |
| BluetoothCharacteristic? rxChar; | |
| for (final service in services) { | |
| if (service.uuid.toString().toLowerCase() == _nusServiceUuid) { | |
| for (final c in service.characteristics) { | |
| final uuid = c.uuid.toString().toLowerCase(); | |
| if (uuid == _nusTxCharUuid) txChar = c; | |
| if (uuid == _nusRxCharUuid) rxChar = c; | |
| } | |
| } | |
| } | |
| if (txChar != null) { | |
| await txChar.setNotifyValue(true); | |
| } | |
| _connections[deviceId] = _SerialConnection.ble(device, txChar, rxChar); | |
| return NodeFrame.response('', payload: { | |
| 'status': 'connected', | |
| 'deviceId': deviceId, | |
| 'type': 'ble', | |
| 'hasNus': txChar != null && rxChar != null, | |
| }); | |
| } | |
| return NodeFrame.response('', error: { | |
| 'code': 'INVALID_DEVICE_ID', | |
| 'message': 'deviceId must start with usb: or ble:', | |
| }); | |
| } catch (e) { | |
| try { await openedPort?.close(); } catch (_) {} | |
| try { if (connectedBle != null) await connectedBle!.disconnect(); } catch (_) {} | |
| return NodeFrame.response('', error: { | |
| 'code': 'CONNECT_ERROR', | |
| 'message': '$e', | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flutter_app/lib/services/capabilities/serial_capability.dart` around lines
126 - 197, The connect path can leak open ports/devices when an exception occurs
after port.open() or device.connect(); modify the connect logic in the method
that uses UsbSerial.listDevices()/device.create()/port.open() and
BluetoothDevice.fromId()/device.connect() to track the local resources (e.g., a
local UsbPort? port and BluetoothDevice? device and flags like bool portOpened,
deviceConnected) and in the catch block close/cleanup any opened resource (call
port.close() if portOpened and the port was not stored in _connections, and call
device.disconnect() or cancel connection if deviceConnected and not stored)
before returning the error; ensure that when you successfully store a
_SerialConnection in _connections[deviceId] you do not close the resource there.
Comment on lines
+175
to
+185
| if (txChar != null) { | ||
| await txChar.setNotifyValue(true); | ||
| } | ||
|
|
||
| _connections[deviceId] = _SerialConnection.ble(device, txChar, rxChar); | ||
| return NodeFrame.response('', payload: { | ||
| 'status': 'connected', | ||
| 'deviceId': deviceId, | ||
| 'type': 'ble', | ||
| 'hasNus': txChar != null && rxChar != null, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the serial_capability.dart file
find . -name "serial_capability.dart" -type fRepository: mithun50/openclaw-termux
Length of output: 130
🏁 Script executed:
# Get the file size to decide how to read it
wc -l flutter_app/lib/services/capabilities/serial_capability.dartRepository: mithun50/openclaw-termux
Length of output: 132
🏁 Script executed:
# Read the relevant section around lines 175-185 with context
sed -n '150,210p' flutter_app/lib/services/capabilities/serial_capability.dart | cat -nRepository: mithun50/openclaw-termux
Length of output: 2449
🏁 Script executed:
# Search for _SerialConnection class definition
rg -n "class _SerialConnection" flutter_app/lib/services/capabilities/serial_capability.dartRepository: mithun50/openclaw-termux
Length of output: 97
🏁 Script executed:
# Look for the ble() constructor or factory method
rg -n "\.ble\(" flutter_app/lib/services/capabilities/serial_capability.dart -A 3Repository: mithun50/openclaw-termux
Length of output: 490
🏁 Script executed:
# Search for read/write operations on characteristics to understand dependencies
rg -n "(write|read|characteristic)" flutter_app/lib/services/capabilities/serial_capability.dart -iRepository: mithun50/openclaw-termux
Length of output: 1222
🏁 Script executed:
# Read the full write() and read() method implementations
sed -n '324,350p' flutter_app/lib/services/capabilities/serial_capability.dart | cat -nRepository: mithun50/openclaw-termux
Length of output: 1163
🏁 Script executed:
# Continue reading the read() method
sed -n '339,380p' flutter_app/lib/services/capabilities/serial_capability.dart | cat -nRepository: mithun50/openclaw-termux
Length of output: 1602
Reject connections when NUS characteristics are not found.
The current implementation accepts the connection even when TX/RX characteristics are missing, returning 'hasNus': false while 'status': 'connected'. While the write() and read() methods have defensive null checks and won't crash, this creates poor UX by delaying the failure until the client attempts an operation. Either validate and fail at connection time if NUS is unavailable, or redesign the client protocol to explicitly handle partial connections.
Suggested validation before successful connect response
- if (txChar != null) {
- await txChar.setNotifyValue(true);
- }
-
- _connections[deviceId] = _SerialConnection.ble(device, txChar, rxChar);
+ if (txChar == null || rxChar == null) {
+ await device.disconnect();
+ return NodeFrame.response('', error: {
+ 'code': 'NUS_NOT_FOUND',
+ 'message': 'Nordic UART Service characteristics not found',
+ });
+ }
+ await txChar.setNotifyValue(true);
+ _connections[deviceId] = _SerialConnection.ble(device, txChar, rxChar);
return NodeFrame.response('', payload: {
'status': 'connected',
'deviceId': deviceId,
'type': 'ble',
- 'hasNus': txChar != null && rxChar != null,
+ 'hasNus': true,
});📝 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
| if (txChar != null) { | |
| await txChar.setNotifyValue(true); | |
| } | |
| _connections[deviceId] = _SerialConnection.ble(device, txChar, rxChar); | |
| return NodeFrame.response('', payload: { | |
| 'status': 'connected', | |
| 'deviceId': deviceId, | |
| 'type': 'ble', | |
| 'hasNus': txChar != null && rxChar != null, | |
| }); | |
| if (txChar == null || rxChar == null) { | |
| await device.disconnect(); | |
| return NodeFrame.response('', error: { | |
| 'code': 'NUS_NOT_FOUND', | |
| 'message': 'Nordic UART Service characteristics not found', | |
| }); | |
| } | |
| await txChar.setNotifyValue(true); | |
| _connections[deviceId] = _SerialConnection.ble(device, txChar, rxChar); | |
| return NodeFrame.response('', payload: { | |
| 'status': 'connected', | |
| 'deviceId': deviceId, | |
| 'type': 'ble', | |
| 'hasNus': true, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flutter_app/lib/services/capabilities/serial_capability.dart` around lines
175 - 185, The code currently accepts BLE connections even when NUS TX/RX
characteristics are missing; update the connect flow in serial_capability.dart
to validate that both txChar and rxChar are non-null before marking the device
connected: if either is null, cancel/close the BLE connection (use the device
disconnect/cancel method), do not add an entry to _connections or call
_SerialConnection.ble, and return a NodeFrame response indicating failure (e.g.
status: 'error' and an explanatory message). Only create _SerialConnection.ble,
call txChar.setNotifyValue(true), and return a successful 'connected' NodeFrame
when both characteristics are present.
Comment on lines
+339
to
+375
| Future<Uint8List?> read(Duration timeout) async { | ||
| if (usbPort != null) { | ||
| // Read from USB input stream with timeout | ||
| final completer = Completer<Uint8List?>(); | ||
| StreamSubscription? sub; | ||
| Timer? timer; | ||
| sub = usbPort!.inputStream?.listen((data) { | ||
| timer?.cancel(); | ||
| sub?.cancel(); | ||
| completer.complete(Uint8List.fromList(data)); | ||
| }); | ||
| timer = Timer(timeout, () { | ||
| sub?.cancel(); | ||
| completer.complete(null); | ||
| }); | ||
| return completer.future; | ||
| } else if (bleTxChar != null) { | ||
| // Return buffered BLE data or wait | ||
| if (_bleBuffer.isNotEmpty) { | ||
| final data = Uint8List.fromList(_bleBuffer); | ||
| _bleBuffer.clear(); | ||
| return data; | ||
| } | ||
| // Wait for data with timeout | ||
| final completer = Completer<Uint8List?>(); | ||
| late StreamSubscription sub; | ||
| Timer? timer; | ||
| sub = bleTxChar!.onValueReceived.listen((data) { | ||
| timer?.cancel(); | ||
| sub.cancel(); | ||
| completer.complete(Uint8List.fromList(data)); | ||
| }); | ||
| timer = Timer(timeout, () { | ||
| sub.cancel(); | ||
| completer.complete(null); | ||
| }); | ||
| return completer.future; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the serial_capability.dart file
fd -t f "serial_capability.dart" --exec wc -l {} \;Repository: mithun50/openclaw-termux
Length of output: 134
🏁 Script executed:
# Let's read the full file to understand the implementation
cat -n flutter_app/lib/services/capabilities/serial_capability.dart | head -100Repository: mithun50/openclaw-termux
Length of output: 3769
🏁 Script executed:
# Search for _bleSubscription and _bleBuffer to understand the buffering mechanism
rg "_bleSubscription|_bleBuffer" flutter_app/lib/services/capabilities/serial_capability.dart -A 2 -B 2Repository: mithun50/openclaw-termux
Length of output: 793
🏁 Script executed:
# Search for class definition and initialization
rg "class.*Serial|_bleSubscription\s*=|_bleBuffer\s*=" flutter_app/lib/services/capabilities/serial_capability.dart -nRepository: mithun50/openclaw-termux
Length of output: 262
🏁 Script executed:
# Get lines 300-380 to see the full _SerialConnection class and read method
sed -n '300,380p' flutter_app/lib/services/capabilities/serial_capability.dart | cat -nRepository: mithun50/openclaw-termux
Length of output: 2978
🏁 Script executed:
# Search for any error handling or comments about the stream listeners
rg "broadcast|onValueReceived" flutter_app/lib/services/capabilities/serial_capability.dart -nRepository: mithun50/openclaw-termux
Length of output: 198
🏁 Script executed:
# Check if there's any cleanup of the second listener or handling of multiple subscriptions
sed -n '339,389p' flutter_app/lib/services/capabilities/serial_capability.dart | cat -nRepository: mithun50/openclaw-termux
Length of output: 1838
🏁 Script executed:
# Check flutter_blue_plus or any local documentation about onValueReceived
rg "onValueReceived" flutter_app -A 1 -B 1 | head -30Repository: mithun50/openclaw-termux
Length of output: 666
🌐 Web query:
flutter_blue_plus BluetoothCharacteristic onValueReceived broadcast stream multiple listeners
💡 Result:
BluetoothCharacteristic.onValueReceived is a Stream<List<int>> of value updates (typically from read() and notifications via setNotifyValue(true)). In practice you’ll hit either of these situations:
-
You added multiple
.listen()calls (often on rebuild), so you see duplicate events
The package docs explicitly call this out: duplicate data usually means you “forgot to cancel the originalchr.onValueReceived.listen”, and they recommenddevice.cancelWhenDisconnected(subscription)for cleanup. [1] -
You truly want multiple listeners (fan-out to multiple widgets/services)
A Dart stream must be broadcast to support multiple simultaneous listeners. If you get “Bad state: Stream has already been listened to”, wrap it once withasBroadcastStream()and reuse that shared stream. [2]
Pattern that supports multiple listeners safely
late final Stream<List<int>> value$; // shared broadcast stream
StreamSubscription<List<int>>? sub;
Future<void> setup(BluetoothDevice device, BluetoothCharacteristic chr) async {
// Create ONE shared broadcast stream instance
value$ = chr.onValueReceived.asBroadcastStream(); // [2]
// (Optional) keep one “primary” subscription if you need side effects
sub = value$.listen((v) { /* ... */ });
// Important: clean up to avoid duplicates/leaks
device.cancelWhenDisconnected(sub!); // [1]
await chr.setNotifyValue(true);
}If you don’t need a “primary” listener, you can skip sub = ... and just let multiple parts of the app listen to value$.
Key rule: don’t call chr.onValueReceived.listen(...) in multiple places; instead, create one shared (broadcast) stream and pass it around (or expose it from a service). [1][2]
Sources: FlutterBluePlus docs on onValueReceived and duplicate-data cause/cleanup [1]; Dart Stream.asBroadcastStream() API [2].
Resolve dual listener race condition on BLE notifications.
_SerialConnection.ble establishes _bleSubscription to buffer notifications via bleTxChar.onValueReceived, but read() creates a second independent listener on the same stream when the buffer is empty. Both listeners are active simultaneously, causing notifications to race between the two paths—data may be consumed by either listener, leading to unpredictable buffering and potential packet loss. This is a documented anti-pattern in flutter_blue_plus. Use a single polling mechanism instead.
📦 Suggested single-listener approach
- } else if (bleTxChar != null) {
- // Return buffered BLE data or wait
- if (_bleBuffer.isNotEmpty) {
- final data = Uint8List.fromList(_bleBuffer);
- _bleBuffer.clear();
- return data;
- }
- // Wait for data with timeout
- final completer = Completer<Uint8List?>();
- late StreamSubscription sub;
- Timer? timer;
- sub = bleTxChar!.onValueReceived.listen((data) {
- timer?.cancel();
- sub.cancel();
- completer.complete(Uint8List.fromList(data));
- });
- timer = Timer(timeout, () {
- sub.cancel();
- completer.complete(null);
- });
- return completer.future;
+ } else if (bleTxChar != null) {
+ final deadline = DateTime.now().add(timeout);
+ while (_bleBuffer.isEmpty && DateTime.now().isBefore(deadline)) {
+ await Future.delayed(const Duration(milliseconds: 20));
+ }
+ if (_bleBuffer.isEmpty) return null;
+ final data = Uint8List.fromList(_bleBuffer);
+ _bleBuffer.clear();
+ return 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
| Future<Uint8List?> read(Duration timeout) async { | |
| if (usbPort != null) { | |
| // Read from USB input stream with timeout | |
| final completer = Completer<Uint8List?>(); | |
| StreamSubscription? sub; | |
| Timer? timer; | |
| sub = usbPort!.inputStream?.listen((data) { | |
| timer?.cancel(); | |
| sub?.cancel(); | |
| completer.complete(Uint8List.fromList(data)); | |
| }); | |
| timer = Timer(timeout, () { | |
| sub?.cancel(); | |
| completer.complete(null); | |
| }); | |
| return completer.future; | |
| } else if (bleTxChar != null) { | |
| // Return buffered BLE data or wait | |
| if (_bleBuffer.isNotEmpty) { | |
| final data = Uint8List.fromList(_bleBuffer); | |
| _bleBuffer.clear(); | |
| return data; | |
| } | |
| // Wait for data with timeout | |
| final completer = Completer<Uint8List?>(); | |
| late StreamSubscription sub; | |
| Timer? timer; | |
| sub = bleTxChar!.onValueReceived.listen((data) { | |
| timer?.cancel(); | |
| sub.cancel(); | |
| completer.complete(Uint8List.fromList(data)); | |
| }); | |
| timer = Timer(timeout, () { | |
| sub.cancel(); | |
| completer.complete(null); | |
| }); | |
| return completer.future; | |
| Future<Uint8List?> read(Duration timeout) async { | |
| if (usbPort != null) { | |
| // Read from USB input stream with timeout | |
| final completer = Completer<Uint8List?>(); | |
| StreamSubscription? sub; | |
| Timer? timer; | |
| sub = usbPort!.inputStream?.listen((data) { | |
| timer?.cancel(); | |
| sub?.cancel(); | |
| completer.complete(Uint8List.fromList(data)); | |
| }); | |
| timer = Timer(timeout, () { | |
| sub?.cancel(); | |
| completer.complete(null); | |
| }); | |
| return completer.future; | |
| } else if (bleTxChar != null) { | |
| final deadline = DateTime.now().add(timeout); | |
| while (_bleBuffer.isEmpty && DateTime.now().isBefore(deadline)) { | |
| await Future.delayed(const Duration(milliseconds: 20)); | |
| } | |
| if (_bleBuffer.isEmpty) return null; | |
| final data = Uint8List.fromList(_bleBuffer); | |
| _bleBuffer.clear(); | |
| return data; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flutter_app/lib/services/capabilities/serial_capability.dart` around lines
339 - 375, The read() method creates its own listener on
bleTxChar.onValueReceived while _SerialConnection.ble already sets
_bleSubscription to buffer notifications into _bleBuffer, causing a race;
instead remove the second listener in read() and implement a single-listener
polling mechanism: keep the existing _bleSubscription that pushes bytes into
_bleBuffer, and in read() if _bleBuffer is empty create a Completer<Uint8List?>
field (e.g., _bleReadCompleter) and a Timer for the timeout, then wait on that
completer; modify the single subscription callback (the one referenced by
_bleSubscription / bleTxChar.onValueReceived) to, when data arrives, append to
_bleBuffer and if _bleReadCompleter != null complete it with the buffered data
and clear the buffer and cancel its timer; ensure you cancel/clear
_bleReadCompleter and timer on completion or timeout to avoid leaks and never
call bleTxChar.onValueReceived.listen from read().
- Add direct file I/O fallback for writing allowCommands config when proot/node isn't ready (the silent failure was the root cause) - Send both 'capabilities' and 'commands' fields in node.capabilities event to match the connect frame format - Gateway now reliably picks up allowCommands even if the Node.js config writer fails Closes #56
mithun50
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flutter_app/lib/services/gateway_service.dart`:
- Around line 143-167: The fallback write silently fails because direct casts of
config['gateway'] and gw['nodes'] (variables gw and nodes) will throw if those
keys are null or not maps; replace those unsafe casts with defensive checks: if
config['gateway'] is not a Map<String,dynamic> (or is null) set
config['gateway'] = <String,dynamic>{} before reading gw, then read gw as a Map
after an explicit type check, and likewise ensure gw['nodes'] is a
Map<String,dynamic> (or replace it with an empty map) before using nodes and
assigning nodes['allowCommands'] = allowCommands; keep the surrounding try/catch
but make these type-guard assignments so the writeAsStringSync call always has a
valid JSON object to persist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7b6a7df-0c96-40e6-9cb5-ab01b0d26a3b
📒 Files selected for processing (3)
CHANGELOG.mdflutter_app/lib/services/gateway_service.dartflutter_app/lib/services/node_service.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Comment on lines
+143
to
+167
| // Direct file I/O fallback (#56): if proot/node isn't ready, write the | ||
| // config directly on the Android filesystem so the gateway still picks | ||
| // up allowCommands on next start. | ||
| if (!prootOk) { | ||
| try { | ||
| final filesDir = await NativeBridge.getFilesDir(); | ||
| final configFile = File('$filesDir/rootfs/ubuntu/root/.openclaw/openclaw.json'); | ||
| Map<String, dynamic> config = {}; | ||
| if (configFile.existsSync()) { | ||
| try { | ||
| config = Map<String, dynamic>.from( | ||
| jsonDecode(configFile.readAsStringSync()) as Map); | ||
| } catch (_) {} | ||
| } | ||
| config.putIfAbsent('gateway', () => <String, dynamic>{}); | ||
| final gw = config['gateway'] as Map<String, dynamic>; | ||
| gw.putIfAbsent('nodes', () => <String, dynamic>{}); | ||
| final nodes = gw['nodes'] as Map<String, dynamic>; | ||
| nodes['denyCommands'] = <String>[]; | ||
| nodes['allowCommands'] = allowCommands; | ||
| configFile.parent.createSync(recursive: true); | ||
| configFile.writeAsStringSync( | ||
| const JsonEncoder.withIndent(' ').convert(config), | ||
| ); | ||
| } catch (_) {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'openclaw\.json|gateway|nodes|allowCommands|denyCommands|jsonDecode|writeAsString'Repository: mithun50/openclaw-termux
Length of output: 50382
🏁 Script executed:
cat -n flutter_app/lib/services/gateway_service.dart | head -200 | tail -100Repository: mithun50/openclaw-termux
Length of output: 4649
🏁 Script executed:
#!/bin/bash
# Look for where openclaw.json is written besides the fallback path
rg -n 'openclaw\.json|writeAsStringSync.*config|jsonEncode.*gateway|jsonEncode.*nodes' flutter_app/lib/services/ flutter_app/lib/screens/Repository: mithun50/openclaw-termux
Length of output: 1161
🏁 Script executed:
cat -n flutter_app/lib/services/provider_config_service.dartRepository: mithun50/openclaw-termux
Length of output: 7218
🏁 Script executed:
#!/bin/bash
# Check how gateway/nodes could be non-map in the config file
# Look at the Node.js fallback script in provider_config_service to see safe pattern
echo "=== Node.js safe pattern in provider_config_service ==="
sed -n '71,85p' flutter_app/lib/services/provider_config_service.dart
echo ""
echo "=== Dart safe pattern in _saveConfigDirect ==="
sed -n '119,131p' flutter_app/lib/services/provider_config_service.dart
echo ""
echo "=== Vulnerable pattern in gateway_service.dart ==="
sed -n '157,162p' flutter_app/lib/services/gateway_service.dartRepository: mithun50/openclaw-termux
Length of output: 1876
Fallback config write can silently fail on unexpected JSON shape.
In the fallback path, direct casts of gateway/nodes (lines 158, 160) throw if those keys exist but hold null or non-map values; the broad catch then suppresses the exception, so allowCommands may never be written. This undermines the fallback's reliability goal.
For comparison, the provider config code in provider_config_service.dart uses safer patterns: Node.js checks if (!c.gateway) before access (line 76), and Dart uses ??= with explicit casts (lines 120–121). The gateway fallback should follow the same defensive approach.
💡 Proposed hardening
- config.putIfAbsent('gateway', () => <String, dynamic>{});
- final gw = config['gateway'] as Map<String, dynamic>;
- gw.putIfAbsent('nodes', () => <String, dynamic>{});
- final nodes = gw['nodes'] as Map<String, dynamic>;
+ final gw = config['gateway'] is Map
+ ? Map<String, dynamic>.from(config['gateway'] as Map)
+ : <String, dynamic>{};
+ config['gateway'] = gw;
+
+ final nodes = gw['nodes'] is Map
+ ? Map<String, dynamic>.from(gw['nodes'] as Map)
+ : <String, dynamic>{};
+ gw['nodes'] = nodes;
nodes['denyCommands'] = <String>[];
nodes['allowCommands'] = allowCommands;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flutter_app/lib/services/gateway_service.dart` around lines 143 - 167, The
fallback write silently fails because direct casts of config['gateway'] and
gw['nodes'] (variables gw and nodes) will throw if those keys are null or not
maps; replace those unsafe casts with defensive checks: if config['gateway'] is
not a Map<String,dynamic> (or is null) set config['gateway'] =
<String,dynamic>{} before reading gw, then read gw as a Map after an explicit
type check, and likewise ensure gw['nodes'] is a Map<String,dynamic> (or replace
it with an empty map) before using nodes and assigning nodes['allowCommands'] =
allowCommands; keep the surrounding try/catch but make these type-guard
assignments so the writeAsStringSync call always has a valid JSON object to
persist.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="flutter_app/lib/services/gateway_service.dart">
<violation number="1" location="flutter_app/lib/services/gateway_service.dart:141">
P2: Silent error swallowing: if both the proot approach and the direct file I/O fallback fail, there is no log output or diagnostic information at all. Consider logging a warning so failures in this fallback path are observable during debugging.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } catch (_) { | ||
| // Non-fatal: gateway may still work with default policy | ||
| prootOk = true; | ||
| } catch (_) {} |
There was a problem hiding this comment.
P2: Silent error swallowing: if both the proot approach and the direct file I/O fallback fail, there is no log output or diagnostic information at all. Consider logging a warning so failures in this fallback path are observable during debugging.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flutter_app/lib/services/gateway_service.dart, line 141:
<comment>Silent error swallowing: if both the proot approach and the direct file I/O fallback fail, there is no log output or diagnostic information at all. Consider logging a warning so failures in this fallback path are observable during debugging.</comment>
<file context>
@@ -131,13 +131,40 @@ c.gateway.nodes.denyCommands = [];
- } catch (_) {
- // Non-fatal: gateway may still work with default policy
+ prootOk = true;
+ } catch (_) {}
+
+ // Direct file I/O fallback (#56): if proot/node isn't ready, write the
</file context>
Suggested change
| } catch (_) {} | |
| } catch (e) { | |
| // Non-fatal: gateway may still work with default policy | |
| _updateState(_state.copyWith( | |
| logs: [..._state.logs, _ts('[WARN] Failed to write allowCommands config: $e')], | |
| )); | |
| } |
This was referenced Mar 5, 2026
Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
Closed
Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flutter_app/lib/screens/settings_screen.dart`:
- Around line 451-456: The onPressed callback currently calls Navigator.pop(ctx)
and launchUrl(...) without checking the Future<bool> result; update the
anonymous onPressed in SettingsScreen to be async, await the bool returned by
launchUrl(Uri.parse(result.url), mode: LaunchMode.externalApplication), and if
it returns false show a SnackBar with an error message (ensure you check mounted
before showing the SnackBar); keep the existing Navigator.pop(ctx) call and only
perform the launch/feedback after popping.
In `@flutter_app/lib/services/update_service.dart`:
- Around line 19-22: The GitHub API call using http.get against
AppConstants.githubApiLatestRelease is unbounded; wrap the http.get request (the
expression that assigns response) with a timeout (e.g.,
.timeout(Duration(seconds: X)) ) and handle TimeoutException (or generic
Exception) so the update check fails fast and is handled cleanly (log/return)
instead of hanging; update the same call site where response is assigned and
ensure headers remain unchanged.
- Around line 41-42: The current parsing of remote and local version strings
(variables remote and local, producing lists r and l) crashes on
prerelease/build metadata; update the logic to first normalize the tag by
stripping a leading "v/V" and extracting the core semver part before any '-' or
'+' (e.g., take remoteCore = remote.trim().replaceFirst(RegExp(r'^[vV]'),
'').split(RegExp(r'[-+]'))[0]) and then split on '.' and int.parse each numeric
identifier to produce r and l, handling missing patch/minor by treating absent
segments as 0; apply the same normalization to local so the semver comparison
code using r and l no longer throws FormatException.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd77f49b-6bb7-407d-b1e3-6ce3a9be98bb
📒 Files selected for processing (4)
CHANGELOG.mdflutter_app/lib/constants.dartflutter_app/lib/screens/settings_screen.dartflutter_app/lib/services/update_service.dart
Comment on lines
+451
to
+456
| onPressed: () { | ||
| Navigator.pop(ctx); | ||
| launchUrl( | ||
| Uri.parse(result.url), | ||
| mode: LaunchMode.externalApplication, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
url_launcher package 6.2.0 launchUrl return value documentation
💡 Result:
In url_launcher 6.2.0, launchUrl returns a Future<bool>:
trueif the URL was launched successfully.- Otherwise it returns
falseor throws aPlatformException(depending on the failure). [1]
Platform note (relevant to interpreting the boolean):
- On Flutter Web,
launchUrlalways returnstruefor allowed schemes, because the browser doesn’t provide a way to know whether the new tab/window actually opened when usingnoopener. [2]
For 6.2.0 specifically, the package changelog also notes that the launchUrl documentation was updated around that release (and that 6.2.0 was later retracted due to incorrect types in new APIs). [3]
Sources: [1] [2] [3]
🏁 Script executed:
# First, verify the file and code exist at the specified location
if [ -f "flutter_app/lib/screens/settings_screen.dart" ]; then
echo "File found. Reading lines 451-456:"
sed -n '451,456p' flutter_app/lib/screens/settings_screen.dart
echo ""
echo "Context around lines 451-456 (440-465):"
sed -n '440,465p' flutter_app/lib/screens/settings_screen.dart
else
echo "File not found at flutter_app/lib/screens/settings_screen.dart"
fiRepository: mithun50/openclaw-termux
Length of output: 1221
Handle launchUrl failure explicitly.
The download action currently ignores whether URL launch succeeds, so failures are silent. The launchUrl() function in url_launcher 6.2.0 returns Future<bool> where false indicates launch failure, and this return value should be checked to provide user feedback.
The proposed fix is correct: make the callback async, await the result, and show a SnackBar if the launch fails. The use of the mounted check is also appropriate for async callbacks in stateful widgets.
Proposed fix
FilledButton(
- onPressed: () {
+ onPressed: () async {
Navigator.pop(ctx);
- launchUrl(
+ final opened = await launchUrl(
Uri.parse(result.url),
mode: LaunchMode.externalApplication,
);
+ if (!opened && mounted) {
+ ScaffoldMessenger.of(context).showSnackBar(
+ const SnackBar(content: Text('Could not open release page')),
+ );
+ }
},
child: const Text('Download'),
),📝 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
| onPressed: () { | |
| Navigator.pop(ctx); | |
| launchUrl( | |
| Uri.parse(result.url), | |
| mode: LaunchMode.externalApplication, | |
| ); | |
| onPressed: () async { | |
| Navigator.pop(ctx); | |
| final opened = await launchUrl( | |
| Uri.parse(result.url), | |
| mode: LaunchMode.externalApplication, | |
| ); | |
| if (!opened && mounted) { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| const SnackBar(content: Text('Could not open release page')), | |
| ); | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flutter_app/lib/screens/settings_screen.dart` around lines 451 - 456, The
onPressed callback currently calls Navigator.pop(ctx) and launchUrl(...) without
checking the Future<bool> result; update the anonymous onPressed in
SettingsScreen to be async, await the bool returned by
launchUrl(Uri.parse(result.url), mode: LaunchMode.externalApplication), and if
it returns false show a SnackBar with an error message (ensure you check mounted
before showing the SnackBar); keep the existing Navigator.pop(ctx) call and only
perform the launch/feedback after popping.
Comment on lines
+19
to
+22
| final response = await http.get( | ||
| Uri.parse(AppConstants.githubApiLatestRelease), | ||
| headers: {'Accept': 'application/vnd.github.v3+json'}, | ||
| ); |
There was a problem hiding this comment.
Add a timeout to the GitHub request.
The network call is unbounded; a stalled connection can keep the update check spinning indefinitely.
Proposed fix
import 'dart:convert';
+import 'dart:async';
import 'package:http/http.dart' as http;
import '../constants.dart';
@@
- final response = await http.get(
- Uri.parse(AppConstants.githubApiLatestRelease),
- headers: {'Accept': 'application/vnd.github.v3+json'},
- );
+ final response = await http
+ .get(
+ Uri.parse(AppConstants.githubApiLatestRelease),
+ headers: {'Accept': 'application/vnd.github.v3+json'},
+ )
+ .timeout(const Duration(seconds: 10));📝 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
| final response = await http.get( | |
| Uri.parse(AppConstants.githubApiLatestRelease), | |
| headers: {'Accept': 'application/vnd.github.v3+json'}, | |
| ); | |
| final response = await http | |
| .get( | |
| Uri.parse(AppConstants.githubApiLatestRelease), | |
| headers: {'Accept': 'application/vnd.github.v3+json'}, | |
| ) | |
| .timeout(const Duration(seconds: 10)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flutter_app/lib/services/update_service.dart` around lines 19 - 22, The
GitHub API call using http.get against AppConstants.githubApiLatestRelease is
unbounded; wrap the http.get request (the expression that assigns response) with
a timeout (e.g., .timeout(Duration(seconds: X)) ) and handle TimeoutException
(or generic Exception) so the update check fails fast and is handled cleanly
(log/return) instead of hanging; update the same call site where response is
assigned and ensure headers remain unchanged.
Comment on lines
+41
to
+42
| final r = remote.split('.').map(int.parse).toList(); | ||
| final l = local.split('.').map(int.parse).toList(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "update_service.dart" -type fRepository: mithun50/openclaw-termux
Length of output: 114
🏁 Script executed:
cat -n flutter_app/lib/services/update_service.dart | head -60Repository: mithun50/openclaw-termux
Length of output: 1936
🌐 Web query:
GitHub API releases endpoint response tag_name format documentation
💡 Result:
In the GitHub REST API Releases endpoints, tag_name is a string containing the Git tag name associated with the release (e.g. "v1.0.0" in the documented responses) and, when creating a release, it’s described simply as “The tag name for the release. This can be an existing tag or a new one.” [1]
GitHub does not document a special tag_name format (e.g., it’s not required to be SemVer); it just needs to be a valid Git ref/tag name, i.e., it must satisfy Git’s reference-name rules (see git-check-ref-format). [2]
Sources: [1] https://docs.github.com/en/rest/releases/releases, [2] https://git-scm.com/docs/git-check-ref-format
🏁 Script executed:
# Test Dart int.parse behavior with various inputs
cat > /tmp/test_parse.dart <<'EOF'
void main() {
// Test cases
var tests = [
"4", // valid
"4+15", // has + (build metadata)
"4-beta", // has - (prerelease)
"0", // valid
];
for (var test in tests) {
try {
var result = int.parse(test);
print("int.parse('$test') = $result");
} catch (e) {
print("int.parse('$test') throws: $e");
}
}
}
EOF
# Run it via web to understand Dart behavior
dart /tmp/test_parse.dart 2>&1 || echo "Dart not available in sandbox; checking via logic"Repository: mithun50/openclaw-termux
Length of output: 161
🌐 Web query:
mithun50 openclaw-termux GitHub releases tag_name format
💡 Result:
For mithun50/openclaw-termux, GitHub Release tag_name uses SemVer with a leading v:
- Format:
vMAJOR.MINOR.PATCH(e.g.,v1.8.3,v1.8.2) (github.com)
Citations:
🏁 Script executed:
# Fetch actual GitHub API response for this repository
curl -s "https://api.github.com/repos/mithun50/openclaw-termux/releases/latest" | python3 <<'EOF'
import json
import sys
try:
data = json.load(sys.stdin)
if 'tag_name' in data:
tag = data['tag_name']
print(f"Latest tag_name: {tag}")
print(f"Tag after stripping 'v': {tag[1:] if tag.startswith('v') else tag}")
# Simulate split behavior
version_str = tag[1:] if tag.startswith('v') else tag
parts = version_str.split('.')
print(f"After split('.'): {parts}")
# Try to parse each part
for i, part in enumerate(parts):
try:
int(part)
print(f" parts[{i}] = '{part}' → can parse as int")
except ValueError:
print(f" parts[{i}] = '{part}' → CANNOT parse as int (would throw FormatException)")
else:
print("No tag_name in response")
except Exception as e:
print(f"Error: {e}")
EOFRepository: mithun50/openclaw-termux
Length of output: 116
🌐 Web query:
Dart int.parse FormatException non-numeric string behavior
💡 Result:
int.parse(source) in Dart expects source to be only a valid (optionally signed) integer literal in the given base (radix). If you pass a non-numeric string like "abc" (or anything with invalid characters for the radix, empty string, etc.), it throws a FormatException. [1]
- Valid form: a non-empty sequence of base‑
radixdigits, optionally prefixed with+or-. [1] radixmust be 2..36; if omitted it defaults to 10 (and in radix 10 it also accepts a0xprefix for hex). [1]- On invalid input, you’ll typically see an error like:
FormatException: Invalid radix-10 number (at character 1)(message/details may vary). [2]
If you don’t want an exception, use int.tryParse(source) which returns null for invalid input instead. [3]
Sources:
[1] Dart API: int.parse docs https://api.dart.dev/dart-core/int/parse.html
[2] Example showing the exception message (Learn X By Example) https://learnxbyexample.com/dart/number-parsing/
[3] Dart API: int.tryParse docs https://api.dart.dev/dart-core/int/tryParse.html
Handle prerelease and build metadata in semver comparison to prevent crashes.
The code will crash if a GitHub release tag includes prerelease (-beta) or build metadata (+15) because int.parse throws FormatException on non-numeric strings. While current releases for this repository use clean SemVer (e.g., v1.8.3), the code is fragile to tag format changes. Extract the core version before parsing to handle edge cases gracefully.
Proposed fix
/// Returns true if [remote] is newer than [local] by semver comparison.
static bool _isNewer(String remote, String local) {
- final r = remote.split('.').map(int.parse).toList();
- final l = local.split('.').map(int.parse).toList();
+ List<int> parseCore(String v) {
+ final core = v.split('+').first.split('-').first; // drop build/prerelease
+ final parts = core.split('.');
+ return List<int>.generate(
+ 3,
+ (i) => i < parts.length ? (int.tryParse(parts[i]) ?? 0) : 0,
+ );
+ }
+
+ final r = parseCore(remote);
+ final l = parseCore(local);
for (var i = 0; i < 3; i++) {
final rv = i < r.length ? r[i] : 0;
final lv = i < l.length ? l[i] : 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flutter_app/lib/services/update_service.dart` around lines 41 - 42, The
current parsing of remote and local version strings (variables remote and local,
producing lists r and l) crashes on prerelease/build metadata; update the logic
to first normalize the tag by stripping a leading "v/V" and extracting the core
semver part before any '-' or '+' (e.g., take remoteCore =
remote.trim().replaceFirst(RegExp(r'^[vV]'), '').split(RegExp(r'[-+]'))[0]) and
then split on '.' and int.parse each numeric identifier to produce r and l,
handling missing patch/minor by treating absent segments as 0; apply the same
normalization to local so the semver comparison code using r and l no longer
throws FormatException.
…ies (#56) - Add ENTER button to terminal toolbar for CJK IME compatibility - Add Android backup/extraction rules, auto-snapshot on version upgrade - Enrich snapshot export/import with node connection preferences - Bring app to foreground before invoking camera/screen/sensor/flash/location Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
There was a problem hiding this comment.
2 issues found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="flutter_app/lib/screens/settings_screen.dart">
<violation number="1" location="flutter_app/lib/screens/settings_screen.dart:367">
P2: Authentication tokens (`nodeDeviceToken`, `nodeGatewayToken`) are exported in plaintext to the public Downloads directory, where other apps with storage permission can read them. Consider either warning the user that the export contains sensitive credentials, encrypting the snapshot file, or writing it to app-private storage (`getApplicationDocumentsDirectory()`) by default instead of only as a fallback.</violation>
</file>
<file name="flutter_app/lib/screens/splash_screen.dart">
<violation number="1" location="flutter_app/lib/screens/splash_screen.dart:90">
P1: Security: Authentication tokens (`nodeDeviceToken`, `nodeGatewayToken`) and the full `openclawConfig` are written in plaintext to the shared Downloads folder on external storage. Any app with storage permission (or the user) can read these credentials. Either exclude sensitive fields from the snapshot, or encrypt the file, or write to app-private internal storage instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+90
to
+101
| final snapshot = { | ||
| 'version': oldVersion, | ||
| 'timestamp': DateTime.now().toIso8601String(), | ||
| 'openclawConfig': openclawJson, | ||
| 'dashboardUrl': prefs.dashboardUrl, | ||
| 'autoStart': prefs.autoStartGateway, | ||
| 'nodeEnabled': prefs.nodeEnabled, | ||
| 'nodeDeviceToken': prefs.nodeDeviceToken, | ||
| 'nodeGatewayHost': prefs.nodeGatewayHost, | ||
| 'nodeGatewayPort': prefs.nodeGatewayPort, | ||
| 'nodeGatewayToken': prefs.nodeGatewayToken, | ||
| }; |
There was a problem hiding this comment.
P1: Security: Authentication tokens (nodeDeviceToken, nodeGatewayToken) and the full openclawConfig are written in plaintext to the shared Downloads folder on external storage. Any app with storage permission (or the user) can read these credentials. Either exclude sensitive fields from the snapshot, or encrypt the file, or write to app-private internal storage instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flutter_app/lib/screens/splash_screen.dart, line 90:
<comment>Security: Authentication tokens (`nodeDeviceToken`, `nodeGatewayToken`) and the full `openclawConfig` are written in plaintext to the shared Downloads folder on external storage. Any app with storage permission (or the user) can read these credentials. Either exclude sensitive fields from the snapshot, or encrypt the file, or write to app-private internal storage instead.</comment>
<file context>
@@ -73,6 +74,39 @@ class _SplashScreenState extends State<SplashScreen>
+ }
+ final snapshotPath = '$sdcard/Download/openclaw-snapshot-$oldVersion.json';
+ final openclawJson = await NativeBridge.readRootfsFile('root/.openclaw/openclaw.json');
+ final snapshot = {
+ 'version': oldVersion,
+ 'timestamp': DateTime.now().toIso8601String(),
</file context>
Suggested change
| final snapshot = { | |
| 'version': oldVersion, | |
| 'timestamp': DateTime.now().toIso8601String(), | |
| 'openclawConfig': openclawJson, | |
| 'dashboardUrl': prefs.dashboardUrl, | |
| 'autoStart': prefs.autoStartGateway, | |
| 'nodeEnabled': prefs.nodeEnabled, | |
| 'nodeDeviceToken': prefs.nodeDeviceToken, | |
| 'nodeGatewayHost': prefs.nodeGatewayHost, | |
| 'nodeGatewayPort': prefs.nodeGatewayPort, | |
| 'nodeGatewayToken': prefs.nodeGatewayToken, | |
| }; | |
| final snapshot = { | |
| 'version': oldVersion, | |
| 'timestamp': DateTime.now().toIso8601String(), | |
| 'dashboardUrl': prefs.dashboardUrl, | |
| 'autoStart': prefs.autoStartGateway, | |
| 'nodeEnabled': prefs.nodeEnabled, | |
| 'nodeGatewayHost': prefs.nodeGatewayHost, | |
| 'nodeGatewayPort': prefs.nodeGatewayPort, | |
| }; |
| 'nodeDeviceToken': _prefs.nodeDeviceToken, | ||
| 'nodeGatewayHost': _prefs.nodeGatewayHost, | ||
| 'nodeGatewayPort': _prefs.nodeGatewayPort, | ||
| 'nodeGatewayToken': _prefs.nodeGatewayToken, |
There was a problem hiding this comment.
P2: Authentication tokens (nodeDeviceToken, nodeGatewayToken) are exported in plaintext to the public Downloads directory, where other apps with storage permission can read them. Consider either warning the user that the export contains sensitive credentials, encrypting the snapshot file, or writing it to app-private storage (getApplicationDocumentsDirectory()) by default instead of only as a fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flutter_app/lib/screens/settings_screen.dart, line 367:
<comment>Authentication tokens (`nodeDeviceToken`, `nodeGatewayToken`) are exported in plaintext to the public Downloads directory, where other apps with storage permission can read them. Consider either warning the user that the export contains sensitive credentials, encrypting the snapshot file, or writing it to app-private storage (`getApplicationDocumentsDirectory()`) by default instead of only as a fallback.</comment>
<file context>
@@ -361,6 +361,10 @@ class _SettingsScreenState extends State<SettingsScreen> {
+ 'nodeDeviceToken': _prefs.nodeDeviceToken,
+ 'nodeGatewayHost': _prefs.nodeGatewayHost,
+ 'nodeGatewayPort': _prefs.nodeGatewayPort,
+ 'nodeGatewayToken': _prefs.nodeGatewayToken,
};
</file context>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
flutter_app/android/app/src/main/res/xml/backup_rules.xml (1)
4-5:⚠️ Potential issue | 🟠 MajorSensitive app data can still be included in backups.
Including all shared prefs while only excluding
rootfs/can still back up auth/config data (including token-bearing prefs and likelyopenclaw.jsonif stored outsiderootfs/). Please explicitly exclude sensitive persisted files from both backup rule sets.🔧 Suggested hardening (after path verification)
<full-backup-content> <include domain="sharedpref" path="." /> + <exclude domain="sharedpref" path="FlutterSharedPreferences.xml" /> <exclude domain="file" path="rootfs/" /> + <exclude domain="file" path="openclaw.json" /> </full-backup-content>#!/bin/bash set -euo pipefail echo "== Sensitive key usage ==" rg -n "nodeGatewayToken|nodeDeviceToken|dashboardUrl|token|openclaw\\.json" flutter_app/lib -C2 || true echo echo "== openclaw.json write/read locations ==" rg -n "openclaw\\.json|writeRootfsFile|readRootfsFile|writeNodeAllowConfig|allowCommands" flutter_app -C2 || true echo echo "== Backup rule files ==" for f in \ flutter_app/android/app/src/main/res/xml/backup_rules.xml \ flutter_app/android/app/src/main/res/xml/data_extraction_rules.xml do if [ -f "$f" ]; then echo "--- $f ---" cat "$f" else echo "MISSING: $f" fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flutter_app/android/app/src/main/res/xml/backup_rules.xml` around lines 4 - 5, The backup rules currently include all shared preferences via the <include domain="sharedpref" path="." /> while only excluding rootfs/, which can still back up sensitive prefs and files like openclaw.json; update the Android backup rule files (the <include>/<exclude> entries in backup_rules.xml and also mirror changes in data_extraction_rules.xml) to explicitly exclude known sensitive keys and paths (e.g., token-bearing shared-prefs and openclaw.json) by adding explicit <exclude domain="sharedpref" path="NAME_OF_PREF_FILE_OR_KEY"/> and <exclude domain="file" path="openclaw.json"/> entries (or their correct filenames/paths observed in the app) so those persisted secrets are never included in backups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flutter_app/lib/screens/splash_screen.dart`:
- Around line 90-101: The snapshot map in splash_screen.dart currently includes
sensitive fields nodeDeviceToken and nodeGatewayToken which are exported to the
Downloads snapshot; remove these keys (or replace them with a non-secret
placeholder like "[REDACTED]") before writing the snapshot to disk so secrets
are never written in plaintext. Locate the snapshot variable and ensure
nodeDeviceToken and nodeGatewayToken are excluded or masked prior to the code
that saves/exports the snapshot.
- Around line 78-108: The snapshot export code can throw and currently swallows
exceptions so prefs.lastAppVersion is not updated; move the assignment
prefs.lastAppVersion = AppConstants.version out of the try body into a finally
block (after the try/catch) so it always runs regardless of errors during
NativeBridge calls, Directory/File operations, or JSON encoding; keep the
existing catch (_) {} behavior or replace it with more specific error handling
if desired, but ensure the finally always sets prefs.lastAppVersion to
AppConstants.version.
In `@flutter_app/lib/services/node_service.dart`:
- Around line 21-25: _isAppInForeground is initialized true and used to gate
foreground-only handlers but isn't re-checked after bringToForeground(),
allowing handlers (camera/screen/sensor/flash/location) to run while
backgrounded; change the initialization to reflect actual app state and add
explicit state re-checks/guards: update _isAppInForeground initialization to
derive real state at startup, ensure setAppInForeground(bool) is the single
source of truth, and add early-return guards that check _isAppInForeground at
the start of each foreground-sensitive method (reference the field
_isAppInForeground and methods like setAppInForeground and bringToForeground),
and add a post-bringToForeground verification (re-check _isAppInForeground)
before invoking any camera/sensor/flash/location handlers; also apply the same
guard additions for the handlers around lines 373-395.
---
Duplicate comments:
In `@flutter_app/android/app/src/main/res/xml/backup_rules.xml`:
- Around line 4-5: The backup rules currently include all shared preferences via
the <include domain="sharedpref" path="." /> while only excluding rootfs/, which
can still back up sensitive prefs and files like openclaw.json; update the
Android backup rule files (the <include>/<exclude> entries in backup_rules.xml
and also mirror changes in data_extraction_rules.xml) to explicitly exclude
known sensitive keys and paths (e.g., token-bearing shared-prefs and
openclaw.json) by adding explicit <exclude domain="sharedpref"
path="NAME_OF_PREF_FILE_OR_KEY"/> and <exclude domain="file"
path="openclaw.json"/> entries (or their correct filenames/paths observed in the
app) so those persisted secrets are never included in backups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ef18834-8f70-4ac6-a2ab-cc19ea390bbf
📒 Files selected for processing (11)
flutter_app/android/app/src/main/AndroidManifest.xmlflutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/MainActivity.ktflutter_app/android/app/src/main/res/xml/backup_rules.xmlflutter_app/android/app/src/main/res/xml/data_extraction_rules.xmlflutter_app/lib/providers/node_provider.dartflutter_app/lib/screens/settings_screen.dartflutter_app/lib/screens/splash_screen.dartflutter_app/lib/services/native_bridge.dartflutter_app/lib/services/node_service.dartflutter_app/lib/services/preferences_service.dartflutter_app/lib/widgets/terminal_toolbar.dart
✅ Files skipped from review due to trivial changes (1)
- flutter_app/android/app/src/main/res/xml/data_extraction_rules.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- flutter_app/lib/screens/settings_screen.dart
- flutter_app/lib/providers/node_provider.dart
Comment on lines
+78
to
+108
| try { | ||
| final oldVersion = prefs.lastAppVersion; | ||
| if (oldVersion != null && oldVersion != AppConstants.version) { | ||
| final hasPermission = await NativeBridge.hasStoragePermission(); | ||
| if (hasPermission) { | ||
| final sdcard = await NativeBridge.getExternalStoragePath(); | ||
| final downloadDir = Directory('$sdcard/Download'); | ||
| if (!await downloadDir.exists()) { | ||
| await downloadDir.create(recursive: true); | ||
| } | ||
| final snapshotPath = '$sdcard/Download/openclaw-snapshot-$oldVersion.json'; | ||
| final openclawJson = await NativeBridge.readRootfsFile('root/.openclaw/openclaw.json'); | ||
| final snapshot = { | ||
| 'version': oldVersion, | ||
| 'timestamp': DateTime.now().toIso8601String(), | ||
| 'openclawConfig': openclawJson, | ||
| 'dashboardUrl': prefs.dashboardUrl, | ||
| 'autoStart': prefs.autoStartGateway, | ||
| 'nodeEnabled': prefs.nodeEnabled, | ||
| 'nodeDeviceToken': prefs.nodeDeviceToken, | ||
| 'nodeGatewayHost': prefs.nodeGatewayHost, | ||
| 'nodeGatewayPort': prefs.nodeGatewayPort, | ||
| 'nodeGatewayToken': prefs.nodeGatewayToken, | ||
| }; | ||
| await File(snapshotPath).writeAsString( | ||
| const JsonEncoder.withIndent(' ').convert(snapshot), | ||
| ); | ||
| } | ||
| } | ||
| prefs.lastAppVersion = AppConstants.version; | ||
| } catch (_) {} |
There was a problem hiding this comment.
Ensure lastAppVersion is updated even when export fails.
Right now, an exception before Line 107 skips the version write, so snapshot export may retry forever across launches.
🛠️ Proposed fix (move version write to finally)
// Auto-export snapshot when app version changes (`#55`)
- try {
+ try {
final oldVersion = prefs.lastAppVersion;
if (oldVersion != null && oldVersion != AppConstants.version) {
final hasPermission = await NativeBridge.hasStoragePermission();
if (hasPermission) {
final sdcard = await NativeBridge.getExternalStoragePath();
final downloadDir = Directory('$sdcard/Download');
if (!await downloadDir.exists()) {
await downloadDir.create(recursive: true);
}
final snapshotPath = '$sdcard/Download/openclaw-snapshot-$oldVersion.json';
final openclawJson = await NativeBridge.readRootfsFile('root/.openclaw/openclaw.json');
final snapshot = {
'version': oldVersion,
'timestamp': DateTime.now().toIso8601String(),
'openclawConfig': openclawJson,
'dashboardUrl': prefs.dashboardUrl,
'autoStart': prefs.autoStartGateway,
'nodeEnabled': prefs.nodeEnabled,
'nodeDeviceToken': prefs.nodeDeviceToken,
'nodeGatewayHost': prefs.nodeGatewayHost,
'nodeGatewayPort': prefs.nodeGatewayPort,
'nodeGatewayToken': prefs.nodeGatewayToken,
};
await File(snapshotPath).writeAsString(
const JsonEncoder.withIndent(' ').convert(snapshot),
);
}
}
- prefs.lastAppVersion = AppConstants.version;
- } catch (_) {}
+ } catch (_) {
+ } finally {
+ prefs.lastAppVersion = AppConstants.version;
+ }📝 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
| try { | |
| final oldVersion = prefs.lastAppVersion; | |
| if (oldVersion != null && oldVersion != AppConstants.version) { | |
| final hasPermission = await NativeBridge.hasStoragePermission(); | |
| if (hasPermission) { | |
| final sdcard = await NativeBridge.getExternalStoragePath(); | |
| final downloadDir = Directory('$sdcard/Download'); | |
| if (!await downloadDir.exists()) { | |
| await downloadDir.create(recursive: true); | |
| } | |
| final snapshotPath = '$sdcard/Download/openclaw-snapshot-$oldVersion.json'; | |
| final openclawJson = await NativeBridge.readRootfsFile('root/.openclaw/openclaw.json'); | |
| final snapshot = { | |
| 'version': oldVersion, | |
| 'timestamp': DateTime.now().toIso8601String(), | |
| 'openclawConfig': openclawJson, | |
| 'dashboardUrl': prefs.dashboardUrl, | |
| 'autoStart': prefs.autoStartGateway, | |
| 'nodeEnabled': prefs.nodeEnabled, | |
| 'nodeDeviceToken': prefs.nodeDeviceToken, | |
| 'nodeGatewayHost': prefs.nodeGatewayHost, | |
| 'nodeGatewayPort': prefs.nodeGatewayPort, | |
| 'nodeGatewayToken': prefs.nodeGatewayToken, | |
| }; | |
| await File(snapshotPath).writeAsString( | |
| const JsonEncoder.withIndent(' ').convert(snapshot), | |
| ); | |
| } | |
| } | |
| prefs.lastAppVersion = AppConstants.version; | |
| } catch (_) {} | |
| try { | |
| final oldVersion = prefs.lastAppVersion; | |
| if (oldVersion != null && oldVersion != AppConstants.version) { | |
| final hasPermission = await NativeBridge.hasStoragePermission(); | |
| if (hasPermission) { | |
| final sdcard = await NativeBridge.getExternalStoragePath(); | |
| final downloadDir = Directory('$sdcard/Download'); | |
| if (!await downloadDir.exists()) { | |
| await downloadDir.create(recursive: true); | |
| } | |
| final snapshotPath = '$sdcard/Download/openclaw-snapshot-$oldVersion.json'; | |
| final openclawJson = await NativeBridge.readRootfsFile('root/.openclaw/openclaw.json'); | |
| final snapshot = { | |
| 'version': oldVersion, | |
| 'timestamp': DateTime.now().toIso8601String(), | |
| 'openclawConfig': openclawJson, | |
| 'dashboardUrl': prefs.dashboardUrl, | |
| 'autoStart': prefs.autoStartGateway, | |
| 'nodeEnabled': prefs.nodeEnabled, | |
| 'nodeDeviceToken': prefs.nodeDeviceToken, | |
| 'nodeGatewayHost': prefs.nodeGatewayHost, | |
| 'nodeGatewayPort': prefs.nodeGatewayPort, | |
| 'nodeGatewayToken': prefs.nodeGatewayToken, | |
| }; | |
| await File(snapshotPath).writeAsString( | |
| const JsonEncoder.withIndent(' ').convert(snapshot), | |
| ); | |
| } | |
| } | |
| } catch (_) { | |
| } finally { | |
| prefs.lastAppVersion = AppConstants.version; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flutter_app/lib/screens/splash_screen.dart` around lines 78 - 108, The
snapshot export code can throw and currently swallows exceptions so
prefs.lastAppVersion is not updated; move the assignment prefs.lastAppVersion =
AppConstants.version out of the try body into a finally block (after the
try/catch) so it always runs regardless of errors during NativeBridge calls,
Directory/File operations, or JSON encoding; keep the existing catch (_) {}
behavior or replace it with more specific error handling if desired, but ensure
the finally always sets prefs.lastAppVersion to AppConstants.version.
Comment on lines
+90
to
+101
| final snapshot = { | ||
| 'version': oldVersion, | ||
| 'timestamp': DateTime.now().toIso8601String(), | ||
| 'openclawConfig': openclawJson, | ||
| 'dashboardUrl': prefs.dashboardUrl, | ||
| 'autoStart': prefs.autoStartGateway, | ||
| 'nodeEnabled': prefs.nodeEnabled, | ||
| 'nodeDeviceToken': prefs.nodeDeviceToken, | ||
| 'nodeGatewayHost': prefs.nodeGatewayHost, | ||
| 'nodeGatewayPort': prefs.nodeGatewayPort, | ||
| 'nodeGatewayToken': prefs.nodeGatewayToken, | ||
| }; |
There was a problem hiding this comment.
Do not export sensitive tokens to public Downloads.
nodeDeviceToken and nodeGatewayToken are being written in plaintext to a broadly accessible path (Line 88), which can leak credentials.
🔐 Proposed fix (exclude secrets from snapshot)
final snapshot = {
'version': oldVersion,
'timestamp': DateTime.now().toIso8601String(),
'openclawConfig': openclawJson,
'dashboardUrl': prefs.dashboardUrl,
'autoStart': prefs.autoStartGateway,
'nodeEnabled': prefs.nodeEnabled,
- 'nodeDeviceToken': prefs.nodeDeviceToken,
'nodeGatewayHost': prefs.nodeGatewayHost,
'nodeGatewayPort': prefs.nodeGatewayPort,
- 'nodeGatewayToken': prefs.nodeGatewayToken,
+ // Intentionally omit credentials/tokens from public snapshot exports.
};📝 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
| final snapshot = { | |
| 'version': oldVersion, | |
| 'timestamp': DateTime.now().toIso8601String(), | |
| 'openclawConfig': openclawJson, | |
| 'dashboardUrl': prefs.dashboardUrl, | |
| 'autoStart': prefs.autoStartGateway, | |
| 'nodeEnabled': prefs.nodeEnabled, | |
| 'nodeDeviceToken': prefs.nodeDeviceToken, | |
| 'nodeGatewayHost': prefs.nodeGatewayHost, | |
| 'nodeGatewayPort': prefs.nodeGatewayPort, | |
| 'nodeGatewayToken': prefs.nodeGatewayToken, | |
| }; | |
| final snapshot = { | |
| 'version': oldVersion, | |
| 'timestamp': DateTime.now().toIso8601String(), | |
| 'openclawConfig': openclawJson, | |
| 'dashboardUrl': prefs.dashboardUrl, | |
| 'autoStart': prefs.autoStartGateway, | |
| 'nodeEnabled': prefs.nodeEnabled, | |
| 'nodeGatewayHost': prefs.nodeGatewayHost, | |
| 'nodeGatewayPort': prefs.nodeGatewayPort, | |
| // Intentionally omit credentials/tokens from public snapshot exports. | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flutter_app/lib/screens/splash_screen.dart` around lines 90 - 101, The
snapshot map in splash_screen.dart currently includes sensitive fields
nodeDeviceToken and nodeGatewayToken which are exported to the Downloads
snapshot; remove these keys (or replace them with a non-secret placeholder like
"[REDACTED]") before writing the snapshot to disk so secrets are never written
in plaintext. Locate the snapshot variable and ensure nodeDeviceToken and
nodeGatewayToken are excluded or masked prior to the code that saves/exports the
snapshot.
Comment on lines
+21
to
+25
| bool _isAppInForeground = true; | ||
|
|
||
| void setAppInForeground(bool value) { | ||
| _isAppInForeground = value; | ||
| } |
There was a problem hiding this comment.
Foreground-sensitive invoke path can still execute while app is backgrounded.
_isAppInForeground starts as true, and after bringToForeground() there is no state re-check before continuing. That can let camera/screen/sensor/flash/location handlers run without a real foreground transition.
✅ Suggested fix
- bool _isAppInForeground = true;
+ bool _isAppInForeground = false;
...
if (foregroundCommands.contains(commandPrefix) && !_isAppInForeground) {
_log('[NODE] App backgrounded, bringing to foreground for $command');
try {
- await NativeBridge.bringToForeground();
+ final broughtToForeground = await NativeBridge.bringToForeground();
await Future.delayed(const Duration(milliseconds: 800));
+ if (!broughtToForeground || !_isAppInForeground) {
+ _ws.sendRequest(NodeFrame.request('node.invoke.result', {
+ 'id': requestId,
+ 'nodeId': nodeId,
+ 'ok': false,
+ 'error': {
+ 'code': 'APP_BACKGROUNDED',
+ 'message': 'Cannot bring app to foreground for $command',
+ },
+ }));
+ return;
+ }
} catch (e) {
_log('[NODE] Failed to bring app to foreground: $e');
_ws.sendRequest(NodeFrame.request('node.invoke.result', {Also applies to: 373-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flutter_app/lib/services/node_service.dart` around lines 21 - 25,
_isAppInForeground is initialized true and used to gate foreground-only handlers
but isn't re-checked after bringToForeground(), allowing handlers
(camera/screen/sensor/flash/location) to run while backgrounded; change the
initialization to reflect actual app state and add explicit state
re-checks/guards: update _isAppInForeground initialization to derive real state
at startup, ensure setAppInForeground(bool) is the single source of truth, and
add early-return guards that check _isAppInForeground at the start of each
foreground-sensitive method (reference the field _isAppInForeground and methods
like setAppInForeground and bringToForeground), and add a post-bringToForeground
verification (re-check _isAppInForeground) before invoking any
camera/sensor/flash/location handlers; also apply the same guard additions for
the handlers around lines 373-395.
Build FailedThe build failed for commit 899bec4. |
Closed
Only sharedpref domain is included, so file domain excludes are unnecessary and cause FullBackupContent lint errors during build. Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flutter_app/android/app/src/main/res/xml/backup_rules.xml`:
- Line 4: The backup rules currently include all SharedPreferences (path=".")
which exposes sensitive items; update Android backup_rules.xml (and
data_extraction_rules.xml) to explicitly exclude the SharedPreferences files
used by node_identity_service.dart and preferences_service.dart that store
_keyPrivate, _keyPublic, node_device_token and node_gateway_token, or instead
migrate those stores to Android EncryptedSharedPreferences/Keystore;
specifically add <exclude> entries for the SharedPreferences filenames used by
the code (or switch code to use EncryptedSharedPreferences/Keystore when
writing/reading keys and tokens) so private keys and auth tokens are not backed
up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 915bd24e-2b49-425e-98a6-2b4afd2b2063
📒 Files selected for processing (2)
flutter_app/android/app/src/main/res/xml/backup_rules.xmlflutter_app/android/app/src/main/res/xml/data_extraction_rules.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- flutter_app/android/app/src/main/res/xml/data_extraction_rules.xml
| <?xml version="1.0" encoding="utf-8"?> | ||
| <!-- fullBackupContent rules for Android ≤11 --> | ||
| <full-backup-content> | ||
| <include domain="sharedpref" path="." /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Android SharedPreferences usage (Kotlin/Java) =="
rg -n -C2 --type=kt --type=java \
'getSharedPreferences\(|EncryptedSharedPreferences|PreferenceManager\.getDefaultSharedPreferences' || true
echo
echo "== Dart persistence usage (shared_preferences / secure storage) =="
rg -n -C2 --type=dart \
'SharedPreferences\.getInstance|shared_preferences|flutter_secure_storage' || true
echo
echo "== Potentially sensitive key names in Dart code =="
rg -n -C2 --type=dart \
'(token|secret|password|api[_-]?key|auth)' flutter_app/lib || true
echo
echo "== Current backup XML resources =="
for f in $(fd 'backup_rules\.xml|data_extraction_rules\.xml' flutter_app/android/app/src/main/res/xml); do
echo "--- $f ---"
cat "$f"
doneRepository: mithun50/openclaw-termux
Length of output: 28711
Exclude private keys and auth tokens from backup rules — they are currently exposed.
Verification confirms this is a critical security issue. The app stores sensitive data in SharedPreferences that will be backed up:
- Ed25519 private keys (
_keyPrivate,_keyPublic) innode_identity_service.dart - Authentication tokens (
node_device_token,node_gateway_token) inpreferences_service.dart
The current rules (path=".") include all SharedPreferences files without exclusion. Add <exclude> entries to both backup_rules.xml and data_extraction_rules.xml for files containing these keys, or migrate private keys and tokens to Android's encrypted EncryptedSharedPreferences or Keystore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flutter_app/android/app/src/main/res/xml/backup_rules.xml` at line 4, The
backup rules currently include all SharedPreferences (path=".") which exposes
sensitive items; update Android backup_rules.xml (and data_extraction_rules.xml)
to explicitly exclude the SharedPreferences files used by
node_identity_service.dart and preferences_service.dart that store _keyPrivate,
_keyPublic, node_device_token and node_gateway_token, or instead migrate those
stores to Android EncryptedSharedPreferences/Keystore; specifically add
<exclude> entries for the SharedPreferences filenames used by the code (or
switch code to use EncryptedSharedPreferences/Keystore when writing/reading keys
and tokens) so private keys and auth tokens are not backed up.
- Bind sshd to 0.0.0.0 explicitly so VPN network changes don't crash it - Auto-restart sshd up to 3 times on unexpected exit - Log sshd stderr for debugging - Read system DNS servers from ConnectivityManager instead of hardcoding 8.8.8.8; fall back to Google DNS when system DNS unavailable - Always refresh resolv.conf with current system DNS on service start Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/SshForegroundService.kt">
<violation number="1" location="flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/SshForegroundService.kt:171">
P1: `isRunning` is read cross-thread without `@Volatile`, so this visibility check may not work correctly. `onDestroy()` sets `isRunning = false` on the main thread, but this worker thread may never see the update due to JMM caching, causing unintended sshd restarts after an intentional stop. Add `@Volatile` to the `isRunning` declaration in the companion object.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| val exitCode = sshdProcess!!.waitFor() | ||
|
|
||
| if (!isRunning) break // Intentional stop |
There was a problem hiding this comment.
P1: isRunning is read cross-thread without @Volatile, so this visibility check may not work correctly. onDestroy() sets isRunning = false on the main thread, but this worker thread may never see the update due to JMM caching, causing unintended sshd restarts after an intentional stop. Add @Volatile to the isRunning declaration in the companion object.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/SshForegroundService.kt, line 171:
<comment>`isRunning` is read cross-thread without `@Volatile`, so this visibility check may not work correctly. `onDestroy()` sets `isRunning = false` on the main thread, but this worker thread may never see the update due to JMM caching, causing unintended sshd restarts after an intentional stop. Add `@Volatile` to the `isRunning` declaration in the companion object.</comment>
<file context>
@@ -114,52 +115,71 @@ class SshForegroundService : Service() {
+
+ val exitCode = sshdProcess!!.waitFor()
+
+ if (!isRunning) break // Intentional stop
+
+ restartCount++
</file context>
When the app or service restarts, the old proot openclaw gateway process may still be alive, leading to duplicate instances and repeated messages on channels like Telegram. Now kills any existing openclaw gateway processes before launching a new one. Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
The rootfs contains a symlink at rootfs/sdcard -> /storage/emulated/0. When extractRootfs() retried and called deleteRecursively() to clean up, it followed this symlink and deleted the user's actual photos, downloads, and other files from shared storage. Now checks for symlinks using Files.isSymbolicLink() and deletes the link itself without traversing into the target directory. Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/BootstrapManager.kt">
<violation number="1" location="flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/BootstrapManager.kt:802">
P1: Catch-all exception handler silently falls through to symlink-following code path. If the `Files.isSymbolicLink` check fails for any reason, `file.isDirectory` below follows symlinks and would recurse into real user directories (e.g. `/storage/emulated/0`). The safe fallback on exception should be to skip the file (not recurse), matching the stated CRITICAL safety intent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/BootstrapManager.kt
Show resolved
Hide resolved
The openclaw gateway schema requires models.providers.<id>.models to be an array. The provider config was only saving apiKey and baseUrl, causing "expected array, received undefined" validation errors that crashed the gateway on startup. Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
The pkill command spawns a separate proot process which can interfere with gateway startup. Reverts the multi-instance fix from 67a3969. Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
Instead of pkill, check if port 18789 is already in use before launching a new proot gateway process. If an existing gateway is healthy, adopt it rather than spawning a duplicate.
…ing deps - Remove auto storage permission request on launch (#67, #63) - Add path boundary check to deleteRecursively to prevent data loss - Change storage icon to warning when granted in settings - Increase gateway maxRestarts to 5, reset counter on long uptime (#60) - Log all stderr on first startup attempt for debugging - Add 15s startup grace period before health checks in Dart side - Show notification with dashboard URL on token detection (#64) - Add copy button for dashboard URL with token hint (#64) - Mark Canvas capability as unavailable on mobile (#65) - Add curl and wget to bootstrap apt-get install (#66) Closes #67, #63, #65, #64, #66 Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
There was a problem hiding this comment.
3 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="flutter_app/lib/services/gateway_service.dart">
<violation number="1" location="flutter_app/lib/services/gateway_service.dart:246">
P1: Using `Future.delayed` here leaks periodic timers on rapid stop/start cycles. Unlike `Timer`, a `Future.delayed` cannot be cancelled, so if the gateway restarts within 15 seconds the old callback still fires and creates an orphaned `Timer.periodic`. Use a cancellable `Timer` instead and cancel it alongside `_healthTimer` in `stop()`.</violation>
</file>
<file name="flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/BootstrapManager.kt">
<violation number="1" location="flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/BootstrapManager.kt:800">
P2: Path prefix check without a trailing separator: `startsWith(filesDir)` would also match sibling directories like `files-extra/`. Append `File.separator` to the prefix to ensure the match is on a directory boundary.</violation>
<violation number="2" location="flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/BootstrapManager.kt:800">
P1: `canonicalPath` resolves symlinks, which prevents the symlink guard below from ever being reached. If a symlink inside the rootfs points outside `filesDir` (e.g., to `/storage/emulated/0`), `canonicalPath` returns the *target* path, the boundary check fails, and the method returns early — leaving the symlink itself undeleted. Use `absolutePath` instead so the check operates on the link's own path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // Path boundary check: refuse to delete anything outside filesDir. | ||
| // This is a secondary safeguard against accidental data loss (#67, #63). | ||
| try { | ||
| if (!file.canonicalPath.startsWith(filesDir)) { |
There was a problem hiding this comment.
P1: canonicalPath resolves symlinks, which prevents the symlink guard below from ever being reached. If a symlink inside the rootfs points outside filesDir (e.g., to /storage/emulated/0), canonicalPath returns the target path, the boundary check fails, and the method returns early — leaving the symlink itself undeleted. Use absolutePath instead so the check operates on the link's own path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/BootstrapManager.kt, line 800:
<comment>`canonicalPath` resolves symlinks, which prevents the symlink guard below from ever being reached. If a symlink inside the rootfs points outside `filesDir` (e.g., to `/storage/emulated/0`), `canonicalPath` returns the *target* path, the boundary check fails, and the method returns early — leaving the symlink itself undeleted. Use `absolutePath` instead so the check operates on the link's own path.</comment>
<file context>
@@ -793,6 +793,17 @@ class BootstrapManager(
+ // Path boundary check: refuse to delete anything outside filesDir.
+ // This is a secondary safeguard against accidental data loss (#67, #63).
+ try {
+ if (!file.canonicalPath.startsWith(filesDir)) {
+ return
+ }
</file context>
Suggested change
| if (!file.canonicalPath.startsWith(filesDir)) { | |
| if (!file.absolutePath.startsWith(filesDir + File.separator) && file.absolutePath != filesDir) { |
| // Path boundary check: refuse to delete anything outside filesDir. | ||
| // This is a secondary safeguard against accidental data loss (#67, #63). | ||
| try { | ||
| if (!file.canonicalPath.startsWith(filesDir)) { |
There was a problem hiding this comment.
P2: Path prefix check without a trailing separator: startsWith(filesDir) would also match sibling directories like files-extra/. Append File.separator to the prefix to ensure the match is on a directory boundary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/BootstrapManager.kt, line 800:
<comment>Path prefix check without a trailing separator: `startsWith(filesDir)` would also match sibling directories like `files-extra/`. Append `File.separator` to the prefix to ensure the match is on a directory boundary.</comment>
<file context>
@@ -793,6 +793,17 @@ class BootstrapManager(
+ // Path boundary check: refuse to delete anything outside filesDir.
+ // This is a secondary safeguard against accidental data loss (#67, #63).
+ try {
+ if (!file.canonicalPath.startsWith(filesDir)) {
+ return
+ }
</file context>
Suggested change
| if (!file.canonicalPath.startsWith(filesDir)) { | |
| if (!file.absolutePath.startsWith(filesDir + File.separator) && file.absolutePath != filesDir) { |
Kotlin (GatewayService.kt): - Add synchronized lock to prevent concurrent startGateway() calls - Add volatile `stopping` flag to prevent restart after explicit stop - Guard all restart/error paths with stopping check - Track processStartTime separately for accurate uptime calculation - Cap restart delay at 16s to avoid excessive waits - Wrap destroyForcibly() in try-catch for safe cleanup Dart (gateway_service.dart): - Replace Future.delayed with cancellable Timer for initial health check delay — fixes orphaned timer firing after stop() - Add _initialDelayTimer tracked and cancelled via _cancelAllTimers() - Add _startInProgress guard to prevent concurrent start() calls - Increase initial health check delay from 15s to 30s (proot is slow) - Extend startup grace period from 60s to 120s - Set _startingAt in init() reconnect path too - Clear _startingAt on stop() Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="flutter_app/lib/services/gateway_service.dart">
<violation number="1" location="flutter_app/lib/services/gateway_service.dart:184">
P1: If `prefs.init()` or `_updateState(...)` throws before the `try` block is entered, `_startInProgress` is never reset to `false`, permanently bricking `start()`. Move the `try`/`finally` to wrap all code after the flag is set.</violation>
</file>
<file name="flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/GatewayService.kt">
<violation number="1" location="flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/GatewayService.kt:103">
P2: `isPortInUse()` performs a blocking socket connect (1 s timeout) while holding `lock`. This can stall `stopGateway()` for up to 1 second because it also acquires the same lock. Move the port check before the synchronized block (or capture the result in a local and check it inside the lock) so the lock is only held briefly for state mutation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| Future<void> start() async { | ||
| // Prevent concurrent start() calls from racing | ||
| if (_startInProgress) return; | ||
| _startInProgress = true; |
There was a problem hiding this comment.
P1: If prefs.init() or _updateState(...) throws before the try block is entered, _startInProgress is never reset to false, permanently bricking start(). Move the try/finally to wrap all code after the flag is set.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flutter_app/lib/services/gateway_service.dart, line 184:
<comment>If `prefs.init()` or `_updateState(...)` throws before the `try` block is entered, `_startInProgress` is never reset to `false`, permanently bricking `start()`. Move the `try`/`finally` to wrap all code after the flag is set.</comment>
<file context>
@@ -176,6 +179,10 @@ fs.writeFileSync(p, JSON.stringify(c, null, 2));
Future<void> start() async {
+ // Prevent concurrent start() calls from racing
+ if (_startInProgress) return;
+ _startInProgress = true;
+
final prefs = PreferencesService();
</file context>
flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/GatewayService.kt
Outdated
Show resolved
Hide resolved
…flag Root cause: isGatewayRunning() only checked a companion object boolean that stayed true even after the proot process died silently. The Dart health check saw isRunning=true during the grace period, then timed out at 120s declaring "Gateway process not running" with no diagnostic info. Changes: - Replace GatewayService.isRunning check with isProcessAlive() that actually calls Process.isAlive on the proot process reference - Falls back to port connectivity check if process ref is null - During first 30s of startup (before process spawns), reports alive - Add watchdog thread that monitors process liveness every 15s (starts after 45s initial delay to avoid false positives) - Watchdog emits warnings if process dies or port stops responding - Add startup phase logging: "Setting up environment...", "Spawning proot process...", "Gateway process spawned" so silent deaths are now diagnosable from the log gap - Clean up watchdog thread on stop/destroy Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/GatewayService.kt">
<violation number="1" location="flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/GatewayService.kt:310">
P1: Watchdog detects a dead process but never sets `isRunning = false`, defeating its stated purpose. If the `waitFor()` thread is stuck, the stale `isRunning = true` flag will mislead all callers.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (proc != null && !proc.isAlive) { | ||
| // Process died — the waitFor() thread should handle restart, | ||
| // but update the flag in case it's stuck | ||
| emitLog("[WARN] Watchdog: gateway process not alive") |
There was a problem hiding this comment.
P1: Watchdog detects a dead process but never sets isRunning = false, defeating its stated purpose. If the waitFor() thread is stuck, the stale isRunning = true flag will mislead all callers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/GatewayService.kt, line 310:
<comment>Watchdog detects a dead process but never sets `isRunning = false`, defeating its stated purpose. If the `waitFor()` thread is stuck, the stale `isRunning = true` flag will mislead all callers.</comment>
<file context>
@@ -261,6 +293,33 @@ class GatewayService : Service() {
+ if (proc != null && !proc.isAlive) {
+ // Process died — the waitFor() thread should handle restart,
+ // but update the flag in case it's stuck
+ emitLog("[WARN] Watchdog: gateway process not alive")
+ break
+ }
</file context>
Suggested change
| emitLog("[WARN] Watchdog: gateway process not alive") | |
| emitLog("[WARN] Watchdog: gateway process not alive") | |
| isRunning = false |
…read
Root cause of silent gateway: all emitLog() calls ran on background
threads (setup thread, stdout/stderr readers, watchdog). Flutter's
EventChannel.EventSink.success() silently drops messages when called
off the main/UI thread. This made it impossible to diagnose crashes
since no Kotlin-side logs reached Dart.
Changes:
- Add Handler(Looper.getMainLooper()) and post all emitLog() calls
to the main thread via mainHandler.post {}
- Remove blocking Socket.connect() from isProcessAlive() — it ran
on the main thread via method channel and could cause ANR. Now
only checks Process.isAlive and startup window (45s)
- Add "Directories ready" log after successful setupDirectories()
Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
…g up The previous 45s startup window was too short — proot setup (dirs, resolv.conf) can take longer on slow devices. isProcessAlive() now checks if the gateway thread is still alive (setup ongoing) and reports true in that case, matching main branch behavior where isRunning stayed true throughout the entire setup phase. Also moves isPortInUse() socket check off the main thread into the gateway thread to avoid blocking onStartCommand() for up to 1s. Co-Authored-By: Mithun Gowda B <mithungowda.b7411@gmail.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/GatewayService.kt">
<violation number="1" location="flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/GatewayService.kt:73">
P2: `gatewayThread` is never interrupted in `stopGateway()` or `onDestroy()`, unlike `uptimeThread` and `watchdogThread`. If the thread is sleeping during auto-restart backoff (up to 16s), it will linger after the service is destroyed. Add interrupt + null following the same pattern used for the other threads.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| private var processStartTime: Long = 0 | ||
| private var uptimeThread: Thread? = null | ||
| private var watchdogThread: Thread? = null | ||
| private var gatewayThread: Thread? = null |
There was a problem hiding this comment.
P2: gatewayThread is never interrupted in stopGateway() or onDestroy(), unlike uptimeThread and watchdogThread. If the thread is sleeping during auto-restart backoff (up to 16s), it will linger after the service is destroyed. Add interrupt + null following the same pattern used for the other threads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flutter_app/android/app/src/main/kotlin/com/nxg/openclawproot/GatewayService.kt, line 73:
<comment>`gatewayThread` is never interrupted in `stopGateway()` or `onDestroy()`, unlike `uptimeThread` and `watchdogThread`. If the thread is sleeping during auto-restart backoff (up to 16s), it will linger after the service is destroyed. Add interrupt + null following the same pattern used for the other threads.</comment>
<file context>
@@ -66,6 +70,7 @@ class GatewayService : Service() {
private var processStartTime: Long = 0
private var uptimeThread: Thread? = null
private var watchdogThread: Thread? = null
+ private var gatewayThread: Thread? = null
private val lock = Object()
@Volatile private var stopping = false
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses 12 issues with critical fixes for storage safety, gateway crash loop, and multiple UX improvements.
P0: Storage Safety (Closes #67, Closes #63)
deleteRecursively()— refuses to delete anything outsidefilesDirP1: Gateway Crash Loop Fix (Closes #60)
emitLog()posting to main thread —EventSink.success()was silently dropping all Kotlin-side logs when called from background threadsisRunningboolean withisProcessAlive()that checks actualProcess.isAlivesynchronizedlock to prevent concurrentstartGateway()race conditionsstoppingflag to prevent restart after explicit stopisPortInUse()off main thread to prevent ANRmaxRestartsfrom 3 to 5, reset counter after 60s+ uptimeFuture.delayed), 30s initial delay, 120s grace period,_startInProgressguardP1: Auth Token UX (Closes #64)
P1: Canvas Capability (Closes #65)
P2: Missing Dependencies (Closes #66)
curlandwgetto bootstrapapt-get installpackage listPreviously included in this branch
Files Changed
MainActivity.ktisProcessAlive()BootstrapManager.ktdeleteRecursively()GatewayService.ktgateway_service.dartdashboard_screen.dartnode_screen.dartsettings_screen.dartbootstrap_service.dartconstants.dartTest Plan
deleteRecursivelyrefuses paths outside filesDir