Skip to content

Standardize command checks and add input validation#23

Merged
shyndman merged 2 commits intomainfrom
claude/issue-18-20251210-0841
Dec 10, 2025
Merged

Standardize command checks and add input validation#23
shyndman merged 2 commits intomainfrom
claude/issue-18-20251210-0841

Conversation

@shyndman
Copy link
Copy Markdown
Owner

  • Replace command -v with zsh-idiomatic (( ${+commands[X]} )) in rc.d/50-editor.zsh and rc.d/84-git-worktrees.zsh
  • Add parameter validation to zush_lazy_load() in lib/lazy-loader.zsh
  • Improve validation in zushp() with regex format checking

Fixes #18

- Replace `command -v` with zsh-idiomatic `(( ${+commands[X]} ))`
  in rc.d/50-editor.zsh and rc.d/84-git-worktrees.zsh
- Add parameter validation to zush_lazy_load() in lib/lazy-loader.zsh
- Improve validation in zushp() with regex format checking

Fixes #18

Co-authored-by: Scott Hyndman <shyndman@users.noreply.github.com>
Copilot AI review requested due to automatic review settings December 10, 2025 09:04
@claude
Copy link
Copy Markdown

claude bot commented Dec 10, 2025

Pull Request Review: Standardize command checks and add input validation

Summary

This PR addresses issue #18 by standardizing command-existence checks and adding input validation to user-facing functions. The changes are well-aligned with the stated objectives and improve code consistency and error handling.

✅ Strengths

1. Correct Use of Zsh Idioms

The change from command -v to (( ${+commands[X]} )) is the right approach:

  • rc.d/50-editor.zsh:5 and rc.d/84-git-worktrees.zsh:110 now use the zsh-idiomatic pattern
  • This avoids spawning subshells and is more performant
  • Follows established patterns elsewhere in the codebase (lib/core.zsh:62,109)

2. Strong Input Validation in zush_lazy_load()

lib/lazy-loader.zsh:89-98 - Excellent parameter validation:

  • Checks for empty tool and init_command parameters
  • Uses zush_error() for consistent error reporting
  • Returns proper error codes (1)
  • Clear error messages that help users understand what is missing

3. Improved User/Repo Format Validation in zushp()

lib/plugin.zsh:80-89 - Much better validation:

  • Consistent use of zush_error() instead of plain echo
  • Regex validation properly ensures GitHub user/repo format
  • Prevents cryptic git clone errors downstream
  • The regex correctly allows dots in repo names (common for repos like foo.js or .github)

🔍 Areas for Consideration

1. Incomplete Standardization in rc.d/

The PR only addresses 2 of 7 files using command -v in the rc.d/ directory. Remaining files:

  • rc.d/42-eza.zsh:5,12 (checking eza and curl)
  • rc.d/81-fzf.zsh:5,8 (checking magick, fd, rg)
  • rc.d/10-lazy-tools.zsh:25 (checking pyenv)
  • rc.d/90-prompt.zsh:5,13 (checking starship and curl)
  • rc.d/60-better-reading.zsh:5,10 (checking bat and batman)

Recommendation: Consider expanding the scope to standardize all command checks in one pass, or create a follow-up issue to track the remaining conversions. Partial standardization can lead to confusion about which pattern to use.

2. Regex Pattern Edge Cases

The regex in lib/plugin.zsh:86 is generally good but could be more restrictive:

  • Allows consecutive dots (user/repo..name) which GitHub does not permit
  • Allows leading/trailing dots (user/.repo or user/repo.) which may not be valid
  • GitHub usernames cannot contain dots, but the user part allows them

However, the current regex is probably good enough since git will fail gracefully anyway for invalid repos.

3. No Tests for New Validation Logic

The codebase appears to lack test files. While not introduced by this PR, the new validation logic would benefit from test coverage.

🐛 Potential Issues

None identified - The implementation is solid.

🔒 Security Considerations

✅ Improved Security Posture

The regex validation in zushp() provides defense-in-depth:

  • Prevents command injection via malformed repository names
  • Reduces attack surface by rejecting unexpected input early

⚡ Performance Considerations

✅ Minor Performance Improvement

  • (( ${+commands[X]} )) is faster than command -v (no subshell fork)
  • The regex check adds negligible overhead
  • Parameter validation prevents cryptic failures later

📋 Code Quality

✅ High Quality

  • Consistent with existing code style and patterns
  • Proper use of zush_error() utility
  • Clear, descriptive error messages
  • Proper return codes

🎯 Final Recommendation

Approve with minor suggestions:

This is a solid PR that successfully addresses the stated objectives. The code quality is high, security is improved, and the changes follow zsh best practices.

Suggested next steps:

  1. ✅ Merge this PR as-is (it is good!)
  2. 📝 Create follow-up issue to standardize remaining command -v usage in rc.d/ directory
  3. 🧪 Consider adding basic test infrastructure (optional)

Nitpicks (totally optional):

  • Consider tightening the regex to better match GitHub naming rules
  • Could add a comment explaining why (( ${+commands[X]} )) is preferred

Great work! 🚀

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR standardizes command existence checks and enhances input validation across the zush codebase. The changes migrate from bash-compatible command -v patterns to zsh-idiomatic (( ${+commands[X]} )) syntax and add robust parameter validation to critical functions.

Key changes:

  • Standardizes command existence checks using zsh's native ${+commands[X]} syntax in two files
  • Adds parameter validation to zush_lazy_load() to prevent silent failures from missing arguments
  • Enhances zushp() with regex-based format validation for user/repo input and consistent error handling

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
rc.d/84-git-worktrees.zsh Replaces command -v git with zsh-idiomatic (( ${+commands[git]} )) check
rc.d/50-editor.zsh Replaces command -v code with zsh-idiomatic (( ${+commands[code]} )) check
lib/lazy-loader.zsh Adds validation for required tool and init_command parameters with descriptive error messages
lib/plugin.zsh Improves validation with regex pattern matching for user/repo format and standardizes error handling using zush_error

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Repository owner deleted a comment from claude bot Dec 10, 2025
Show the actual value passed when validation fails in zush_lazy_load()
and zushp() so users can easily identify their mistake.
@shyndman shyndman merged commit 53f5a6f into main Dec 10, 2025
1 of 2 checks passed
@shyndman shyndman deleted the claude/issue-18-20251210-0841 branch December 10, 2025 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Standardize command-existence checks and add input validation

3 participants