Skip to content

ci: auto-generate skills on toolbox version updates#164

Open
twishabansal wants to merge 1 commit into
mainfrom
ci/auto-generate-skills
Open

ci: auto-generate skills on toolbox version updates#164
twishabansal wants to merge 1 commit into
mainfrom
ci/auto-generate-skills

Conversation

@twishabansal

Copy link
Copy Markdown
Contributor

@twishabansal twishabansal requested review from a team as code owners June 23, 2026 16:18

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new Bash script, .github/scripts/generate_skills.sh, designed to validate and generate AlloyDB PostgreSQL skills by fetching upstream configurations and executing the toolbox SDK. The review feedback focuses on improving the script's robustness and safety. Key recommendations include adopting set -euo pipefail with proper unbound variable handling, redirecting error messages to stderr (especially within command substitutions), replacing positional parameter shifting (set -- and shift) with standard for loops to avoid clobbering global parameters, using glob matching instead of regex operators, and adding the --yes flag to npx to prevent interactive prompts in CI/CD environments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

# Fetch the upstream source of truth YAML for this specific version
RAW_URL="https://raw.githubusercontent.com/googleapis/mcp-toolbox/v${VERSION}/internal/prebuiltconfigs/tools/alloydb-postgres.yaml"
echo "Fetching upstream config from: $RAW_URL"
UPSTREAM_YAML=$(curl -sL --fail "$RAW_URL" || { echo "Error: Could not fetch upstream YAML for v$VERSION"; exit 1; })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Because this echo is inside a command substitution $(...), its standard output is captured into the UPSTREAM_YAML variable rather than being printed to the terminal. If curl fails, the script will exit due to set -e, and the error message will be completely lost/silenced. Redirecting the message to stderr (>&2) ensures it is visible in the console/logs.

Suggested change
UPSTREAM_YAML=$(curl -sL --fail "$RAW_URL" || { echo "Error: Could not fetch upstream YAML for v$VERSION"; exit 1; })
UPSTREAM_YAML=$(curl -sL --fail "$RAW_URL" || { echo "Error: Could not fetch upstream YAML for v$VERSION" >&2; exit 1; })


echo "Generating skill: $SKILL_NAME..."

npx "@toolbox-sdk/server@${VERSION}" --prebuilt alloydb-postgres skills-generate \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

In non-interactive environments like CI/CD pipelines, npx may prompt the user to install the package if it is not already cached (e.g., Need to install the following packages... Ok to proceed? (y)). This will cause the build to hang or fail. Adding the --yes flag forces npx to automatically accept the installation prompt.

Suggested change
npx "@toolbox-sdk/server@${VERSION}" --prebuilt alloydb-postgres skills-generate \
npx --yes "@toolbox-sdk/server@${VERSION}" --prebuilt alloydb-postgres skills-generate \

Comment on lines +1 to +8
#!/bin/bash
set -e

# Ensure VERSION is passed from the environment
if [ -z "$VERSION" ]; then
echo "Error: VERSION environment variable is not set."
exit 1
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

It is highly recommended to use set -euo pipefail for robust error handling in Bash scripts. Additionally, when set -u is active, referencing $VERSION directly when it is not set will cause the script to exit immediately with an unbound variable error. Using ${VERSION:-} prevents this and allows the custom error message to be printed. Also, redirecting the error message to >&2 ensures it is printed to stderr.

Suggested change
#!/bin/bash
set -e
# Ensure VERSION is passed from the environment
if [ -z "$VERSION" ]; then
echo "Error: VERSION environment variable is not set."
exit 1
fi
#!/bin/bash
set -euo pipefail
# Ensure VERSION is passed from the environment
if [ -z "${VERSION:-}" ]; then
echo "Error: VERSION environment variable is not set." >&2
exit 1
fi

Comment on lines +38 to +45
# Dynamically build the SUPPORTED_TOOLSETS array from the SKILLS array.
# We use 'set --' to process the array in chunks without index arithmetic.
SUPPORTED_TOOLSETS=()
set -- "${SKILLS[@]}"
while [ $# -gt 0 ]; do
SUPPORTED_TOOLSETS+=("$1")
shift 2
done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Instead of using set -- and shift which overwrites and clobbers the script's global positional parameters (especially problematic if the script is ever sourced), you can use a standard for loop with index arithmetic to iterate over the array in pairs. This is cleaner and safer.

Suggested change
# Dynamically build the SUPPORTED_TOOLSETS array from the SKILLS array.
# We use 'set --' to process the array in chunks without index arithmetic.
SUPPORTED_TOOLSETS=()
set -- "${SKILLS[@]}"
while [ $# -gt 0 ]; do
SUPPORTED_TOOLSETS+=("$1")
shift 2
done
# Dynamically build the SUPPORTED_TOOLSETS array from the SKILLS array.
SUPPORTED_TOOLSETS=()
for ((i=0; i<${#SKILLS[@]}; i+=2)); do
SUPPORTED_TOOLSETS+=("${SKILLS[i]}")
done

for upstream_tool in $UPSTREAM_TOOLSETS; do
if [ -z "$upstream_tool" ] || [ "$upstream_tool" == "-" ]; then continue; fi

if [[ ! " ${SUPPORTED_TOOLSETS[*]} " =~ " ${upstream_tool} " ]]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using the regular expression operator =~ with an unquoted variable on the right side treats the variable's content as a regex pattern. If upstream_tool contains any regex special characters, it could lead to unexpected matching behavior. Using standard glob matching (== *pattern*) is safer, faster, and avoids regex interpretation.

Suggested change
if [[ ! " ${SUPPORTED_TOOLSETS[*]} " =~ " ${upstream_tool} " ]]; then
if [[ ! " ${SUPPORTED_TOOLSETS[*]} " == *" ${upstream_tool} "* ]]; then

Comment on lines +115 to +119
set -- "${SKILLS[@]}"
while [ $# -gt 0 ]; do
generate_skill "$1" "$2"
shift 2
done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Similar to the validation loop, using a standard for loop with index arithmetic is cleaner and avoids clobbering the global positional parameters.

Suggested change
set -- "${SKILLS[@]}"
while [ $# -gt 0 ]; do
generate_skill "$1" "$2"
shift 2
done
for ((i=0; i<${#SKILLS[@]}; i+=2)); do
generate_skill "${SKILLS[i]}" "${SKILLS[i+1]}"
done

@twishabansal twishabansal added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge Indicates a pull request not ready for merge, due to either quality or timing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant