feat: add 'mobilecli device logs' support for streaming logs from mobile devices#195
Open
feat: add 'mobilecli device logs' support for streaming logs from mobile devices#195
Conversation
Adds real-time log streaming from iOS simulators (via xcrun simctl log stream) and real iOS devices (via go-ios syslog_relay). Android and remote devices return "not yet supported". Supports --limit flag to stop after N entries.
Filters are applied before --limit, so the user sees exactly N matching entries. --process does substring matching on the process name.
WalkthroughAdds end-to-end device log streaming. Introduces a 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
commands/logs.go (1)
31-33: PID filter condition may have unintended behavior with PID 0.The condition
req.PID >= 0means PID 0 is treated as a valid filter value. If a user passes--pid 0, it would filter for processes with PID 0 (typically the kernel scheduler). This might be intentional, but consider whether-1should be the only "unset" sentinel, or if you want to use a pointer type (*int) to distinguish "not set" from "filter for PID 0".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/logs.go` around lines 31 - 33, The PID-check treats 0 as a valid filter because it uses req.PID >= 0; change the filter to distinguish "unset" from a real PID by making req.PID a pointer (*int) or by using -1 as the explicit unset sentinel; then update the check in the logs filtering code to something like "if req.PID != nil && entry.PID != *req.PID { return true }" (or if using sentinel: "if req.PID != -1 && entry.PID != req.PID { return true }") and adjust flag/constructors that populate req.PID accordingly so callers can actually signal "no PID filter".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commands/logs.go`:
- Around line 35-37: The current handler silently swallows JSON encoding
failures by returning false on encoder.Encode(entry) and later returning
NewSuccessResponse("done"); modify the error path so encode failures are
propagated instead of masked: when encoder.Encode(entry) returns an error,
return a failure value (e.g., propagate the error to the caller or return a
NewErrorResponse with err.Error()) rather than returning false, and ensure the
function no longer returns NewSuccessResponse("done") when an encode error
occurred. Update the code around encoder.Encode(entry) to surface the actual err
and stop further processing, and adjust any callers of this function to handle
the propagated error if needed.
In `@devices/ios.go`:
- Around line 1667-1717: StreamLogs currently calls d.getEnhancedDevice()
directly which can fail on iOS 17+ if the tunnel isn't started; mirror other
methods (Reboot, LaunchApp, TerminateApp) by calling d.startTunnel() first and
returning any error. Modify StreamLogs to invoke d.startTunnel(), check and
return its error, then proceed to call d.getEnhancedDevice() and the rest of the
existing logic; ensure you reference the IOSDevice methods startTunnel and
getEnhancedDevice and preserve existing defer conn.Close() behavior.
---
Nitpick comments:
In `@commands/logs.go`:
- Around line 31-33: The PID-check treats 0 as a valid filter because it uses
req.PID >= 0; change the filter to distinguish "unset" from a real PID by making
req.PID a pointer (*int) or by using -1 as the explicit unset sentinel; then
update the check in the logs filtering code to something like "if req.PID != nil
&& entry.PID != *req.PID { return true }" (or if using sentinel: "if req.PID !=
-1 && entry.PID != req.PID { return true }") and adjust flag/constructors that
populate req.PID accordingly so callers can actually signal "no PID filter".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c7acb32-e5d2-4071-b9d8-7d12c6621eaf
📒 Files selected for processing (7)
cli/logs.gocommands/logs.godevices/android.godevices/common.godevices/ios.godevices/remote.godevices/simulator.go
Comment on lines
+1667
to
+1717
| func (d *IOSDevice) StreamLogs(onLog func(LogEntry) bool) error { | ||
| device, err := d.getEnhancedDevice() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get device: %w", err) | ||
| } | ||
|
|
||
| conn, err := syslog.New(device) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to connect to syslog: %w", err) | ||
| } | ||
| defer conn.Close() | ||
|
|
||
| parse := syslog.Parser() | ||
|
|
||
| for { | ||
| msg, err := conn.ReadLogMessage() | ||
| if err != nil { | ||
| if errors.Is(err, io.EOF) { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("syslog read error: %w", err) | ||
| } | ||
|
|
||
| msg = strings.TrimSuffix(msg, "\x00") | ||
| msg = strings.TrimSuffix(msg, "\x0A") | ||
| if msg == "" { | ||
| continue | ||
| } | ||
|
|
||
| entry, err := parse(msg) | ||
| if err != nil { | ||
| // unparseable line — emit raw message | ||
| if !onLog(LogEntry{ | ||
| Message: msg, | ||
| }) { | ||
| return nil | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| if !onLog(LogEntry{ | ||
| Timestamp: entry.Timestamp, | ||
| Message: entry.Message, | ||
| Level: entry.Level, | ||
| Process: entry.Process, | ||
| PID: atoiOrZero(entry.PID), | ||
| }) { | ||
| return nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider calling startTunnel() before getEnhancedDevice() for iOS 17+ devices.
Other methods that call getEnhancedDevice() (e.g., Reboot, LaunchApp, TerminateApp) first call d.startTunnel() to ensure the tunnel is running for iOS 17+ devices. This method skips that step, which could cause failures on iOS 17+ if no tunnel is established.
Proposed fix
func (d *IOSDevice) StreamLogs(onLog func(LogEntry) bool) error {
+ err := d.startTunnel()
+ if err != nil {
+ return fmt.Errorf("failed to start tunnel: %w", err)
+ }
+
device, err := d.getEnhancedDevice()
if err != nil {
return fmt.Errorf("failed to get device: %w", err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/ios.go` around lines 1667 - 1717, StreamLogs currently calls
d.getEnhancedDevice() directly which can fail on iOS 17+ if the tunnel isn't
started; mirror other methods (Reboot, LaunchApp, TerminateApp) by calling
d.startTunnel() first and returning any error. Modify StreamLogs to invoke
d.startTunnel(), check and return its error, then proceed to call
d.getEnhancedDevice() and the rest of the existing logic; ensure you reference
the IOSDevice methods startTunnel and getEnhancedDevice and preserve existing
defer conn.Close() behavior.
Streams logs via adb logcat with threadtime,year format. Parses using existing logcatLineRegex. Resolves PID to package name via one-time adb shell ps call for --process filtering. Adds tag field to LogEntry (populated for Android, omitted for iOS).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
commands/logs.go (1)
34-36:⚠️ Potential issue | 🟠 MajorPropagate JSON encode failures instead of returning success.
At Line 34, encode errors stop the callback, but the error is discarded; then Line 47 can still return success. This masks partial stream failures.
Suggested fix
encoder := json.NewEncoder(os.Stdout) count := 0 + var encodeErr error err = device.StreamLogs(func(entry devices.LogEntry) bool { if req.Process != "" && entry.Process != req.Process { return true } if req.PID >= 0 && entry.PID != req.PID { return true } if err := encoder.Encode(entry); err != nil { + encodeErr = err return false } count++ if req.Limit > 0 && count >= req.Limit { return false } return true }) if err != nil { return NewErrorResponse(fmt.Errorf("error streaming logs: %w", err)) } + if encodeErr != nil { + return NewErrorResponse(fmt.Errorf("error encoding log entry: %w", encodeErr)) + } return NewSuccessResponse("done")Also applies to: 43-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/logs.go` around lines 34 - 36, The JSON encode failures are being swallowed where encoder.Encode(entry) is called; change the callback to propagate the error instead of returning a bool success sentinel: when encoder.Encode(entry) (and the similar block at 43-47) returns an error, return that error (or set a named err variable and break) so the caller can detect and surface the failure, and update the outer caller/return path to return the error rather than always returning success; reference the encoder.Encode(entry) calls and the surrounding callback/handler to locate and update the code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@commands/logs.go`:
- Around line 34-36: The JSON encode failures are being swallowed where
encoder.Encode(entry) is called; change the callback to propagate the error
instead of returning a bool success sentinel: when encoder.Encode(entry) (and
the similar block at 43-47) returns an error, return that error (or set a named
err variable and break) so the caller can detect and surface the failure, and
update the outer caller/return path to return the error rather than always
returning success; reference the encoder.Encode(entry) calls and the surrounding
callback/handler to locate and update the code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bcf44939-e9f5-4879-b35b-2ccb292cf83b
📒 Files selected for processing (3)
commands/logs.godevices/android.godevices/common.go
🚧 Files skipped from review as they are similar to previous changes (2)
- devices/common.go
- devices/android.go
- Fix --process flag description to say "exact match" - Split callback into matchesFilter and emit helpers - Extract processNameFromPath and atoiOrZero to common.go
Supports key=value (include) and key!=value (exclude) syntax. Multiple filters are ANDed. Valid keys: pid, process, tag, level, subsystem, category, message. Invalid keys return a clear error.
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 `@devices/simulator.go`:
- Around line 1072-1116: The code currently treats any json.Decoder errors as
normal end-of-stream and kills the child without reaping it; update the
log-stream handling so that failures from decoder.Token() or decoder.Decode()
that are not io.EOF are returned as errors and the child is reaped: after
calling cmd.Process.Kill() invoke cmd.Wait() and include its error in the
returned fmt.Errorf; when decoder.Decode() returns io.EOF treat it as normal
end-of-stream, but for other errors return them (again ensuring cmd.Wait() is
called and its error combined); finally, don't ignore the result of cmd.Wait()
at the end—return any non-nil wait error. Add the io import and reference
functions/values like cmd.Start, decoder.Token, decoder.Decode,
cmd.Process.Kill, cmd.Wait, simctlLogEntry, and onLog when making these fixes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d003d51-ab06-4dfe-8267-12fe09f41d2e
📒 Files selected for processing (6)
README.mdcli/logs.gocommands/logs.godevices/common.godevices/ios.godevices/simulator.go
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/logs.go
Comment on lines
+1072
to
+1116
| if err := cmd.Start(); err != nil { | ||
| return fmt.Errorf("failed to start log stream: %w", err) | ||
| } | ||
|
|
||
| decoder := json.NewDecoder(stdout) | ||
|
|
||
| // read opening '[' of the JSON array | ||
| token, err := decoder.Token() | ||
| if err != nil { | ||
| _ = cmd.Process.Kill() | ||
| return fmt.Errorf("failed to read opening token: %w", err) | ||
| } | ||
| if delim, ok := token.(json.Delim); !ok || delim != '[' { | ||
| _ = cmd.Process.Kill() | ||
| return fmt.Errorf("expected '[', got %v", token) | ||
| } | ||
|
|
||
| // decode entries one at a time until the stream ends | ||
| for decoder.More() { | ||
| var raw simctlLogEntry | ||
| if err := decoder.Decode(&raw); err != nil { | ||
| // stream ended (process killed) — not an error | ||
| break | ||
| } | ||
|
|
||
| processName := processNameFromPath(raw.ProcessImagePath) | ||
|
|
||
| if !onLog(LogEntry{ | ||
| Timestamp: raw.Timestamp, | ||
| Message: raw.EventMessage, | ||
| Level: raw.MessageType, | ||
| Subsystem: raw.Subsystem, | ||
| Category: raw.Category, | ||
| PID: raw.ProcessID, | ||
| Process: processName, | ||
| EventType: raw.EventType, | ||
| }) { | ||
| _ = cmd.Process.Kill() | ||
| break | ||
| } | ||
| } | ||
|
|
||
| // we killed it ourselves, or stream ended naturally | ||
| _ = cmd.Wait() | ||
| return nil |
There was a problem hiding this comment.
Do not turn xcrun failures into a clean end-of-stream.
After cmd.Start(), the token-read error paths kill the child but never reap it, and the loop treats every decoder.Decode failure as a normal stop. Combined with the ignored cmd.Wait() result, malformed JSON or a non-zero xcrun exit gets reported as success.
Proposed fix
if err := cmd.Start(); err != nil {
return fmt.Errorf("failed to start log stream: %w", err)
}
decoder := json.NewDecoder(stdout)
+ stoppedByCaller := false
// read opening '[' of the JSON array
token, err := decoder.Token()
if err != nil {
_ = cmd.Process.Kill()
+ _ = cmd.Wait()
return fmt.Errorf("failed to read opening token: %w", err)
}
if delim, ok := token.(json.Delim); !ok || delim != '[' {
_ = cmd.Process.Kill()
+ _ = cmd.Wait()
return fmt.Errorf("expected '[', got %v", token)
}
// decode entries one at a time until the stream ends
for decoder.More() {
var raw simctlLogEntry
if err := decoder.Decode(&raw); err != nil {
- // stream ended (process killed) — not an error
- break
+ if err == io.EOF {
+ break
+ }
+ _ = cmd.Process.Kill()
+ _ = cmd.Wait()
+ return fmt.Errorf("failed to decode log entry: %w", err)
}
processName := processNameFromPath(raw.ProcessImagePath)
if !onLog(LogEntry{
@@
EventType: raw.EventType,
}) {
+ stoppedByCaller = true
_ = cmd.Process.Kill()
break
}
}
- // we killed it ourselves, or stream ended naturally
- _ = cmd.Wait()
+ if err := cmd.Wait(); err != nil && !stoppedByCaller {
+ return fmt.Errorf("log stream exited unexpectedly: %w", err)
+ }
return nilAlso add the io import.
📝 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 err := cmd.Start(); err != nil { | |
| return fmt.Errorf("failed to start log stream: %w", err) | |
| } | |
| decoder := json.NewDecoder(stdout) | |
| // read opening '[' of the JSON array | |
| token, err := decoder.Token() | |
| if err != nil { | |
| _ = cmd.Process.Kill() | |
| return fmt.Errorf("failed to read opening token: %w", err) | |
| } | |
| if delim, ok := token.(json.Delim); !ok || delim != '[' { | |
| _ = cmd.Process.Kill() | |
| return fmt.Errorf("expected '[', got %v", token) | |
| } | |
| // decode entries one at a time until the stream ends | |
| for decoder.More() { | |
| var raw simctlLogEntry | |
| if err := decoder.Decode(&raw); err != nil { | |
| // stream ended (process killed) — not an error | |
| break | |
| } | |
| processName := processNameFromPath(raw.ProcessImagePath) | |
| if !onLog(LogEntry{ | |
| Timestamp: raw.Timestamp, | |
| Message: raw.EventMessage, | |
| Level: raw.MessageType, | |
| Subsystem: raw.Subsystem, | |
| Category: raw.Category, | |
| PID: raw.ProcessID, | |
| Process: processName, | |
| EventType: raw.EventType, | |
| }) { | |
| _ = cmd.Process.Kill() | |
| break | |
| } | |
| } | |
| // we killed it ourselves, or stream ended naturally | |
| _ = cmd.Wait() | |
| return nil | |
| if err := cmd.Start(); err != nil { | |
| return fmt.Errorf("failed to start log stream: %w", err) | |
| } | |
| decoder := json.NewDecoder(stdout) | |
| stoppedByCaller := false | |
| // read opening '[' of the JSON array | |
| token, err := decoder.Token() | |
| if err != nil { | |
| _ = cmd.Process.Kill() | |
| _ = cmd.Wait() | |
| return fmt.Errorf("failed to read opening token: %w", err) | |
| } | |
| if delim, ok := token.(json.Delim); !ok || delim != '[' { | |
| _ = cmd.Process.Kill() | |
| _ = cmd.Wait() | |
| return fmt.Errorf("expected '[', got %v", token) | |
| } | |
| // decode entries one at a time until the stream ends | |
| for decoder.More() { | |
| var raw simctlLogEntry | |
| if err := decoder.Decode(&raw); err != nil { | |
| if err == io.EOF { | |
| break | |
| } | |
| _ = cmd.Process.Kill() | |
| _ = cmd.Wait() | |
| return fmt.Errorf("failed to decode log entry: %w", err) | |
| } | |
| processName := processNameFromPath(raw.ProcessImagePath) | |
| if !onLog(LogEntry{ | |
| Timestamp: raw.Timestamp, | |
| Message: raw.EventMessage, | |
| Level: raw.MessageType, | |
| Subsystem: raw.Subsystem, | |
| Category: raw.Category, | |
| PID: raw.ProcessID, | |
| Process: processName, | |
| EventType: raw.EventType, | |
| }) { | |
| stoppedByCaller = true | |
| _ = cmd.Process.Kill() | |
| break | |
| } | |
| } | |
| if err := cmd.Wait(); err != nil && !stoppedByCaller { | |
| return fmt.Errorf("log stream exited unexpectedly: %w", err) | |
| } | |
| return nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/simulator.go` around lines 1072 - 1116, The code currently treats any
json.Decoder errors as normal end-of-stream and kills the child without reaping
it; update the log-stream handling so that failures from decoder.Token() or
decoder.Decode() that are not io.EOF are returned as errors and the child is
reaped: after calling cmd.Process.Kill() invoke cmd.Wait() and include its error
in the returned fmt.Errorf; when decoder.Decode() returns io.EOF treat it as
normal end-of-stream, but for other errors return them (again ensuring
cmd.Wait() is called and its error combined); finally, don't ignore the result
of cmd.Wait() at the end—return any non-nil wait error. Add the io import and
reference functions/values like cmd.Start, decoder.Token, decoder.Decode,
cmd.Process.Kill, cmd.Wait, simctlLogEntry, and onLog when making these fixes.
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.
No description provided.