refactor(skills): modularize generated skill template and improve env handling#2965
refactor(skills): modularize generated skill template and improve env handling#2965twishabansal merged 17 commits intomainfrom
Conversation
Simplify the logic used to check for duplicate entries in OptionalEnvVars by using the slices.Contains idiom. This improves readability and aligns with standard Go 1.21+ patterns. Also fix template syntax error.
Simplify the logic used to check for duplicate entries in OptionalEnvVars by using the slices.Contains idiom. This improves readability and aligns with standard Go 1.21+ patterns. Also fix template syntax error.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the generated script template in generator.go by modularizing the environment setup and execution logic into mergeEnvVars, prepareEnvironment, and main functions. It also updates the test suite and changes the mode identifier from 'binary' to 'bin'. A critical scope error was identified in the refactored template where the child process variable was declared using const inside conditional blocks, making it inaccessible to event listeners. The reviewer provided a suggestion to fix this by using function-scoped declaration and corrected inconsistent indentation throughout the template.
I am having trouble creating individual review comments. Click here to see my feedback.
cmd/internal/skills/generator.go (137-232)
The refactored script template has two significant issues:
- Scope Error (Critical): The
childvariable is declared usingconstinside theifandelseblocks of the template. Sinceconstis block-scoped in JavaScript, thechildvariable is not accessible outside of those blocks (e.g., at line 222). This will result in aReferenceError: child is not definedwhen the script is executed. - Inconsistent Indentation (Medium): The template content mixes tabs and spaces, and the indentation levels are inconsistent (e.g., jumping from 1 tab to 3 tabs, or mixing 4 spaces with tabs). For the embedded JavaScript, it is recommended to use a consistent indentation style (e.g., 4 spaces).
I have provided a suggestion that fixes the scope issue by declaring let child at the top of the main() function and cleans up the indentation throughout the template.
function mergeEnvVars(env) {
const envPath = path.resolve(__dirname, '../../../.env');
if (fs.existsSync(envPath)) {
const envContent = fs.readFileSync(envPath, 'utf-8');
envContent.split('\n').forEach(line => {
const trimmed = line.trim();
if (trimmed && !trimmed.startsWith('#')) {
const splitIdx = trimmed.indexOf('=');
if (splitIdx !== -1) {
const key = trimmed.slice(0, splitIdx).trim();
let value = trimmed.slice(splitIdx + 1).trim();
value = value.replace(/(^['"]|['"]$)/g, '');
if (env[key] === undefined) {
env[key] = value;
}
}
}
});
}
}
function prepareEnvironment() {
let env = { ...process.env };
let userAgent = "skills";
if (process.env.GEMINI_CLI === '1') {
userAgent = "skills-geminicli";
mergeEnvVars(env);
}
{{if .OptionalVars}}
OPTIONAL_VARS_TO_OMIT_IF_EMPTY.forEach(varName => {
if (env[varName] === '') {
delete env[varName];
}
});
{{end}}
return { env, userAgent };
}
/**
* Main execution function.
*/
function main() {
const { env, userAgent } = prepareEnvironment();
const args = process.argv.slice(2);
let child;
{{if eq .InvocationMode "npx"}}
const command = os.platform() === 'win32' ? 'npx.cmd' : 'npx';
const processedArgs = os.platform() === 'win32' ? args.map(arg => arg.includes('"') ? '"' + arg.replace(/"/g, '""') + '"' : arg) : args;
const npxArgs = ["--yes", "@toolbox-sdk/server@{{.ToolboxVersion}}", "--log-level", "error", ...configArgs, "invoke", toolName, "--user-agent-metadata", userAgent, ...processedArgs];
child = spawn(command, npxArgs, { shell: os.platform() === 'win32', stdio: 'inherit', env });
{{else}}
function getToolboxPath() {
if (process.env.GEMINI_CLI === '1') {
const ext = process.platform === 'win32' ? '.exe' : '';
const localPath = path.resolve(__dirname, '../../../toolbox' + ext);
if (fs.existsSync(localPath)) {
return localPath;
}
}
try {
const checkCommand = process.platform === 'win32' ? 'where toolbox' : 'which toolbox';
const globalPath = execSync(checkCommand, { stdio: 'pipe', encoding: 'utf-8' }).trim();
if (globalPath) {
return globalPath.split('\n')[0].trim();
}
throw new Error("Toolbox binary not found");
} catch (e) {
throw new Error("Toolbox binary not found");
}
}
let toolboxBinary;
try {
toolboxBinary = getToolboxPath();
} catch (err) {
console.error("Error:", err.message);
process.exit(1);
}
const toolboxArgs = ["--log-level", "error", ...configArgs, "invoke", toolName, "--user-agent-metadata", userAgent, ...args];
child = spawn(toolboxBinary, toolboxArgs, { stdio: 'inherit', env });
{{end}}
child.on('close', (code) => {
process.exit(code);
});
child.on('error', (err) => {
console.error("Error executing toolbox:", err);
process.exit(1);
});
}
main();|
🧨 Preview deployments removed. Cloudflare Pages environments for |
…rove env handling (googleapis#2965) ## Description > Should include a concise description of the changes (bug or feature), it's > impact, along with a summary of the solution ## PR Checklist > Thank you for opening a Pull Request! Before submitting your PR, there are a > few things you can do to make sure it goes smoothly: - [ ] Make sure you reviewed [CONTRIBUTING.md](https://github.com/googleapis/genai-toolbox/blob/main/CONTRIBUTING.md) - [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/genai-toolbox/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) - [ ] Make sure to add `!` if this involve a breaking change 🛠️ Fixes #<issue_number_goes_here> --------- Co-authored-by: Haoyu Wang <whaoyu@google.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> 4655bcd
Description
PR Checklist
CONTRIBUTING.md
bug/issue
before writing your code! That way we can discuss the change, evaluate
designs, and agree on the general idea
!if this involve a breaking change🛠️ Fixes #<issue_number_goes_here>