feat(track): add terminal tracking via PPID resolution#200
Conversation
Capture parent process ID in shell hooks and resolve to terminal emulator name (iTerm2, Terminal.app, tmux, etc.) in daemon service. - Add PPID field to Command and TrackingData structs - Add Terminal field to TrackingMetaData - Add --ppid flag to track command - Create terminal_resolver.go for process tree walking - Integrate resolver in daemon sync handler 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @AnnatarHe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the context captured for tracked commands by adding terminal application and multiplexer detection. By leveraging the parent process ID, the system can now identify the environment where commands are run, providing richer insights into user activity. This change involves updates across the CLI, daemon, and data models to seamlessly integrate this new tracking capability. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to track the terminal application used for shell commands. This is achieved by capturing the shell's parent process ID (PPID) and resolving it to a terminal name in the daemon.
The changes look good overall. The core logic for resolving the terminal by walking the process tree is implemented in the new daemon/terminal_resolver.go file. I've made a few suggestions to improve this implementation:
- Improve performance and robustness on Linux by reading from the
/procfilesystem directly instead of shelling out to external commands likecat. - Improve the robustness of terminal resolution in the daemon by searching for the first valid PPID in a batch of commands, rather than only checking the first one.
- A minor refactoring suggestion to improve code readability.
Once these points are addressed, the PR should be in great shape.
| out, err := exec.Command("cat", "/proc/"+strconv.Itoa(pid)+"/stat").Output() | ||
| if err != nil { | ||
| return 0 | ||
| } | ||
| // /proc/pid/stat format: pid (comm) state ppid ... | ||
| // Find the closing ) and get the 4th field after it | ||
| data := string(out) | ||
| idx := strings.LastIndex(data, ")") | ||
| if idx < 0 { | ||
| return 0 | ||
| } | ||
| fields := strings.Fields(data[idx+1:]) | ||
| if len(fields) < 2 { | ||
| return 0 | ||
| } | ||
| ppid, _ := strconv.Atoi(fields[1]) | ||
| return ppid |
There was a problem hiding this comment.
There are two issues here:
- For better performance, it's preferable to read
/proc/<pid>/statdirectly usingos.ReadFileinstead of shelling out tocat. - The error from
strconv.Atoiis ignored. This could lead to incorrect behavior if the parsed value is not a valid integer, although unlikely for/proc/pid/stat. It's best practice to handle this error.
out, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat")
if err != nil {
return 0
}
// /proc/pid/stat format: pid (comm) state ppid ...
// Find the closing ) and get the 4th field after it
data := string(out)
idx := strings.LastIndex(data, ")")
if idx < 0 {
return 0
}
fields := strings.Fields(data[idx+1:])
if len(fields) < 2 {
return 0
}
ppid, err := strconv.Atoi(fields[1])
if err != nil {
return 0
}
return ppid| if len(syncMsg.Data) > 0 && syncMsg.Data[0].PPID > 0 { | ||
| terminal := ResolveTerminal(syncMsg.Data[0].PPID) | ||
| syncMsg.Meta.Terminal = terminal | ||
| slog.Debug("Resolved terminal", slog.String("terminal", terminal), slog.Int("ppid", syncMsg.Data[0].PPID)) | ||
| } |
There was a problem hiding this comment.
The current implementation only checks the PPID of the first data item (syncMsg.Data[0]). If that item happens to have a PPID of 0, but subsequent items have a valid PPID, the terminal will not be resolved. It would be more robust to iterate through the data items and use the first valid PPID found.
var ppid int
for _, d := range syncMsg.Data {
if d.PPID > 0 {
ppid = d.PPID
break
}
}
if ppid > 0 {
terminal := ResolveTerminal(ppid)
syncMsg.Meta.Terminal = terminal
slog.Debug("Resolved terminal", slog.String("terminal", terminal), slog.Int("ppid", ppid))
}
daemon/terminal_resolver.go
Outdated
| // Build result string | ||
| if terminal == "" && multiplexer == "" { | ||
| return "unknown" | ||
| } | ||
|
|
||
| if terminal != "" && multiplexer != "" { | ||
| return terminal + " -> " + multiplexer | ||
| } | ||
|
|
||
| if terminal != "" { | ||
| return terminal | ||
| } | ||
|
|
||
| return multiplexer | ||
| } |
There was a problem hiding this comment.
This block of if statements for building the result string can be simplified using a switch statement, which can make the logic clearer and more concise.
// Build result string
switch {
case terminal != "" && multiplexer != "":
return terminal + " -> " + multiplexer
case terminal != "":
return terminal
case multiplexer != "":
return multiplexer
default:
return "unknown"
}
}|
|
||
| case "linux": | ||
| // Linux: /proc/<pid>/comm | ||
| out, err := exec.Command("cat", "/proc/"+strconv.Itoa(pid)+"/comm").Output() |
There was a problem hiding this comment.
For better performance, consider reading from the /proc filesystem directly using os.ReadFile instead of shelling out to cat. This avoids the overhead of creating a new process.
out, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/comm")Separate the combined "terminal -> multiplexer" format into two distinct fields for cleaner server-side storage and querying. - Change ResolveTerminal to return (terminal, multiplexer) tuple - Add Multiplexer field to TrackingMetaData struct - Update sync handler to populate both fields 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
iTerm2 -> tmuxChanges
PPIDfield toCommandandTrackingDatastructsTerminalfield toTrackingMetaData--ppidflag toshelltime trackcommanddaemon/terminal_resolver.gofor process tree walkingRelated Changes (other repos)
--ppid=$PPIDTest plan
iTerm2 -> tmux)🤖 Generated with Claude Code