fix: shellcheck - add quotes and remove unused variable#55
fix: shellcheck - add quotes and remove unused variable#55vlordier wants to merge 1 commit intoLiquid4All:mainfrom
Conversation
- smoke-test.sh: quote $servers in two npx tsx calls - validate-mcp-servers.sh: remove unused implemented_tools variable No more shellcheck errors or warnings.
There was a problem hiding this comment.
Pull request overview
This PR addresses shellcheck warnings in two shell scripts within the examples/localcowork/scripts/ directory. It also normalizes indentation from spaces to tabs throughout both files.
Changes:
smoke-test.sh: Quotes$serversin twonpx tsxinvocations to fix SC2086, and normalizes indentation from spaces to tabs.validate-mcp-servers.sh: Removes the unusedimplemented_toolsvariable (SC2034), and normalizes indentation from spaces to tabs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
examples/localcowork/scripts/smoke-test.sh |
Indentation normalized to tabs; $servers quoted in two npx tsx calls to silence SC2086 |
examples/localcowork/scripts/validate-mcp-servers.sh |
Indentation normalized to tabs; removed unused implemented_tools variable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Run the contract validator if it exists | ||
| if [ -f "$SMOKE_DIR/contract-validator.ts" ]; then | ||
| local output | ||
| if output=$(cd "$PROJECT_ROOT" && npx tsx "$SMOKE_DIR/contract-validator.ts" "$servers" 2>&1); then |
There was a problem hiding this comment.
Bug: Quoting "$servers" here passes the entire space-separated list of server names as a single argument to contract-validator.ts. The find_implemented_servers function (line 148) returns a space-separated string like "filesystem calendar contacts". Without quotes, shell word-splitting correctly expands this into separate positional arguments. With quotes, the TypeScript script receives one argument containing all names joined by spaces.
Note that just 15 lines below (line 214), $servers is intentionally left unquoted in for server_name in $servers to get the correct word-splitting behavior — these two usages should be consistent.
The proper shellcheck-compliant fix would be to convert servers to an array: use read -ra servers <<< "$(find_implemented_servers)" and then pass "${servers[@]}" to the npx call and the for loop.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| # Run the health checker if it exists | ||
| if [ -f "$SMOKE_DIR/server-health.ts" ]; then | ||
| local output | ||
| if output=$(cd "$PROJECT_ROOT" && npx tsx "$SMOKE_DIR/server-health.ts" "$servers" 2>&1); then |
There was a problem hiding this comment.
Bug: Same issue as in run_contract_tests — quoting "$servers" passes the entire space-separated list of server names as a single argument to server-health.ts instead of separate arguments. This will break when there are multiple implemented servers. See the comment on line 199 for the recommended fix using arrays.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@copilot open a new pull request to apply changes based on the comments in this thread |
- Add GitHub Actions workflow for shellcheck - Add shellcheck validation to pre-commit hook for staged .sh files This improves on PR Liquid4All#55 by adding automated shellcheck validation.
Summary
No more shellcheck errors or warnings in these scripts.