Add include directive support for dotfile management#253
Conversation
Instead of directly syncing config files, configs that support include
directives now use a two-file approach:
- The original file (e.g., ~/.gitconfig) gets an include line added at top
- The actual synced content lives in a .shelltime suffixed file
(e.g., ~/.gitconfig.shelltime)
On push: ensures include line exists in original, copies content to
.shelltime file, and uploads the .shelltime version to server.
On pull: ensures include line exists in original, downloads content
from server to the .shelltime file.
Supported include syntaxes by app:
- git: [include] path = ~/.gitconfig.shelltime
- zsh/bash: [[ -f ~/.zshrc.shelltime ]] && source ~/.zshrc.shelltime
- fish: test -f <path>.shelltime; and source <path>.shelltime
- ssh: Include ~/.ssh/config.shelltime
- nvim: if filereadable(expand('~/.vimrc.shelltime')) | source ... | endif
Apps without include support (ghostty, claude, starship, npm, kitty,
kubernetes) keep the existing direct push/pull behavior.
Directory-based configs also keep existing behavior.
https://claude.ai/code/session_01WKUeEgn1nMULetY8i2WwDR
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 significantly enhances dotfile management by introducing include directive support. This feature allows users to maintain local customizations in their dotfiles while keeping the core, synced configurations separate in Highlights
Changelog
Activity
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
|
![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
The pull request introduces robust include directive support for dotfile management, allowing for separation of synced and local configurations. The implementation covers various applications and includes comprehensive test coverage for different scenarios. The overall approach is well-structured and addresses a common need in dotfile management. My feedback focuses on a minor consistency improvement in the BaseApp interface implementation.
Note: Security Review did not run due to the size of the PR.
| func (b *BaseApp) GetIncludeDirectives() []IncludeDirective { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The comment for GetIncludeDirectives states it returns an "empty slice by default", but the implementation returns nil. While nil slices often behave like empty slices, it's generally a good practice in Go to return an actual empty slice ([]IncludeDirective{}) to ensure consistency and avoid potential nil checks by consumers who might expect a non-nil slice.
| func (b *BaseApp) GetIncludeDirectives() []IncludeDirective { | |
| return nil | |
| } | |
| func (b *BaseApp) GetIncludeDirectives() []IncludeDirective { | |
| return []IncludeDirective{} | |
| } |
|
@claude review it |
|
Claude finished @AnnatarHe's task in 1m 29s —— View job Code Review: Add include directive support for dotfile management
Overall this is a well-structured feature with good separation of concerns. The Bugs (failing tests)1. End-to-end test fails — The E2E test This is a real bug — when pulling dotfiles from the server, the 2. This test also fails due to the diff-merge producing unexpected output for a simple file write. The diff shows Potential Issues3. File permission not preserved (
4. Comment vs implementation mismatch ( The comment says "returns an empty slice" but the implementation returns 5.
6. SSH SSH's 7. Race condition on first-time setup ( In Minor Suggestions8. The 9. Consider adding a 10. The SummaryThe architecture is solid. The main blocker is the diff-merge bug causing 2 test failures — | |
|
@claude fix testcases |
|
Claude finished @AnnatarHe's task in 2m 18s —— View job Fixing failing test cases
SummaryFixed 2 failing tests with two changes:
|
…erge SaveWithIncludeSupport now writes .shelltime files directly (overwrite) since they are fully server-managed. BaseApp.Save now writes new files directly to avoid packfile delta binary artifacts with empty base content. Co-authored-by: Le He <AnnatarHe@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| func writeFileWithDir(path, content string) error { | ||
| dir := filepath.Dir(path) | ||
| if err := os.MkdirAll(dir, 0755); err != nil { | ||
| return err | ||
| } | ||
| return os.WriteFile(path, []byte(content), 0644) | ||
| } |
There was a problem hiding this comment.
🔴 SSH ~/.ssh directory created with 0755 instead of required 0700 permissions
The writeFileWithDir function creates parent directories with 0755 permissions (model/dotfile_include.go:230). When the SSH app's include support triggers creation of ~/.ssh/config.shelltime, if ~/.ssh doesn't already exist, os.MkdirAll(dir, 0755) creates it with world-readable permissions. OpenSSH requires ~/.ssh to be 0700 and will refuse to use keys/config from an insecure directory.
This is reachable during pull: if a user pulls SSH config to a machine without ~/.ssh/, ensureIncludeLineInFile at model/dotfile_include.go:112 calls writeFileWithDir to create ~/.ssh/config, which creates ~/.ssh with 0755. Similarly, SaveWithIncludeSupport at model/dotfile_include.go:210 calls writeFileWithDir to create ~/.ssh/config.shelltime. The resulting directory permissions silently break SSH.
Prompt for agents
The writeFileWithDir function in model/dotfile_include.go:228-234 hardcodes directory permissions to 0755. This breaks SSH, which requires ~/.ssh to be 0700. Similarly, the ensureIncludeSetup function at lines 59, 63, 69 and ensureIncludeLineInFile at line 112 write files with 0644 permissions, but SSH config files are conventionally 0600.
To fix this properly, you should either:
1. Make writeFileWithDir accept a directory permission parameter and a file permission parameter, allowing SSH-specific code to pass 0700 and 0600 respectively.
2. Or add SSH-specific overrides in the SshApp that handle the special permission requirements.
All call sites that may write to ~/.ssh/ paths need to use 0700 for directory creation and 0600 for file creation. This includes:
- model/dotfile_include.go:59 (writeFileWithDir for .shelltime file)
- model/dotfile_include.go:63 (os.WriteFile for original file)
- model/dotfile_include.go:69 (os.WriteFile for original file)
- model/dotfile_include.go:112 (writeFileWithDir for original file)
- model/dotfile_include.go:210 (writeFileWithDir for .shelltime file)
- model/dotfile_include.go:228-234 (the generic writeFileWithDir helper)
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Fallback: remove any line containing the check string | ||
| var filtered []string | ||
| for _, line := range lines { | ||
| if !strings.Contains(line, directive.CheckString) { | ||
| filtered = append(filtered, line) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 removeIncludeLines fallback leaves orphaned [include] section header for git's multi-line directive
For git's multi-line include directive (IncludeLine: "[include]\n path = ~/.gitconfig.shelltime", CheckString: ".gitconfig.shelltime"), the fallback path in removeIncludeLines (lines 262-268) only removes lines containing the CheckString. This removes the path = ~/.gitconfig.shelltime line but leaves the [include] section header orphaned in the file, since [include] doesn't contain .gitconfig.shelltime.
This is triggered when the include directive is not at the top of the file (e.g., user moved it). The result is a gitconfig with an empty [include] section, which is harmless for git but leaves config file debris.
Prompt for agents
In model/dotfile_include.go, the removeIncludeLines function's fallback logic at lines 262-268 only removes lines containing the CheckString. For multi-line include directives like git's [include] section, this leaves the section header orphaned.
To fix this, the fallback should also try to find and remove the full multi-line include block anywhere in the file (not just at the top). One approach: scan through the file looking for a sequence of lines that match the includeLines (similar to the top-of-file check at lines 244-259 but searching at any position), and remove those lines plus any trailing blank line.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
This PR introduces include directive support to enable separation of synced and local configuration in dotfiles. When pushing/pulling dotfiles, the original config file gets an include line added that sources a
.shelltimeversion, allowing users to maintain local customizations alongside synced settings.Key Changes
New
IncludeDirectivetype (dotfile_include.go): Defines how config files include their shelltime-managed versions with fields for original path, shelltime path, include line syntax, and check string for detection.Core include management functions:
ensureIncludeSetup(): Handles initial setup by copying content to.shelltimefile and adding include line to originalensureIncludeLineInFile(): Ensures include line exists in original config during pull operationsremoveIncludeLines(): Extracts synced content by removing include directives from original filesCollectWithIncludeSupport(): Collects dotfiles from.shelltimefiles instead of originals when directives existSaveWithIncludeSupport(): Saves to.shelltimefiles and ensures include lines in originalsApp-specific include directives: Added
GetIncludeDirectives()method to all supported apps:.bashrc,.bash_profile,.bash_aliases,.bash_logout.zshrc,.zshenv,.zprofile~/.gitconfigand~/.config/git/configwith multi-line[include]syntax~/.config/fish/config.fish~/.ssh/configwithIncludedirective~/.vimrcUpdated collection and save flows: Modified
CollectDotfiles()andSave()methods in bash, zsh, git, fish, ssh, and nvim apps to use include-aware variants.Interface update: Added
GetIncludeDirectives()toDotfileAppinterface; apps without include support returnnil.Comprehensive test coverage (
dotfile_include_test.go): 747 lines of tests covering:.shelltimefile)Implementation Details
.shelltimefile writes[include]section) are properly handled with line-by-line matchinghttps://claude.ai/code/session_01WKUeEgn1nMULetY8i2WwDR