feat(version): add CLI version check on extension startup #12
Conversation
Check daemon CLI version against server API when extension starts.
If a newer version is available, show warning notification with
update command: curl -sSL {webEndpoint}/i | bash
- Add version.lua module with async curl HTTP request
- Read apiEndpoint and webEndpoint from config file
- Show warning only once per session
- Auto-copy update command to clipboard
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @AnnatarHe, 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 integrates an essential feature for maintaining an up-to-date CLI environment. It introduces an automatic version check that runs when the extension starts, querying a remote server to determine if the currently installed daemon is the latest. If an update is available, the user receives a clear notification with instructions and an update command, which is conveniently copied to their clipboard, streamlining the update process and ensuring users are always running the most current and secure version of the CLI. 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
|
Pull Request Review: feat(version): add CLI version check on extension startupSummaryThis PR adds version checking functionality that calls a server API on startup to verify if the daemon version is up-to-date, and displays a notification if an update is available. Overall, the implementation is solid, but there are several areas that need attention. Code Quality & Best Practices✅ Positives
|
| Category | Rating | Notes |
|---|---|---|
| Code Quality | Follows conventions but needs refinement | |
| Test Coverage | ❌ Poor | No tests added |
| Performance | ✅ Good | Async, non-blocking implementation |
| Security | 🔴 Critical | Command injection risk needs addressing |
| Documentation | Needs user-facing docs |
Verdict
Changes Requested - This PR needs work before merging:
Must Fix (Blocking)
- ❌ Add comprehensive test coverage
- 🔴 Fix command injection security vulnerability
⚠️ Replace manual JSON parsing withvim.json.decode()⚠️ Fix URL encoding function
Should Fix (Recommended)
- Add URL validation for endpoints
- Add user documentation
- Consider making version check optional via config
- Add error handling in sender.lua
Great work on the feature implementation! The architecture is sound, but the security and testing gaps need to be addressed before this can be merged safely.
🤖 Generated with Claude Code
![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 a CLI version check on startup, which is a great feature for keeping users up-to-date. The implementation is well-structured in a new version.lua module and uses an asynchronous approach for the check, which is good for performance.
However, I have identified a few issues that should be addressed:
- A critical security concern with the suggested update command, which executes a script directly from the internet.
- The use of a fragile custom JSON parser instead of the robust built-in
vim.json.decode. - A bug in the URL encoding logic.
Additionally, the new functionality in version.lua is not covered by automated tests. It would be beneficial to add tests for this new module to ensure its correctness and prevent future regressions.
Please see my detailed comments below.
| ---@param latest_version string Latest available version | ||
| ---@param web_endpoint string Web endpoint for update command | ||
| function M.show_update_warning(current_version, latest_version, web_endpoint) | ||
| local update_command = 'curl -sSL ' .. web_endpoint .. '/i | bash' |
There was a problem hiding this comment.
Piping the output of curl directly into bash is a significant security risk. This pattern can lead to remote code execution if the download URL is compromised, or if the user is on a malicious network (e.g., via DNS spoofing). The script is executed without any opportunity for the user to inspect it. While this is a common installation method, it's dangerous to promote within an editor plugin, especially by copying the command to the clipboard which encourages blind execution. Consider providing just the URL to the user and advising them to inspect the script before running it.
| local function parse_json(json_str) | ||
| -- Simple JSON parser for { "isLatest": bool, "latestVersion": "...", "version": "..." } | ||
| local is_latest = json_str:match('"isLatest"%s*:%s*(true)') | ||
| local latest_version = json_str:match('"latestVersion"%s*:%s*"([^"]+)"') | ||
| local version = json_str:match('"version"%s*:%s*"([^"]+)"') | ||
|
|
||
| if latest_version and version then | ||
| return { | ||
| isLatest = is_latest ~= nil, | ||
| latestVersion = latest_version, | ||
| version = version, | ||
| } | ||
| end | ||
| return nil | ||
| end |
There was a problem hiding this comment.
The custom JSON parser implemented here is very fragile as it relies on string matching with regular expressions. This can easily break if the API response format changes in ways not anticipated by the regex (e.g., different value types, number formats). Neovim provides a robust built-in JSON decoder, vim.json.decode, which is already used in other parts of this project (e.g., socket.lua). It's highly recommended to use vim.json.decode for parsing the response to improve robustness and maintainability.
local function parse_json(json_str)
local ok, result = pcall(vim.json.decode, json_str)
if ok and type(result) == 'table' and result.isLatest ~= nil and result.latestVersion and result.version then
return result
end
return nil
end
| if status and status.version then | ||
| version.check_version(status.version) | ||
| end |
There was a problem hiding this comment.
The error returned from socket.get_status is not handled. If the call fails, the version check is silently skipped. While this prevents a crash, it would be beneficial to log the error when in debug mode to aid in troubleshooting connection issues. This would make the behavior consistent with other error handling in the codebase.
if status and status.version then
version.check_version(status.version)
elseif err and config.get('debug') then
vim.notify('[shelltime] Failed to get status for version check: ' .. err, vim.log.levels.DEBUG)
end
| ---@return string Encoded string | ||
| local function url_encode(str) | ||
| if str then | ||
| str = string.gsub(str, '\n', '\r\n') |
There was a problem hiding this comment.
The handling of newline characters (\n) is incorrect for URL encoding. Newlines should be percent-encoded as %0A, not converted to \r\n which is used for HTTP headers. While version strings are unlikely to contain newlines, this is still a bug in the implementation that could cause issues.
str = string.gsub(str, '\n', '%%0A')
Replace manual regex-based JSON parser with Neovim's built-in vim.json.decode for more robust handling of edge cases. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review: CLI Version Check FeatureOverall AssessmentThis PR adds a valuable feature for notifying users about CLI updates. The implementation is generally solid, but there are several important issues that should be addressed before merging. Critical Issues1. Missing Test CoverageThe PR adds 146 lines of new code in version.lua but includes no tests. Based on the project testing conventions (all other modules have corresponding *_spec.lua files), you should add tests/version_spec.lua. Recommended test coverage:
2. Security: Command Injection in Update CommandLocation: lua/shelltime/version.lua:128 The update command is constructed by concatenating user-configurable input (web_endpoint from config). If a malicious config contains a crafted webEndpoint value, it could lead to command injection. Recommended fix: Validate the web_endpoint format before using it in the shell command. 3. Error Handling: Silent FailuresLocation: lua/shelltime/version.lua:74-120 The curl job does not handle several failure modes:
4. Race Condition in sender.luaLocation: lua/shelltime/sender.lua:73 The version check callback could fire after the plugin is stopped. Add state tracking to prevent this race condition. Code Quality Issues5. Inconsistent Error Handling PatternThe codebase uses pcall() for error handling (per CLAUDE.md), but version.lua uses if-checks instead. Consider wrapping risky operations consistently. 6. URL Encoding ImplementationLocation: lua/shelltime/version.lua:16-24 The url_encode() function has issues:
7. Magic NumbersThe curl timeout is hardcoded as 5 seconds. This should be a configurable constant at the module level. 8. Blocking NotificationThe clipboard operation vim.fn.setreg could block if the clipboard is unavailable. Consider wrapping in pcall(). Positive Aspects
Recommendations SummaryMust fix before merge:
Should fix: Nice to have: Great work on this feature! The core functionality is solid. Address the security and testing concerns, and this will be ready to merge. |
Add tests for check_version and show_update_warning functions covering: - Configuration handling (missing endpoints, already shown warning) - API call construction (URL encoding, headers, timeout) - Success responses (version latest vs update available) - Error handling (curl failures, invalid JSON, empty responses) - Session state (warning shown once per session) - Clipboard functionality Also add _set_for_testing helper to config module for test isolation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review - PR #12: Version Check FeatureSummaryThis PR adds CLI version checking functionality that runs on extension startup. The implementation is well-structured with comprehensive test coverage (584 test lines). Overall, the code follows the project conventions and is production-ready with minor improvements suggested below. Strengths
Code Quality Issues1. Security: Command Injection Risk (High Priority)Location: lua/shelltime/version.lua:129 The web_endpoint is user-configurable and directly concatenated into a shell command without validation. A malicious config could inject arbitrary commands. Recommendations: Add URL validation to ensure web_endpoint is a valid HTTPS URL, sanitize command construction, add validation in config.lua 2. Security: Arbitrary URL in curl (Medium Priority)Location: lua/shelltime/version.lua:74 The api_endpoint is not validated. An attacker could set it to file:///etc/passwd or other protocols. Recommendation: Validate that both api_endpoint and web_endpoint start with https:// when loaded 3. Logic Issue: Session StateLocation: lua/shelltime/version.lua:8 The has_shown_warning flag persists across module reloads. Users won't see warnings after updating daemon until Neovim restarts. Best Practices & Improvements
Test CoverageStrengths: 584 lines of tests for 146 lines of code (4:1 ratio) - excellent! Missing: Version string edge cases, concurrent calls, jobstart failure handling Security Summary
ConclusionSolid implementation with excellent test coverage. Main concerns are security-related around URL validation and command construction. Recommended Before Merge:
Great work overall! The async implementation and test coverage are particularly commendable. |
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
Summary
apiEndpointandwebEndpointfrom config fileTest plan
apiEndpointandwebEndpointin~/.shelltime/config.yaml🤖 Generated with Claude Code