fix: init - #423
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughCross-platform support is added to home-manager services previously limited to macOS. The code-syncer service gains a Linux systemd unit alongside refactored Darwin launchd configuration. The sync script now detects OS type and uses appropriate configuration paths and watcher tools (fswatch for macOS, inotifywait for Linux). The dotfiles-updater PATH is expanded with additional tools. GPG cache TTLs are increased. Tests are updated for OS-aware path validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
Summary of ChangesHello @shunkakinoki, 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 improves the cross-platform compatibility and robustness of 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. 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
|
Mesa DescriptionTL;DRFixes an initialization issue. What changed?File summaries are unavailable. The PR body indicates several chore updates. 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 introduces cross-platform support for the code-syncer service, enabling it to run on both Linux and macOS. The changes include adding a systemd service for Linux, and updating the synchronization script to handle OS-specific file paths and tools. The tests have also been adapted for cross-platform execution. My review focuses on improving script maintainability and efficiency, refactoring duplicated test code, and addressing a critical security vulnerability related to an excessively long GPG agent cache TTL.
| defaultCacheTtl = 94608000; # 3 years | ||
| maxCacheTtl = 94608000; # 3 years |
There was a problem hiding this comment.
Setting the GPG agent cache TTL to 3 years (94608000 seconds) is a significant security risk. If this machine were compromised, an attacker would have access to your unlocked GPG key for an extremely long period. It is strongly recommended to use a much shorter TTL. The previous values (30 minutes and 2 hours) were much safer. If you need a longer duration for convenience, consider a value like one day (86400) instead.
defaultCacheTtl = 1800;
maxCacheTtl = 7200;
| if [ "$OS_TYPE" = "Darwin" ]; then | ||
| VSCODE_USER_DIR="$HOME/Library/Application Support/Code/User" | ||
| VSCODE_INSIDERS_USER_DIR="$HOME/Library/Application Support/Code - Insiders/User" | ||
| ANTIGRAVITY_USER_DIR="$HOME/Library/Application Support/Antigravity/User" | ||
| CURSOR_USER_DIR="$HOME/Library/Application Support/Cursor/User" | ||
| WINDSURF_USER_DIR="$HOME/Library/Application Support/Windsurf/User" | ||
| else | ||
| # Linux/Other | ||
| VSCODE_USER_DIR="$HOME/.config/Code/User" | ||
| VSCODE_INSIDERS_USER_DIR="$HOME/.config/Code - Insiders/User" | ||
| ANTIGRAVITY_USER_DIR="$HOME/.config/Antigravity/User" | ||
| CURSOR_USER_DIR="$HOME/.config/Cursor/User" | ||
| WINDSURF_USER_DIR="$HOME/.config/Windsurf/User" | ||
| fi |
There was a problem hiding this comment.
There's a lot of repetition in defining the base directories for different operating systems. You can reduce this by defining a single base path variable depending on the OS and then constructing the full paths from it. This will make the script easier to maintain if you need to add more application paths in the future.
| if [ "$OS_TYPE" = "Darwin" ]; then | |
| VSCODE_USER_DIR="$HOME/Library/Application Support/Code/User" | |
| VSCODE_INSIDERS_USER_DIR="$HOME/Library/Application Support/Code - Insiders/User" | |
| ANTIGRAVITY_USER_DIR="$HOME/Library/Application Support/Antigravity/User" | |
| CURSOR_USER_DIR="$HOME/Library/Application Support/Cursor/User" | |
| WINDSURF_USER_DIR="$HOME/Library/Application Support/Windsurf/User" | |
| else | |
| # Linux/Other | |
| VSCODE_USER_DIR="$HOME/.config/Code/User" | |
| VSCODE_INSIDERS_USER_DIR="$HOME/.config/Code - Insiders/User" | |
| ANTIGRAVITY_USER_DIR="$HOME/.config/Antigravity/User" | |
| CURSOR_USER_DIR="$HOME/.config/Cursor/User" | |
| WINDSURF_USER_DIR="$HOME/.config/Windsurf/User" | |
| fi | |
| if [ "$OS_TYPE" = "Darwin" ]; then | |
| CONFIG_BASE_DIR="$HOME/Library/Application Support" | |
| else | |
| # Linux/Other | |
| CONFIG_BASE_DIR="$HOME/.config" | |
| fi | |
| VSCODE_USER_DIR="$CONFIG_BASE_DIR/Code/User" | |
| VSCODE_INSIDERS_USER_DIR="$CONFIG_BASE_DIR/Code - Insiders/User" | |
| ANTIGRAVITY_USER_DIR="$CONFIG_BASE_DIR/Antigravity/User" | |
| CURSOR_USER_DIR="$CONFIG_BASE_DIR/Cursor/User" | |
| WINDSURF_USER_DIR="$CONFIG_BASE_DIR/Windsurf/User" |
| for bad_ext in "${PROPRIETARY_EXTENSIONS[@]}"; do | ||
| # Remove the line containing the bad extension | ||
| sed -i '' "/$bad_ext/d" "$clean_file" | ||
| $SED_INPLACE "/$bad_ext/d" "$clean_file" | ||
| done | ||
|
|
||
| for ai_ext in "${AI_EXTENSIONS[@]}"; do | ||
| # Remove the line containing the AI extension | ||
| sed -i '' "/$ai_ext/d" "$clean_file" | ||
| $SED_INPLACE "/$ai_ext/d" "$clean_file" | ||
| done |
There was a problem hiding this comment.
These two loops can be combined into one by first merging the two arrays of extensions. This improves readability and reduces code duplication. While the performance impact of multiple sed calls may be small, a single loop is cleaner. A further optimization would be to generate a single sed command to perform all deletions in one pass.
| for bad_ext in "${PROPRIETARY_EXTENSIONS[@]}"; do | |
| # Remove the line containing the bad extension | |
| sed -i '' "/$bad_ext/d" "$clean_file" | |
| $SED_INPLACE "/$bad_ext/d" "$clean_file" | |
| done | |
| for ai_ext in "${AI_EXTENSIONS[@]}"; do | |
| # Remove the line containing the AI extension | |
| sed -i '' "/$ai_ext/d" "$clean_file" | |
| $SED_INPLACE "/$ai_ext/d" "$clean_file" | |
| done | |
| local extensions_to_remove=("${PROPRIETARY_EXTENSIONS[@]}" "${AI_EXTENSIONS[@]}") | |
| for ext in "${extensions_to_remove[@]}"; do | |
| # Remove the line containing the extension | |
| $SED_INPLACE "/$ext/d" "$clean_file" | |
| done |
| if [ "$OS_TYPE" = "Darwin" ]; then | ||
| # macOS: use fswatch | ||
| if command -v fswatch >/dev/null; then | ||
| echo "Watching for changes in VS Code settings (macOS)..." | ||
| fswatch -o "$VSCODE_USER_DIR/$SETTINGS_FILE" "$VSCODE_USER_DIR/$KEYBINDINGS_FILE" | while read num; do | ||
| sync_config_file "$SETTINGS_FILE" | ||
| sync_config_file "$KEYBINDINGS_FILE" | ||
| echo "Updated at $(date)" | ||
| done | ||
| else | ||
| echo "fswatch not found. Auto-sync disabled." | ||
| fi | ||
| else | ||
| echo "fswatch not found. Auto-sync disabled." | ||
| # Linux: use inotifywait | ||
| if command -v inotifywait >/dev/null; then | ||
| echo "Watching for changes in VS Code settings (Linux)..." | ||
| inotifywait -m -e modify,create "$VSCODE_USER_DIR/$SETTINGS_FILE" "$VSCODE_USER_DIR/$KEYBINDINGS_FILE" 2>/dev/null | while read -r directory events filename; do | ||
| sync_config_file "$SETTINGS_FILE" | ||
| sync_config_file "$KEYBINDINGS_FILE" | ||
| echo "Updated at $(date)" | ||
| done | ||
| else | ||
| echo "inotifywait not found. Auto-sync disabled." | ||
| fi | ||
| fi |
There was a problem hiding this comment.
The watcher logic currently re-syncs both settings.json and keybindings.json regardless of which one actually changed. Both fswatch and inotifywait can report the specific file that triggered the event. By using this information, you can make the sync more efficient by only processing the file that was modified.
For fswatch, you can remove the -o flag to get file paths. For inotifywait, the filename is already available in the while loop.
if [ "$OS_TYPE" = "Darwin" ]; then
# macOS: use fswatch
if command -v fswatch >/dev/null; then
echo "Watching for changes in VS Code settings (macOS)..."
fswatch "$VSCODE_USER_DIR/$SETTINGS_FILE" "$VSCODE_USER_DIR/$KEYBINDINGS_FILE" | while read -r path; do
filename=$(basename "$path")
sync_config_file "$filename"
echo "Updated at $(date)"
done
else
echo "fswatch not found. Auto-sync disabled."
fi
else
# Linux: use inotifywait
if command -v inotifywait >/dev/null; then
echo "Watching for changes in VS Code settings (Linux)..."
inotifywait -m -e modify,create "$VSCODE_USER_DIR/$SETTINGS_FILE" "$VSCODE_USER_DIR/$KEYBINDINGS_FILE" 2>/dev/null | while read -r directory events filename; do
if [ -n "$filename" ]; then
sync_config_file "$filename"
echo "Updated at $(date)"
fi
done
else
echo "inotifywait not found. Auto-sync disabled."
fi
fi| setup() { | ||
| TEMP_HOME=$(mktemp -d) | ||
| mkdir -p "$TEMP_HOME/Library/Application Support/Code/User" | ||
| mkdir -p "$TEMP_HOME/Library/Application Support/Cursor/User" | ||
| mkdir -p "$TEMP_HOME/Library/Application Support/Windsurf/User" | ||
| mkdir -p "$TEMP_HOME/Library/Application Support/Antigravity/User" | ||
| mkdir -p "$TEMP_HOME/Library/Application Support/Code - Insiders/User" | ||
| OS_TYPE="$(uname -s)" | ||
| if [ "$OS_TYPE" = "Darwin" ]; then | ||
| mkdir -p "$TEMP_HOME/Library/Application Support/Code/User" | ||
| mkdir -p "$TEMP_HOME/Library/Application Support/Cursor/User" | ||
| mkdir -p "$TEMP_HOME/Library/Application Support/Windsurf/User" | ||
| mkdir -p "$TEMP_HOME/Library/Application Support/Antigravity/User" | ||
| mkdir -p "$TEMP_HOME/Library/Application Support/Code - Insiders/User" | ||
| else | ||
| mkdir -p "$TEMP_HOME/.config/Code/User" | ||
| mkdir -p "$TEMP_HOME/.config/Cursor/User" | ||
| mkdir -p "$TEMP_HOME/.config/Windsurf/User" | ||
| mkdir -p "$TEMP_HOME/.config/Antigravity/User" | ||
| mkdir -p "$TEMP_HOME/.config/Code - Insiders/User" | ||
| fi | ||
| } |
There was a problem hiding this comment.
The directory creation logic inside this setup function is repeated in other setup functions within this test file (e.g., lines 41-59 and 108-126). To improve maintainability and reduce code duplication, you could extract this logic into a shared helper function.
For example, you could define a function like _setup_test_dirs() at the top of the file and call it from each setup function.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
named-hosts/kyber/default.nix (1)
133-139: Reconsider 3-year GPG agent cache TTLs—significantly longer than best practices.The
defaultCacheTtlandmaxCacheTtlvalues of94608000seconds (~3 years) are orders of magnitude longer than Home Manager's defaults (600 and 7200 seconds respectively). This means a GPG passphrase remains cached indefinitely on this server, creating a substantial security risk if the user session is compromised. No rationale is documented for this extreme TTL.For a server, consider using shorter TTLs (hours at most, e.g. 3600–28800 seconds) unless a specific, documented use case requires longer caching.
services.gpg-agent = { enable = true; enableSshSupport = false; pinentry.package = pkgs.pinentry-tty; - defaultCacheTtl = 94608000; # 3 years - maxCacheTtl = 94608000; # 3 years + defaultCacheTtl = 3600; # 1 hour + maxCacheTtl = 28800; # 8 hours };
🧹 Nitpick comments (2)
home-manager/services/code-syncer/sync.sh (1)
109-124: Refactor sed invocation to avoid word-splitting issues.The current approach of storing
sed -i ''as a string inSED_INPLACEand then using unquoted expansion ($SED_INPLACE) is fragile and can cause word-splitting issues. Consider using a more robust pattern.Apply this diff to use a shell array for safer expansion:
- # Use appropriate sed syntax for OS - if [ "$OS_TYPE" = "Darwin" ]; then - SED_INPLACE="sed -i ''" - else - SED_INPLACE="sed -i" - fi - for bad_ext in "${PROPRIETARY_EXTENSIONS[@]}"; do # Remove the line containing the bad extension - $SED_INPLACE "/$bad_ext/d" "$clean_file" + if [ "$OS_TYPE" = "Darwin" ]; then + sed -i '' "/$bad_ext/d" "$clean_file" + else + sed -i "/$bad_ext/d" "$clean_file" + fi done for ai_ext in "${AI_EXTENSIONS[@]}"; do # Remove the line containing the AI extension - $SED_INPLACE "/$ai_ext/d" "$clean_file" + if [ "$OS_TYPE" = "Darwin" ]; then + sed -i '' "/$ai_ext/d" "$clean_file" + else + sed -i "/$ai_ext/d" "$clean_file" + fi donehome-manager/services/code-syncer/default.nix (1)
25-45: Consider adding service dependencies and documentation.The Linux systemd service definition is functional but could benefit from more explicit dependencies and documentation to align with systemd best practices.
Consider applying this diff:
systemd.user.services.code-syncer = lib.mkIf pkgs.stdenv.isLinux { Unit = { - Description = "VS Code settings syncer"; + Description = "Sync VS Code settings and extensions to VSCode forks"; + After = [ "graphical-session.target" ]; }; Service = { Type = "simple";This adds:
- More descriptive documentation of what the service does
Afterdependency to ensure the service starts after the graphical session is available (important for user-facing editors)Based on learnings, service configurations should include proper service dependencies and have clear documentation for service parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
home-manager/services/code-syncer/default.nix(2 hunks)home-manager/services/code-syncer/sync.sh(3 hunks)home-manager/services/dotfiles-updater/default.nix(1 hunks)named-hosts/kyber/default.nix(1 hunks)spec/code_syncer_spec.sh(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.nix
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.nix: Use nixfmt for formatting all Nix files
Document complex configurations with comments in Nix files
**/*.nix: Use 2 spaces for indentation in Nix files
Keep line length under 100 characters in Nix files
Sort attribute sets alphabetically in Nix files
Use consistent spacing around operators in Nix files
Format lists and sets consistently in Nix filesUse treefmt.toml for formatting Nix files
**/*.nix: UsemkOptionfor configurable options in Nix modules
Implement proper typing for all options in Nix modules
Follow the Nix expression language style guide
Files:
named-hosts/kyber/default.nixhome-manager/services/dotfiles-updater/default.nixhome-manager/services/code-syncer/default.nix
**/default.nix
📄 CodeRabbit inference engine (CLAUDE.md)
Use
default.nixfiles for module exports
Files:
named-hosts/kyber/default.nixhome-manager/services/dotfiles-updater/default.nixhome-manager/services/code-syncer/default.nix
**/*.{sh,bash}
📄 CodeRabbit inference engine (CLAUDE.md)
Use shfmt with 2-space indentation for shell scripts
**/*.{sh,bash}: Use 2 spaces for indentation in shell scripts
Add proper shebang lines to shell scripts
Follow shellcheck recommendations in shell scripts
Document complex commands in shell scripts
Use consistent variable naming in shell scripts
Files:
spec/code_syncer_spec.shhome-manager/services/code-syncer/sync.sh
home-manager/services/*/default.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
Service configurations should be located in
home-manager/services/<name>/with proper service definitions and correct dependency handling
Files:
home-manager/services/dotfiles-updater/default.nixhome-manager/services/code-syncer/default.nix
home-manager/services/**/default.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
Service configurations must include proper service definitions, handle dependencies correctly, and document service parameters
Files:
home-manager/services/dotfiles-updater/default.nixhome-manager/services/code-syncer/default.nix
home-manager/**/*.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
home-manager/**/*.nix: Use typed options whenever possible in Nix configurations
Document all configuration options in Nix modules and programs
Follow home-manager's module structure and keep configurations modular
Use proper indentation and formatting in Nix configuration files
Files:
home-manager/services/dotfiles-updater/default.nixhome-manager/services/code-syncer/default.nix
home-manager/services/**/*.nix
📄 CodeRabbit inference engine (.cursor/rules/nix.mdc)
Service configurations in
home-manager/services/should follow systemd service conventions, include proper service dependencies, and have clear documentation for service parameters
Files:
home-manager/services/dotfiles-updater/default.nixhome-manager/services/code-syncer/default.nix
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Applies to home-manager/services/**/*.nix : Service configurations in `home-manager/services/` should follow systemd service conventions, include proper service dependencies, and have clear documentation for service parameters
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Keep configurations modular across home-manager, hosts, and nix-darwin directories
📚 Learning: 2025-11-25T09:34:40.062Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Document all major changes in Nix configurations
Applied to files:
named-hosts/kyber/default.nixhome-manager/services/dotfiles-updater/default.nixhome-manager/services/code-syncer/default.nix
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Applies to home-manager/services/**/*.nix : Service configurations in `home-manager/services/` should follow systemd service conventions, include proper service dependencies, and have clear documentation for service parameters
Applied to files:
home-manager/services/dotfiles-updater/default.nixhome-manager/services/code-syncer/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/programs/**/*.nix : Follow program-specific best practices in program configuration files
Applied to files:
home-manager/services/dotfiles-updater/default.nixhome-manager/services/code-syncer/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/services/**/default.nix : Service configurations must include proper service definitions, handle dependencies correctly, and document service parameters
Applied to files:
home-manager/services/dotfiles-updater/default.nixhome-manager/services/code-syncer/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/services/*/default.nix : Service configurations should be located in `home-manager/services/<name>/` with proper service definitions and correct dependency handling
Applied to files:
home-manager/services/dotfiles-updater/default.nixhome-manager/services/code-syncer/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Follow home-manager's module structure and keep configurations modular
Applied to files:
home-manager/services/dotfiles-updater/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Document all configuration options in Nix modules and programs
Applied to files:
home-manager/services/dotfiles-updater/default.nix
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Applies to home-manager/programs/**/*.nix : Program configurations in `home-manager/programs/` should be organized by program name, include all necessary dependencies, use `home.packages` for package installations, and use `programs.<name>` when available in home-manager
Applied to files:
home-manager/services/dotfiles-updater/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Use proper indentation and formatting in Nix configuration files
Applied to files:
home-manager/services/dotfiles-updater/default.nix
📚 Learning: 2025-11-25T09:34:40.062Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Keep configurations modular across home-manager, hosts, and nix-darwin directories
Applied to files:
home-manager/services/dotfiles-updater/default.nix
🧬 Code graph analysis (1)
spec/code_syncer_spec.sh (1)
spec/support/custom_matcher.sh (1)
mock_bin_setup(5-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Agent
- GitHub Check: shellspec
- GitHub Check: shellcheck
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
- GitHub Check: docker-build-push (linux/amd64, amd64, ubuntu-latest)
- GitHub Check: lua-hammerspoon
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
- GitHub Check: lua-neovim
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: lua-neovim-test
- GitHub Check: nix-nixos
- GitHub Check: nix-linux
- GitHub Check: nix-darwin
🔇 Additional comments (4)
home-manager/services/code-syncer/sync.sh (2)
6-23: LGTM!The OS detection and platform-specific directory paths follow standard conventions: macOS uses
~/Library/Application Supportand Linux uses~/.config. The structure is clear and covers all supported editors.
416-441: LGTM!The OS-specific file watcher implementation properly uses
fswatchfor macOS andinotifywaitfor Linux, with appropriate fallback messages when tools are unavailable. The logic is well-structured and symmetric across platforms.home-manager/services/code-syncer/default.nix (1)
2-4: LGTM!The introduction of a local
libbinding viainherit (pkgs) libis idiomatic Nix and improves readability by avoiding repetitivepkgs.libprefixes.Also applies to: 6-6, 14-14
spec/code_syncer_spec.sh (1)
10-23: LGTM!The test updates properly implement OS-aware directory handling that mirrors the script's cross-platform logic. Tests now detect the OS and create appropriate directories for both macOS (
~/Library/Application Support) and Linux (~/.config) environments, ensuring comprehensive cross-platform test coverage.Also applies to: 42-59, 109-137
| Environment = "PATH=${ | ||
| lib.makeBinPath [ | ||
| pkgs.git | ||
| pkgs.bash | ||
| pkgs.coreutils | ||
| pkgs.gnumake | ||
| pkgs.curl | ||
| pkgs.gawk | ||
| pkgs.git | ||
| pkgs.gnumake | ||
| pkgs.gnused | ||
| pkgs.nix | ||
| pkgs.sudo | ||
| pkgs.which | ||
| ] | ||
| }"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "update.sh" home-manager/services/dotfiles-updater/Repository: shunkakinoki/dotfiles
Length of output: 235
🏁 Script executed:
fd --type f "update.sh" home-manager/services/dotfiles-updater/Repository: shunkakinoki/dotfiles
Length of output: 113
🏁 Script executed:
cat -n home-manager/services/dotfiles-updater/update.shRepository: shunkakinoki/dotfiles
Length of output: 536
🏁 Script executed:
fd --type f "install.sh" home-manager/services/dotfiles-updater/Repository: shunkakinoki/dotfiles
Length of output: 47
🏁 Script executed:
fd --type f "install.sh"Repository: shunkakinoki/dotfiles
Length of output: 75
🏁 Script executed:
wc -l install.sh && head -100 install.shRepository: shunkakinoki/dotfiles
Length of output: 3808
🏁 Script executed:
tail -60 install.sh && echo "---" && rg -n "sudo" install.shRepository: shunkakinoki/dotfiles
Length of output: 2186
🏁 Script executed:
cat -n home-manager/services/dotfiles-updater/default.nix | head -60Repository: shunkakinoki/dotfiles
Length of output: 1827
Remove sudo from the service PATH or document why it's necessary.
The sudo package is included in the systemd user service PATH, but examination of install.sh shows it's only used conditionally (line 136) to install make via apt-get if it's not already present. For a periodic update service, make should already be installed, making sudo unnecessary. Additionally, the macOS equivalent launchd.agents configuration omits sudo, suggesting it's not essential. Including sudo in a user service PATH presents a potential security consideration—if the update script is ever compromised, it could perform privileged operations. Either remove sudo from the PATH or document the security rationale for its inclusion.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (all 2 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="home-manager/services/code-syncer/sync.sh">
<violation number="1" location="home-manager/services/code-syncer/sync.sh:111">
P1: Storing `sed -i ''` in a string variable and expanding it with `$SED_INPLACE` won't work correctly. The `''` becomes literal quote characters, not an empty string argument. Use a bash array instead:
```bash
if [ "$OS_TYPE" = "Darwin" ]; then
SED_INPLACE=(sed -i '')
else
SED_INPLACE=(sed -i)
fi
Then invoke with "${SED_INPLACE[@]}" to preserve argument boundaries.
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
|
|
||
| # Use appropriate sed syntax for OS | ||
| if [ "$OS_TYPE" = "Darwin" ]; then | ||
| SED_INPLACE="sed -i ''" |
There was a problem hiding this comment.
P1: Storing sed -i '' in a string variable and expanding it with $SED_INPLACE won't work correctly. The '' becomes literal quote characters, not an empty string argument. Use a bash array instead:
if [ "$OS_TYPE" = "Darwin" ]; then
SED_INPLACE=(sed -i '')
else
SED_INPLACE=(sed -i)
fiThen invoke with "${SED_INPLACE[@]}" to preserve argument boundaries.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/code-syncer/sync.sh, line 111:
<comment>Storing `sed -i ''` in a string variable and expanding it with `$SED_INPLACE` won't work correctly. The `''` becomes literal quote characters, not an empty string argument. Use a bash array instead:
```bash
if [ "$OS_TYPE" = "Darwin" ]; then
SED_INPLACE=(sed -i '')
else
SED_INPLACE=(sed -i)
fi
Then invoke with "${SED_INPLACE[@]}" to preserve argument boundaries.
-
Use appropriate sed syntax for OS
- if [ "$OS_TYPE" = "Darwin" ]; then
- SED_INPLACE="sed -i ''"
- else
- SED_INPLACE="sed -i"
</file context>
</details>
<a href="https://www.cubic.dev/action/fix/violation/0e062d72-4e68-40be-bd63-cd9a5881b1d7" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true">
<picture>
<source media="(prefers-color-scheme: dark)" srcset="https://cubic.dev/buttons/fix-with-cubic-dark.svg">
<source media="(prefers-color-scheme: light)" srcset="https://cubic.dev/buttons/fix-with-cubic-light.svg">
<img alt="Fix with Cubic" src="https://cubic.dev/buttons/fix-with-cubic-dark.svg">
</picture>
</a>
| Service = { | ||
| Type = "simple"; | ||
| Environment = "PATH=${ | ||
| lib.makeBinPath [ |
There was a problem hiding this comment.
P1: The systemd service PATH is missing gnugrep and gnused packages. The sync.sh script uses grep (for filtering extensions) and sed (for in-place edits), which are separate packages in Nix, not part of coreutils.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/code-syncer/default.nix, line 32:
<comment>The systemd service PATH is missing `gnugrep` and `gnused` packages. The sync.sh script uses `grep` (for filtering extensions) and `sed` (for in-place edits), which are separate packages in Nix, not part of `coreutils`.</comment>
<file context>
@@ -18,4 +21,26 @@
+ Service = {
+ Type = "simple";
+ Environment = "PATH=${
+ lib.makeBinPath [
+ pkgs.bash
+ pkgs.coreutils
</file context>
There was a problem hiding this comment.
Pull request overview
This PR adds Linux support to the code syncer service and updates various system configurations. The main goal is to make the VS Code settings synchronization service work across both macOS and Linux platforms by detecting the OS and using platform-specific directory paths and file watchers.
- Adds OS detection logic using
uname -sto differentiate between macOS (Darwin) and Linux - Implements platform-specific file watchers (fswatch for macOS, inotifywait for Linux)
- Updates service configurations to support both launchd (macOS) and systemd (Linux)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/code_syncer_spec.sh | Updates test setup to create OS-specific directory structures and mock both fswatch and inotifywait |
| named-hosts/kyber/default.nix | Modifies GPG cache TTL settings from 30 minutes/2 hours to 3 years |
| home-manager/services/dotfiles-updater/default.nix | Adds missing packages (gawk, gnused, sudo, which) and reorders dependencies alphabetically |
| home-manager/services/code-syncer/sync.sh | Implements OS detection, platform-specific directory paths, sed command handling, and dual file watcher support |
| home-manager/services/code-syncer/default.nix | Adds systemd service configuration for Linux alongside existing launchd configuration for macOS |
Comments suppressed due to low confidence (1)
named-hosts/kyber/default.nix:138
- Setting GPG cache TTL to 3 years (94608000 seconds) is an extremely long duration that significantly undermines security. This means the GPG key will remain unlocked and accessible for 3 years without re-authentication. If the system is compromised during this period, the unlocked key can be used by an attacker. Consider using a more reasonable cache duration, such as a few hours (e.g., 3600-7200 seconds) to balance convenience and security.
defaultCacheTtl = 94608000; # 3 years
maxCacheTtl = 94608000; # 3 years
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Use appropriate sed syntax for OS | ||
| if [ "$OS_TYPE" = "Darwin" ]; then | ||
| SED_INPLACE="sed -i ''" |
There was a problem hiding this comment.
The SED_INPLACE variable is incorrectly set for macOS. The syntax "sed -i ''" needs to be split into separate arguments when used with variable expansion. The current approach will result in sed receiving a single argument "sed -i ''" instead of "sed", "-i", and an empty string. This should be changed to use an array or invoke sed directly in each branch to handle the platform-specific syntax correctly.
| if [ "$OS_TYPE" = "Darwin" ]; then | ||
| SED_INPLACE="sed -i ''" | ||
| else | ||
| SED_INPLACE="sed -i" | ||
| fi |
There was a problem hiding this comment.
The SED_INPLACE variable contains the command name "sed" itself, which will cause command execution to fail. When $SED_INPLACE is expanded on lines 118 and 123, it will result in commands like "sed -i '' /$bad_ext/d", which treats "sed" as a command to find rather than executing sed. The variable should only contain the flag arguments (-i '' for macOS, -i for Linux), and sed should be invoked directly.
| lib.makeBinPath [ | ||
| pkgs.bash | ||
| pkgs.coreutils | ||
| pkgs.inotify-tools |
There was a problem hiding this comment.
The systemd service PATH is missing several required packages that the script uses. The sync.sh script requires 'sed' (for extension filtering), 'grep' (for extension checking), and 'cp' (for file copying), but only bash, coreutils, and inotify-tools are included in the PATH. While coreutils provides cp, you should also add gnused and gnugrep to ensure the script has access to all required utilities on Linux systems.
| pkgs.inotify-tools | |
| pkgs.inotify-tools | |
| pkgs.gnused | |
| pkgs.gnugrep |
| inotifywait -m -e modify,create "$VSCODE_USER_DIR/$SETTINGS_FILE" "$VSCODE_USER_DIR/$KEYBINDINGS_FILE" 2>/dev/null | while read -r directory events filename; do | ||
| sync_config_file "$SETTINGS_FILE" | ||
| sync_config_file "$KEYBINDINGS_FILE" | ||
| echo "Updated at $(date)" |
There was a problem hiding this comment.
The inotifywait command will exit with a non-zero status code if the watched files don't exist at startup. If the SETTINGS_FILE or KEYBINDINGS_FILE don't exist when the service starts, inotifywait will fail and the script will exit. Consider adding existence checks for these files before starting the watcher, or use the --quiet flag with inotifywait and handle the case where files might be created after the watcher starts.
| inotifywait -m -e modify,create "$VSCODE_USER_DIR/$SETTINGS_FILE" "$VSCODE_USER_DIR/$KEYBINDINGS_FILE" 2>/dev/null | while read -r directory events filename; do | |
| sync_config_file "$SETTINGS_FILE" | |
| sync_config_file "$KEYBINDINGS_FILE" | |
| echo "Updated at $(date)" | |
| inotifywait -m -e modify,create "$VSCODE_USER_DIR" 2>/dev/null | while read -r directory events filename; do | |
| if [ "$filename" = "$SETTINGS_FILE" ]; then | |
| sync_config_file "$SETTINGS_FILE" | |
| fi | |
| if [ "$filename" = "$KEYBINDINGS_FILE" ]; then | |
| sync_config_file "$KEYBINDINGS_FILE" | |
| fi | |
| if [ "$filename" = "$SETTINGS_FILE" ] || [ "$filename" = "$KEYBINDINGS_FILE" ]; then | |
| echo "Updated at $(date)" | |
| fi |
| defaultCacheTtl = 94608000; # 3 years | ||
| maxCacheTtl = 94608000; # 3 years |
There was a problem hiding this comment.
services.gpg-agent is configured with defaultCacheTtl and maxCacheTtl set to 94,608,000 seconds (~3 years), which effectively keeps your GPG key passphrase cached and usable for an extremely long period once it has been entered. If an attacker gains access to this user account at any point while the agent is still running, they can sign or decrypt data with your GPG key without ever knowing the passphrase. Consider reducing these TTL values to a short duration (e.g., minutes or hours) so that long-lived server sessions do not leave the GPG key effectively unlocked for years.
| defaultCacheTtl = 94608000; # 3 years | |
| maxCacheTtl = 94608000; # 3 years | |
| defaultCacheTtl = 600; # 10 minutes | |
| maxCacheTtl = 7200; # 2 hours |
Summary by cubic
Cross-platform support for the VS Code settings syncer, adding Linux systemd integration and fixing OS-specific path and watcher handling. Also expands tool PATHs and increases GPG agent cache TTL to reduce prompts.
New Features
Refactors
Written for commit c9801a0. Summary will update automatically on new commits.