fix: shell test coverage and llm-update.sh formatting - #805
Conversation
- Add scripts/llm-update.sh to coverage_spec.sh tracking list - Add llm_update_spec.sh with tests for the new script - Quote associative array keys to prevent shfmt from mangling paths - Apply formatter fixes to cliproxyapi config YAML files Entire-Checkpoint: 0e630aa734fa
|
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 addresses shell test failures and improves code quality by enhancing test coverage for a new script, correcting formatting issues in a shell script to prevent 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
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request introduces formatting refinements to YAML configuration files, makes minor adjustments to the llm-update shell script (quoting and output messaging), and adds comprehensive test coverage for the llm-update script, along with updating the test coverage manifest. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
✨ 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 |
Mesa DescriptionTL;DRThis PR fixes shell test coverage for What changed?
Description generated by Mesa. Update settings |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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 improves shell script test coverage and formatting, especially for llm-update.sh. A high-severity command injection vulnerability was found in scripts/llm-update.sh due to unsafe sed usage with unvalidated input from models.json, requiring immediate attention to prevent Remote Code Execution (RCE). Further suggestions include fixing a potential bug in llm-update.sh related to unquoted variables and enhancing new tests to check for runtime behavior. The formatting changes are positive.
| echo "SKIP: $src" | ||
| continue | ||
| } | ||
| sed "${sed_args[@]}" "$ROOT/$src" >"$ROOT/$dst" |
There was a problem hiding this comment.
The sed command is vulnerable to command injection because it uses unvalidated input from models.json to construct substitution expressions. If a value in models.json contains the delimiter | followed by the e flag (e.g., value|e;s|a|b), GNU sed will execute the replacement as a shell command. This could lead to Remote Code Execution (RCE) if models.json is influenced by an attacker (e.g., via a malicious Pull Request).
To remediate this, ensure that all values from models.json are properly escaped before being added to sed_args. For example, you can escape the pipe character using Bash string replacement: ${value//|/\\|}. This should be applied to the value, pretty, and nondot variables where they are added to the sed_args array.
| MODELS="$ROOT/models.json" | ||
|
|
||
| [[ -f "$MODELS" ]] || { echo "ERROR: models.json not found" >&2; exit 1; } | ||
| [[ -f $MODELS ]] || { |
There was a problem hiding this comment.
The $MODELS variable should be quoted to prevent issues with word splitting if the file path contains spaces. It's a good practice to always quote variables that hold file paths, as this will prevent bugs if the script is run from a directory with spaces in its name.
| [[ -f $MODELS ]] || { | |
| [[ -f "$MODELS" ]] || { |
| It 'exits if models.json is missing' | ||
| When run bash -c "grep 'models.json not found' '$SCRIPT'" | ||
| The output should include 'ERROR' | ||
| End |
There was a problem hiding this comment.
This test only verifies that the error message string exists within the script file, but it doesn't test the actual runtime behavior. A more robust test would execute the script in a state where models.json is absent and assert that the script exits with a failure status code and prints the expected error message to stderr. This would provide stronger guarantees about the script's correctness.
Here's an example of how you could write a functional test for this case using shellspec features:
It 'exits if models.json is missing'
# Temporarily move models.json to simulate its absence
Before 'mv "$PWD/models.json" "$PWD/models.json.bak"'
After 'mv "$PWD/models.json.bak" "$PWD/models.json"'
When run script "$SCRIPT"
The status should be failure
The stderr should include 'ERROR: models.json not found'
EndThis approach of testing behavior over implementation details could be applied to other tests in this file to make them more effective.
There was a problem hiding this comment.
Pull request overview
This PR adds shell test coverage for the scripts/llm-update.sh script and applies formatting fixes to ensure compatibility with the shfmt formatter. The changes fix a shell test failure by adding the new script to the coverage tracking list and prevent shfmt from mangling file paths by quoting associative array keys.
Changes:
- Added
spec/llm_update_spec.shwith grep-based tests verifying script structure and functionality - Added
scripts/llm-update.shto the coverage tracking list inspec/coverage_spec.sh - Quoted associative array keys in
scripts/llm-update.shto prevent shfmt from splitting paths with slashes - Applied shfmt formatting (multi-line error handling blocks, removed spaces before redirection operators)
- Removed extraneous blank lines in cliproxyapi YAML configuration files
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| spec/llm_update_spec.sh | New test file with grep-based tests for script properties, models.json handling, template processing, jq pretty-printing, and placeholder generation |
| spec/coverage_spec.sh | Added scripts/llm-update.sh to the tracked scripts list in alphabetical order |
| scripts/llm-update.sh | Quoted associative array keys, reformatted error handling blocks to multi-line format, removed space before output redirection operator |
| config/cliproxyapi/config.tpl.yaml | Removed two extraneous blank lines for cleaner formatting |
| config/cliproxyapi/config.template.yaml | Removed two extraneous blank lines to match template file formatting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MODELS="$ROOT/models.json" | ||
|
|
||
| [[ -f "$MODELS" ]] || { echo "ERROR: models.json not found" >&2; exit 1; } | ||
| [[ -f $MODELS ]] || { |
There was a problem hiding this comment.
The variable $MODELS should be quoted in the test to be consistent with line 63 and to follow shell best practices. While the [[ ]] construct is more forgiving than [ ], it's safer and more consistent to quote variables to prevent potential issues with special characters or whitespace in paths.
| [[ -f $MODELS ]] || { | |
| [[ -f "$MODELS" ]] || { |
Changes
scripts/llm-update.shtocoverage_spec.shtracking list (fixes shell-test failure)spec/llm_update_spec.shwith tests for the new scriptllm-update.shto prevent shfmt from mangling file paths (e.g.config/openclaw/...→config / openclaw / ...)Testing
make shell-test— 491 examples, 0 failuresmake format— 0 files changed (stable)Generated with Claude Code
Summary by cubic
Fix shell test coverage and shfmt formatting issues in llm-update.sh, and add a focused test suite for the script. Shell tests now pass and formatting is stable.
Written for commit 97f10c1. Summary will update on new commits.