refactor(tmux): move session logger to system services - #1185
Conversation
Replace the tmux nohup bootstrap with a dedicated Home Manager service for launchd and systemd. Convert the logger script to a single-pass command so the service scheduler owns the 30 second cadence. Harden fish-test so shell-test can use an existing store-installed fishtape runner with isolated Fish state when fishtape is not on PATH. This keeps the shell test target working in sandboxed environments where nix develop cannot reach the daemon. Entire-Checkpoint: 6f13ed11d582
📝 WalkthroughWalkthroughThis pull request refactors the tmux session logger from being managed internally by tmux to external launchd/systemd management. It updates the Makefile's fish-test target for better fishtape detection, modifies the session-logger.sh script to run once per invocation with improved efficiency, removes the internal tmux launcher, and adds a new Home Manager service module supporting both Darwin and Linux platforms. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 150, 200, 0.5)
Note over tmux: Previous Flow (Internal Management)
tmux->>tmux: On startup: run-shell
tmux->>session-logger.sh: Launch process
loop Every 30s
session-logger.sh->>tmux: Query sessions/panes
session-logger.sh->>session-logger.sh: Log & rotate
end
end
rect rgba(150, 200, 100, 0.5)
Note over launchd,systemd: New Flow (External Management)
launchd/systemd->>launchd/systemd: Timer triggers (boot + every 30s)
launchd/systemd->>session-logger.sh: Execute once
session-logger.sh->>tmux: Query sessions/panes
session-logger.sh->>session-logger.sh: Log & rotate
session-logger.sh-->>launchd/systemd: Complete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, 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 refactors the tmux session history logging mechanism by migrating it from a tmux-managed process to system-level services (launchd for macOS and systemd for Linux). This change aims to improve the reliability and predictability of the logger's scheduling. Additionally, the PR enhances the 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. Footnotes
|
Mesa DescriptionTL;DRRefactor tmux session history logging into Home Manager-managed What changed?
Description generated by Mesa. Update settings |
![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 is a great refactoring that moves the tmux session logger from a process managed by tmux itself to a more robust system service managed by launchd or systemd. This improves separation of concerns and reliability. The related hardening of the fish-test target in the Makefile is also a solid improvement for the test environment. My review includes one suggestion to improve the robustness of the session logger script to correctly handle tmux session names that contain spaces.
| tmux list-panes -a -F "#{session_name} #{window_index} #{pane_index} #{pane_id}" \ | ||
| 2>/dev/null | while IFS= read -r line; do | ||
| sess=$(printf '%s' "$line" | cut -d' ' -f1) | ||
| widx=$(printf '%s' "$line" | cut -d' ' -f2) | ||
| pidx=$(printf '%s' "$line" | cut -d' ' -f3) | ||
| pane_id=$(printf '%s' "$line" | cut -d' ' -f4) | ||
| tmux capture-pane -pt "$pane_id" -S - 2>/dev/null \ | ||
| >"$PANE_DIR/$sess--$widx--$pidx.txt" | ||
| done |
There was a problem hiding this comment.
The current method of parsing the output from tmux list-panes is not robust against session names containing spaces. Using cut -d' ' will incorrectly split such names, leading to errors.
A more reliable approach is to use a delimiter that is not expected in session names (like a tab character) in the tmux format string and then parse the line using read with a matching IFS.
This also simplifies the loop by removing multiple calls to printf and cut, making it more efficient.
| tmux list-panes -a -F "#{session_name} #{window_index} #{pane_index} #{pane_id}" \ | |
| 2>/dev/null | while IFS= read -r line; do | |
| sess=$(printf '%s' "$line" | cut -d' ' -f1) | |
| widx=$(printf '%s' "$line" | cut -d' ' -f2) | |
| pidx=$(printf '%s' "$line" | cut -d' ' -f3) | |
| pane_id=$(printf '%s' "$line" | cut -d' ' -f4) | |
| tmux capture-pane -pt "$pane_id" -S - 2>/dev/null \ | |
| >"$PANE_DIR/$sess--$widx--$pidx.txt" | |
| done | |
| tmux list-panes -a -F "#{session_name}_TAB_#{window_index}_TAB_#{pane_index}_TAB_#{pane_id}" \ | |
| 2>/dev/null | while IFS='_TAB_' read -r sess widx pidx pane_id; do | |
| if [ -n "$sess" ]; then | |
| tmux capture-pane -pt "$pane_id" -S - 2>/dev/null \ | |
| >"$PANE_DIR/$sess--$widx--$pidx.txt" | |
| fi | |
| done |
There was a problem hiding this comment.
Fixed in 078690b — switched to tab delimiter in the format string and to correctly handle session names containing spaces. Also simplified by removing the multiple calls.
…sion names Switch from space-delimited `cut` parsing to tab-delimited `IFS` read to correctly handle tmux session names that contain spaces. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 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="home-manager/services/tmux-session-logger/default.nix">
<violation number="1" location="home-manager/services/tmux-session-logger/default.nix:19">
P1: Use `EnvironmentVariables` instead of `Environment` in the launchd job config; otherwise PATH is not applied to the agent process.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "${pkgs.bash}/bin/bash" | ||
| "${sessionLoggerScript}" | ||
| ]; | ||
| Environment = { |
There was a problem hiding this comment.
P1: Use EnvironmentVariables instead of Environment in the launchd job config; otherwise PATH is not applied to the agent process.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/tmux-session-logger/default.nix, line 19:
<comment>Use `EnvironmentVariables` instead of `Environment` in the launchd job config; otherwise PATH is not applied to the agent process.</comment>
<file context>
@@ -0,0 +1,54 @@
+ "${pkgs.bash}/bin/bash"
+ "${sessionLoggerScript}"
+ ];
+ Environment = {
+ PATH = "${servicePath}:/usr/bin:/bin:/usr/sbin:/sbin";
+ };
</file context>
Add systemctl-tmux-session-logger to restart the timer and include it in the top-level systemctl target. Add systemctl-tmux-session-logger-logs to inspect service status, journal entries, session history, and pane snapshots. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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 (2)
home-manager/services/tmux-session-logger/default.nix (1)
29-53: Systemd timer usesOnUnitActiveSec- verify this matches intended behavior.
OnUnitActiveSec = "30s"means the timer fires 30 seconds after the service completes, not 30 seconds from the previous start. This creates a slight drift compared to launchd'sStartIntervalwhich fires at fixed 30-second intervals.If strict 30-second intervals are needed regardless of script execution time, consider using
OnCalendarinstead:OnCalendar = "*:*:0/30"; # Every 30 seconds on the clockHowever, the current approach is often preferred as it prevents overlapping executions if the script takes longer than expected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/services/tmux-session-logger/default.nix` around lines 29 - 53, The timer currently uses OnUnitActiveSec = "30s" in systemd.user.timers.tmux-session-logger which schedules 30s after the service finishes (allowing drift); decide whether you need strict fixed 30s clock-aligned intervals and if so replace the Timer's OnUnitActiveSec entry with an OnCalendar value (e.g. an every-30-seconds expression) in systemd.user.timers.tmux-session-logger (preserving the Timer/Unit and Install blocks), otherwise keep OnUnitActiveSec to avoid overlapping runs; verify behavior after change by testing the timer activation against the sessionLoggerScript execution time.home-manager/programs/tmux/session-logger.sh (1)
24-25: Consider sanitizing session names for filenames.If a session name contains characters like
/or--, the filename could be malformed or collide with other panes. This is an edge case but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/tmux/session-logger.sh` around lines 24 - 25, Sanitize the session name before using it in the filename to avoid illegal characters or collisions: create a sanitized variable from $sess (e.g., replace slashes and whitespace with underscores, collapse or replace repeated hyphens, and remove/replace any characters not allowed in filenames) and then use that sanitized variable in the capture target (replace "$sess" with the sanitized name in the "$PANE_DIR/$sess--$widx--$pidx.txt" path); ensure you handle the empty-name case (use a fallback like "session") so filenames are always valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@home-manager/services/tmux-session-logger/default.nix`:
- Line 4: The sessionLoggerScript currently points to a source-tree path that
won't exist at runtime; update the sessionLoggerScript assignment to reference
the installed user location instead (e.g. ~/.config/tmux/session-logger.sh or
$XDG_CONFIG_HOME/tmux/session-logger.sh) so the tmux-session-logger service uses
the file installed by programs.tmux rather than
../../programs/tmux/session-logger.sh.
In `@Makefile`:
- Around line 849-857: The current fallback in the fish-test target can
recursively re-invoke fish-test if fishtape is missing; change the logic so
either (A) add a one-shot guard environment variable (e.g.
FISH_TEST_DEV_INVOKED) checked in the fish-test target before calling make
fish-test-dev and set it when invoking fish-test-dev so the second invocation
aborts with a clear error instead of re-entering, or (B) modify fish-test-dev to
call a lower-level runner target (e.g. fish-test-runner) that performs the
dev-shell setup and executes fishtape directly without calling back into
fish-test; update references to fish_runner and the fish-test / fish-test-dev
invocation accordingly.
---
Nitpick comments:
In `@home-manager/programs/tmux/session-logger.sh`:
- Around line 24-25: Sanitize the session name before using it in the filename
to avoid illegal characters or collisions: create a sanitized variable from
$sess (e.g., replace slashes and whitespace with underscores, collapse or
replace repeated hyphens, and remove/replace any characters not allowed in
filenames) and then use that sanitized variable in the capture target (replace
"$sess" with the sanitized name in the "$PANE_DIR/$sess--$widx--$pidx.txt"
path); ensure you handle the empty-name case (use a fallback like "session") so
filenames are always valid.
In `@home-manager/services/tmux-session-logger/default.nix`:
- Around line 29-53: The timer currently uses OnUnitActiveSec = "30s" in
systemd.user.timers.tmux-session-logger which schedules 30s after the service
finishes (allowing drift); decide whether you need strict fixed 30s
clock-aligned intervals and if so replace the Timer's OnUnitActiveSec entry with
an OnCalendar value (e.g. an every-30-seconds expression) in
systemd.user.timers.tmux-session-logger (preserving the Timer/Unit and Install
blocks), otherwise keep OnUnitActiveSec to avoid overlapping runs; verify
behavior after change by testing the timer activation against the
sessionLoggerScript execution time.
🪄 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: 2fba1f8c-e1b3-4c3f-84f8-bbc5e95a84c3
📒 Files selected for processing (5)
Makefilehome-manager/programs/tmux/session-logger.shhome-manager/programs/tmux/tmux.confhome-manager/services/default.nixhome-manager/services/tmux-session-logger/default.nix
| { pkgs, ... }: | ||
| let | ||
| inherit (pkgs) lib; | ||
| sessionLoggerScript = ../../programs/tmux/session-logger.sh; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's an existing mechanism that installs session-logger.sh to a known location
rg -l 'session-logger' --type nixRepository: shunkakinoki/dotfiles
Length of output: 191
🏁 Script executed:
cat -n home-manager/services/tmux-session-logger/default.nixRepository: shunkakinoki/dotfiles
Length of output: 1809
🏁 Script executed:
cat -n home-manager/programs/tmux/default.nixRepository: shunkakinoki/dotfiles
Length of output: 632
🏁 Script executed:
cat -n home-manager/services/default.nixRepository: shunkakinoki/dotfiles
Length of output: 1149
Script path will fail at runtime; use the installed home directory location instead.
The relative path ../../programs/tmux/session-logger.sh resolves to a source tree location during Nix evaluation. At runtime on a deployed system, this source path will not exist, causing the service to fail.
The script is already installed to ~/.config/tmux/session-logger.sh by the programs.tmux module. Reference that location instead:
🔧 Proposed fix
- sessionLoggerScript = ../../programs/tmux/session-logger.sh;
+ sessionLoggerScript = "$HOME/.config/tmux/session-logger.sh";Or use the XDG base directory variable:
- sessionLoggerScript = ../../programs/tmux/session-logger.sh;
+ sessionLoggerScript = "\${XDG_CONFIG_HOME:-$HOME/.config}/tmux/session-logger.sh";📝 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.
| sessionLoggerScript = ../../programs/tmux/session-logger.sh; | |
| sessionLoggerScript = "${XDG_CONFIG_HOME:-$HOME/.config}/tmux/session-logger.sh"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/services/tmux-session-logger/default.nix` at line 4, The
sessionLoggerScript currently points to a source-tree path that won't exist at
runtime; update the sessionLoggerScript assignment to reference the installed
user location instead (e.g. ~/.config/tmux/session-logger.sh or
$XDG_CONFIG_HOME/tmux/session-logger.sh) so the tmux-session-logger service uses
the file installed by programs.tmux rather than
../../programs/tmux/session-logger.sh.
| if [ -z "$$fish_runner" ]; then \ | ||
| set -- /nix/store/*-fishtape/bin/fishtape; \ | ||
| if [ -x "$$1" ]; then \ | ||
| fish_runner=$$1; \ | ||
| echo " fishtape not on PATH, using $$fish_runner"; \ | ||
| else \ | ||
| echo " fishtape not found, running inside Nix dev shell..."; \ | ||
| $(MAKE) fish-test-dev; \ | ||
| exit $$?; \ |
There was a problem hiding this comment.
Prevent recursive fallback when fishtape is missing.
If fishtape is unavailable both locally and in the dev shell, Line 856 calls fish-test-dev, and Lines 877-880 immediately re-enter fish-test again. That turns a missing dependency into recursive self-invocation instead of a single clear failure. Add a one-shot guard, or have fish-test-dev invoke a lower-level runner target directly.
🛠️ Example guard
else \
- echo " fishtape not found, running inside Nix dev shell..."; \
- $(MAKE) fish-test-dev; \
+ if [ "$$FISHTAPE_BOOTSTRAPPED" = "1" ]; then \
+ echo " fishtape not found in the Nix dev shell"; \
+ exit 1; \
+ fi; \
+ echo " fishtape not found, running inside Nix dev shell..."; \
+ FISHTAPE_BOOTSTRAPPED=1 $(MAKE) fish-test-dev; \
exit $$?; \
fi; \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 849 - 857, The current fallback in the fish-test
target can recursively re-invoke fish-test if fishtape is missing; change the
logic so either (A) add a one-shot guard environment variable (e.g.
FISH_TEST_DEV_INVOKED) checked in the fish-test target before calling make
fish-test-dev and set it when invoking fish-test-dev so the second invocation
aborts with a clear error instead of re-entering, or (B) modify fish-test-dev to
call a lower-level runner target (e.g. fish-test-runner) that performs the
dev-shell setup and executes fishtape directly without calling back into
fish-test; update references to fish_runner and the fish-test / fish-test-dev
invocation accordingly.
Add launchctl-tmux-session-logger and include it in the top-level launchctl aggregate so make switch covers the agent on both Darwin and Linux. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Moves tmux session history logging out of tmux startup and into Home Manager-managed launchd/systemd units, and adjusts the logger script to run single-pass under a scheduler. Also hardens the fish test runner selection in the Makefile.
Changes:
- Add a new
tmux-session-loggerHome Manager service (launchd agent on Darwin, systemd user service + timer on Linux). - Refactor
session-logger.shto be single-pass and fix pane parsing to handle spaces in session names. - Update
Makefilefish tests to prefer an existing fishtape runner (including store fallback), isolate Fish XDG state, and filter known notification warnings.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| home-manager/services/tmux-session-logger/default.nix | New launchd/systemd units that run the tmux session logger on a 30s cadence. |
| home-manager/services/default.nix | Registers the new tmux session logger module in the Home Manager services list. |
| home-manager/programs/tmux/tmux.conf | Removes tmux-managed nohup bootstrap and documents that services own scheduling. |
| home-manager/programs/tmux/session-logger.sh | Converts logger to one-shot execution and uses tab-delimited parsing for space-safe session names. |
| Makefile | Improves fishtape discovery/fallback, isolates Fish state, and filters known stderr warnings. |
Comments suppressed due to low confidence (1)
Makefile:862
mktempis invoked without a template forfish_errors/fish_errors_filtered. BSDmktemp(macOS default) errors without a template, which will breakmake fish-testoutside a GNU coreutils environment. Use a portable form (e.g.,mktemp "${TMPDIR:-/tmp}/fish-errors.XXXXXX"ormktemp -t fish-errors) for both temp files.
@bash -c "shellspec"
@$(MAKE) fish-test
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| # Recapture all currently live panes | ||
| tmux list-panes -a -F "#{session_name} #{window_index} #{pane_index} #{pane_id}" \ | ||
| 2>/dev/null | while IFS=' ' read -r sess widx pidx pane_id; do | ||
| if [ -n "$sess" ]; then |
Add systemctl targets for cliproxyapi-backup, make-updater, and neverssl-keepalive which were present on Darwin via launchctl but missing from the Linux systemctl aggregate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move tmux session history logging out of tmux startup and into Home Manager managed launchd/systemd units.
The old setup spawned the logger from tmux with a nohup guard, which tied process management to tmux server startup and made the scheduling behavior harder to reason about. This refactor turns the logger into a single-pass script and lets launchd or systemd own the 30 second cadence.
I also hardened the fish test fallback in the Makefile so shell-test still works when fishtape is not on PATH. It now prefers an existing store-installed fishtape runner, uses isolated writable Fish state, and filters the known notification warnings that are environmental noise in this setup.
Verified with make shell-test and Nix eval checks for the Darwin and Linux Home Manager configurations.
Summary by cubic
Refactor tmux session logging into Home Manager services and convert the logger to a single-pass script on a reliable 30s schedule. Fix pane parsing to handle spaces in session names.
Refactors
session-logger.shis single-pass and runs via bash with PATH.tmux.conf.launchctl-tmux-session-loggerandsystemctl-tmux-session-logger(included in top-levellaunchctl/systemctl). Linux aggregate now also includescliproxyapi-backup,make-updater, andneverssl-keepalive.fishtape, isolate XDG dirs, and filter notify warnings.Bug Fixes
Written for commit 620c037. Summary will update on new commits.