fixes discussed in call #5
Conversation
…pdate README to replace 'git config' with 'gtr config' for clarity and consistency. Enhance gtr script to support 'add' action in config management, allowing multi-valued configurations. Update error messages and documentation for better user guidance.
…rom potentially multi-word editor strings to ensure proper detection and execution. Update dynamic function definitions to accommodate editor arguments, improving compatibility with various editor commands.
…from potentially multi-word AI tool strings to ensure accurate detection. Update dynamic function definitions to handle arguments correctly, enhancing compatibility with various AI tools.
…e function to capture worktree path and redirect error output to stderr for better logging. Enhance log functions in ui.sh to ensure consistent error reporting.
|
Warning Rate limit exceeded@helizaga has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaces the top-level "open" command with "editor", adds a config "add" action, implements PATH-based fallback for editor and AI adapters, captures created worktree paths, and redirects several outputs/logs to stderr; documentation and shell completions updated to match. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant gtr as bin/gtr
participant Explicit as Explicit Adapter
participant PATH as PATH-based Adapter
User->>gtr: request editor for worktree
gtr->>gtr: load_editor_adapter(editor_name)
alt explicit adapter file exists
gtr->>Explicit: load explicit adapter
Explicit-->>gtr: provides editor_can_open / editor_open
else no explicit adapter
gtr->>PATH: check editor command in PATH
PATH-->>gtr: provide generic editor_can_open / editor_open wrappers
end
gtr-->>User: invoke editor_open to open worktree
sequenceDiagram
participant User
participant gtr as bin/gtr
participant ExplicitAI as Explicit AI Adapter
participant PATHAI as PATH-based AI
User->>gtr: request AI tool start
gtr->>gtr: load_ai_adapter(ai_tool)
alt explicit AI adapter exists
gtr->>ExplicitAI: load adapter
ExplicitAI-->>gtr: ai_can_start / ai_start
else fallback to PATH
gtr->>PATHAI: extract command, verify in PATH
PATHAI-->>gtr: supply generic ai_can_start / ai_start wrappers
end
gtr-->>User: start AI tool
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
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: 0
🧹 Nitpick comments (4)
lib/core.sh (1)
293-368:git worktree addoutput to stderr matches the new “stdout is data” contractRouting all
git worktree addcalls through>&2is a good fit with the new usage wherecreate_worktree’s stdout is consumed programmatically (captured inbin/gtr). This keeps git’s messages on stderr whilecreate_worktreeitself prints only$worktree_pathon stdout.To keep this contract robust over time, it’s worth double-checking that no other commands inside
create_worktreeever emit to stdout (now or in future refactors), otherwise callers that capture stdout may end up with mixed output instead of a clean path.bin/gtr (2)
186-215: Capturingcreate_worktreeoutput is correct but makes its stdout contract importantUsing
worktree_path=$(create_worktree ...)and then passing that tocopy_patternsandrun_hooks_inis a solid improvement; hooks and copy now work with the actual created path instead of recomputing it.Because of this,
create_worktree’s stdout effectively becomes an API (must be exactly the worktree path, nothing else). You’ve already redirected thegit worktree addcalls to stderr; I’d just recommend:
- Add a short comment to
create_worktreedocumenting that it must only print the path on stdout, or- Proactively redirect any other potentially chatty commands in that function to stderr as well.
That will make future edits less likely to accidentally break
worktree_pathcapture.
802-856: Generic editor/AI adapters via PATH are handy; consider reducingevaland hardening quotingThe PATH-based fallbacks in
load_editor_adapterandload_ai_adapterare a nice usability improvement and align with the docs (any command in PATH, multi-word commands likecode --waitorbunx @github/copilot@latest). Functionally this should work as intended.A couple of refinements would make this more robust:
- Both functions use
evalwith interpolated$editor/$ai_tooland$cmd_name. If these ever contain quotes, braces, or other shell metacharacters (e.g., from a malicious repo-local config), the generated function bodies can break or behave unexpectedly.- You can avoid
evalentirely by storing the command and its first word in globals and writing simple wrappers, e.g.:load_editor_adapter() { local editor="$1" local adapter_file="$GTR_DIR/adapters/editor/${editor}.sh" if [ -f "$adapter_file" ]; then . "$adapter_file" return 0 fi local cmd_name="${editor%% *}" if ! command -v "$cmd_name" >/dev/null 2>&1; then log_error "Editor '$editor' not found" log_info "Built-in adapters: cursor, vscode, zed, idea, pycharm, webstorm, vim, nvim, emacs, sublime, nano, atom" log_info "Or use any editor command available in your PATH (e.g., code-insiders, fleet)" exit 1 fi + GTR_EDITOR_CMD="$editor" + GTR_EDITOR_CMD_NAME="$cmd_name" +} + +editor_can_open() { + command -v "$GTR_EDITOR_CMD_NAME" >/dev/null 2>&1 +} + +editor_open() { + # $GTR_EDITOR_CMD may contain arguments (e.g., "code --wait") + eval "$GTR_EDITOR_CMD \"\$1\"" }A similar pattern can be applied to
load_ai_adapter/ai_can_start/ai_start.Not strictly required for this PR, but it would remove
evalfrom function-definition paths and make behavior more predictable, especially when config comes from a cloned repo.README.md (1)
223-231: Includeaddin thegtr configcommand signature for consistencyThis section still shows:
gtr config {get|set|unset} <key> [value] [--global]Everywhere else (CLI help and examples) now documents
addas a first-class action, and the implementation supports it. Consider updating this line to:
gtr config {get|set|add|unset} <key> [value] [--global]to keep the README consistent with the tool’s capabilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(5 hunks)bin/gtr(6 hunks)lib/core.sh(4 hunks)lib/ui.sh(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/core.sh (1)
lib/ui.sh (2)
log_info(4-6)log_step(16-18)
🔍 Remote MCP Exa
Based on my research, I have gathered sufficient context about the PR changes and best practices. Let me compile the relevant findings for the pull request review.
Summary of Relevant Context for PR #5 Review
Output Redirection and Error Handling Best Practices
The PR changes in lib/core.sh and lib/ui.sh redirect output to stderr using >&2 instead of suppressing it with 2>/dev/null. This aligns with bash shell scripting best practices:
- Error visibility: Redirecting to stderr (
>&2) preserves error information for debugging and logging, which is preferable to suppressing errors unless there's a specific reason - Logging separation: Best practice is to separate standard output (informational messages) from standard error (diagnostic/error messages), allowing flexible handling downstream
- Logging functions: Modern shell scripts typically implement error logging functions that explicitly write to stderr using
>&2for diagnostic messages
Git Worktree Command Patterns and Configuration
The changes in bin/gtr align with standard git worktree management patterns:
- Worktree path capturing: The change to capture the worktree path from
create_worktreeoutput is consistent with practical worktree workflows where the path is needed for subsequent operations - Configuration management: Git worktrees support flexible configuration including per-worktree settings. The extension of
cmd_configto supportaddaction alongsidesetandunsetfollows standard configuration management practices - Fallback adapter patterns: The implementation of PATH-based fallback adapters for editor and AI tools is consistent with flexible tool integration patterns in shell scripts, allowing users to specify any command available in their PATH
Configuration Command Syntax
The README changes replacing git config syntax with gtr config add/set/get commands represent a significant public API update. Key review considerations:
- Clarity and consistency of the new
gtrsyntax across documentation examples - Backward compatibility impact if this replaces previous configuration methods
- Documentation completeness for the new
addaction capability
Code Quality Observations
Based on the changes:
- bin/gtr: High effort review justified by multiple behavioral changes including new
addaction and fallback adapter logic - lib/core.sh & lib/ui.sh: Medium effort review appropriate for focused output redirection changes
- README.md: Medium effort review appropriate for documentation updates affecting user guidance
🔇 Additional comments (4)
lib/ui.sh (1)
4-18: Logging info/step messages to stderr is consistent and helpfulRedirecting
log_infoandlog_steptostderrkeeps stdout clean for command-substitution/machine output and aligns them withlog_warn/log_error.log_questioncorrectly stays on stdout for prompts. Looks good.bin/gtr (2)
728-800:cmd_config“add” action wiring looks goodExtending
cmd_configto supportaddalongsideget|set|unsetis wired correctly:
- Argument parsing only captures
valuewhenactionissetoradd.- The
addbranch validates bothkeyandvalueand delegates tocfg_add.- Usage/help strings are updated to mention
add.Assuming
cfg_addis implemented to mirrorgit config --addsemantics for multi-valued keys, this looks consistent with the README examples usinggtr config add ....
922-937: Help text correctly documentsconfig addand PATH-based adaptersThe
cmd_helpsection now:
- Lists
config {get|set|add|unset}and explainsaddas “multi-valued configs like hooks, copy patterns”.- Explicitly notes that any command in PATH can be used for adapters (e.g.,
code-insiders,bunx).This matches the behavior in
cmd_config,load_editor_adapter, andload_ai_adapterand keeps the CLI self-documenting.README.md (1)
321-402: README config examples align with newconfig addbehaviorThe updated examples under “File Copying”, “Hooks”, “Configuration Examples”, and
.gtr-setup.shall usegtr config addfor multi-valued keys (gtr.copy.include,gtr.copy.exclude,gtr.hook.postCreate,gtr.hook.postRemove, etc.), which matches the newcmd_config+cfg_addsemantics and the waycfg_get_allis used in the code.This makes it much clearer how to manage multi-valued settings and is consistent with the CLI help.
…mentation and scripts for consistency. Modify completions and command handling in gtr script to reflect the new editor command. Enhance README and CLAUDE.md to guide users on the updated command usage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CLAUDE.md (1)
207-213: Consider adding a language specifier to the code block.The fenced code block showing the command flow would benefit from a language specifier for consistency with the rest of the document.
Apply this diff:
-``` +```text bin/gtr main() → cmd_editor() → resolve_target() [lib/core.sh] → load_editor_adapter() → editor_open() [adapters/editor/*.sh]</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d4bd5aa31044fae7dcbb02edc346cb3b560f7c55 and 251a8d4458ca05005521729c5b8fb288bba4f3a7. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `CLAUDE.md` (6 hunks) * `README.md` (12 hunks) * `bin/gtr` (13 hunks) * `completions/_gtr` (2 hunks) * `completions/gtr.bash` (1 hunks) * `completions/gtr.fish` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>CLAUDE.md</summary> 207-207: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> <details> <summary>🪛 Shellcheck (0.11.0)</summary> <details> <summary>completions/gtr.bash</summary> [warning] 12-12: Prefer mapfile or read -a to split command output (or quote to avoid splitting). (SC2207) </details> </details><details> <summary>🔍 Remote MCP Context7, Exa, Linear</summary> ## Additional context gathered (concise, review-focused) - Practical git-worktree usage and patterns (creates/removes worktrees, recommended workflows for reviewing PRs without stashing). Useful to verify gtr's worktree creation/capture changes and PATH-based editor-open behavior against common expectations for worktree tools. - Related community projects and tooling (gwtm, gwt) that implement similar CLI wrappers for git worktree; helpful for UX/CLI-pattern comparisons (e.g., how they handle defaults, editor integration). Use this when reviewing CLI semantics, completions, and README examples. - General git worktree tutorials and docs explaining add/remove/view semantics and cleanup considerations (prune, removing by path). Relevant to validating the PR change that now captures created worktree path and to ensure removal/prune flows remain correct. - Linear documentation search returned general product docs (creating issues, parent/sub-issues, integrations) but no repository- or PR-specific Linear item was found. This indicates no discoverable Linear tickets linked to this PR in the workspace documentation; recommend checking the project’s Linear board directly if needed. - Context7 library lookup failed due to an authorization error when attempting to resolve a library ID; I could not fetch Context7 docs. Please provide a valid Context7 API key or a direct Context7 library ID if you want documentation lookups (e.g., '/org/project' format). Sources/tools used: - Exa_web_search_exa (web searches for git-worktree, gwtm/gwt, tutorials) — - Linear_search_documentation (broad Linear docs search) — - Context7_resolve-library-id (attempt; returned auth error) — </details> </details> <details> <summary>🔇 Additional comments (11)</summary><blockquote> <details> <summary>completions/gtr.bash (1)</summary><blockquote> `12-18`: **LGTM!** The completion updates correctly reflect the command rename from `open` to `editor`. The changes are consistent with the pattern used in other completion files. </blockquote></details> <details> <summary>completions/gtr.fish (1)</summary><blockquote> `7-7`: **LGTM!** The Fish completion correctly reflects the command rename from `open` to `editor`. Both the command definition and the branch completion helper are properly updated. Also applies to: 55-55 </blockquote></details> <details> <summary>completions/_gtr (1)</summary><blockquote> `10-10`: **LGTM!** The Zsh completion correctly reflects the command rename from `open` to `editor`. The changes are consistent with the Bash and Fish completion updates. Also applies to: 32-32 </blockquote></details> <details> <summary>README.md (1)</summary><blockquote> `178-185`: **LGTM!** The README updates are comprehensive and consistent: - Command signature properly updated from `gtr open` to `gtr editor` - Configuration examples correctly migrate from `git config --add` to `gtr config add` - Configuration examples correctly migrate from `git config set` to `gtr config set` These documentation changes align well with the functional updates in bin/gtr. Also applies to: 327-374, 381-410 </blockquote></details> <details> <summary>bin/gtr (7)</summary><blockquote> `46-47`: **LGTM!** The command dispatcher correctly routes the new `editor` command to `cmd_editor`. The change maintains consistency with the existing case statement pattern. --- `187-187`: **Good fix!** Capturing the `worktree_path` from `create_worktree` is essential for the subsequent `copy_patterns` (line 199) and `run_hooks_in` (line 204) calls to work correctly. This addresses a critical issue where the worktree path was needed but not captured. --- `335-391`: **LGTM!** The function rename from `cmd_open` to `cmd_editor` correctly aligns with the new command name. The function logic remains unchanged, maintaining the same behavior. --- `807-828`: **Approve with awareness of eval usage.** The PATH-based fallback for editor adapters is well-implemented and provides excellent flexibility. The code correctly: - Tries explicit adapters first - Falls back to PATH-based commands - Extracts the command name for validation - Supports commands with arguments (e.g., "code --wait") Note: The `eval` usage (lines 826-827) with user-provided editor names is acceptable here since the values come from git config (trusted source), but be mindful of this pattern if extending to untrusted input. --- `835-856`: **LGTM!** The PATH-based fallback for AI adapters follows the same pattern as the editor adapters and is well-implemented. The code correctly: - Tries explicit adapters first - Falls back to PATH-based commands - Uses a subshell to change directory without affecting the current shell - Supports commands with arguments The consistency between editor and AI adapter loading is excellent. --- `872-872`: **LGTM!** The help text updates comprehensively document the changes: - Command examples updated from `open` to `editor` - Config action documentation includes the new `add` action with clear descriptions - Note about PATH-based commands provides helpful guidance to users The documentation is clear and consistent with the implemented functionality. Also applies to: 898-898, 924-929, 936-936 --- `740-785`: **Code changes verified—no issues found.** The `cfg_add` function is properly defined in `lib/config.sh` (lines 96-109) and correctly implements the add functionality for git config. The integration in `bin/gtr` is sound. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…ool integration. Update README to reflect new configuration command options, including 'add'. Improve command handling for better compatibility with multi-word commands.
Summary by CodeRabbit
New Features
config addaction; introducededitorsubcommand (replacingopen) and PATH-based fallback for editors and AI tools.Bug Fixes
Documentation
gtr editorand the new config/adapter behaviors.