From 77f795192e0b9b80613ae4a7aea539ea2684b49c Mon Sep 17 00:00:00 2001 From: John Thomas <157211333+johnthomasdev@users.noreply.github.com> Date: Sat, 6 Sep 2025 04:22:10 +0530 Subject: [PATCH] refactor: Extract path resolution logic into helper function (#6910) --- packages/core/src/core/prompts.test.ts | 152 ++++++++++++++++++++++++- packages/core/src/core/prompts.ts | 121 +++++++++++++------- 2 files changed, 233 insertions(+), 40 deletions(-) diff --git a/packages/core/src/core/prompts.test.ts b/packages/core/src/core/prompts.test.ts index 99cf76c4f7..e4e315c322 100644 --- a/packages/core/src/core/prompts.test.ts +++ b/packages/core/src/core/prompts.test.ts @@ -5,7 +5,7 @@ */ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { getCoreSystemPrompt } from './prompts.js'; +import { getCoreSystemPrompt, resolvePathFromEnv } from './prompts.js'; import { isGitRepository } from '../utils/gitUtils.js'; import fs from 'node:fs'; import os from 'node:os'; @@ -270,3 +270,153 @@ describe('Core System Prompt (prompts.ts)', () => { }); }); }); + +describe('resolvePathFromEnv helper function', () => { + beforeEach(() => { + vi.resetAllMocks(); + }); + + describe('when envVar is undefined, empty, or whitespace', () => { + it('should return null for undefined', () => { + const result = resolvePathFromEnv(undefined); + expect(result).toEqual({ + isSwitch: false, + value: null, + isDisabled: false, + }); + }); + + it('should return null for empty string', () => { + const result = resolvePathFromEnv(''); + expect(result).toEqual({ + isSwitch: false, + value: null, + isDisabled: false, + }); + }); + + it('should return null for whitespace only', () => { + const result = resolvePathFromEnv(' \n\t '); + expect(result).toEqual({ + isSwitch: false, + value: null, + isDisabled: false, + }); + }); + }); + + describe('when envVar is a boolean-like string', () => { + it('should handle "0" as disabled switch', () => { + const result = resolvePathFromEnv('0'); + expect(result).toEqual({ + isSwitch: true, + value: '0', + isDisabled: true, + }); + }); + + it('should handle "false" as disabled switch', () => { + const result = resolvePathFromEnv('false'); + expect(result).toEqual({ + isSwitch: true, + value: 'false', + isDisabled: true, + }); + }); + + it('should handle "1" as enabled switch', () => { + const result = resolvePathFromEnv('1'); + expect(result).toEqual({ + isSwitch: true, + value: '1', + isDisabled: false, + }); + }); + + it('should handle "true" as enabled switch', () => { + const result = resolvePathFromEnv('true'); + expect(result).toEqual({ + isSwitch: true, + value: 'true', + isDisabled: false, + }); + }); + + it('should be case-insensitive for boolean values', () => { + expect(resolvePathFromEnv('FALSE')).toEqual({ + isSwitch: true, + value: 'false', + isDisabled: true, + }); + expect(resolvePathFromEnv('TRUE')).toEqual({ + isSwitch: true, + value: 'true', + isDisabled: false, + }); + }); + }); + + describe('when envVar is a file path', () => { + it('should resolve absolute paths', () => { + const result = resolvePathFromEnv('/absolute/path/file.txt'); + expect(result).toEqual({ + isSwitch: false, + value: path.resolve('/absolute/path/file.txt'), + isDisabled: false, + }); + }); + + it('should resolve relative paths', () => { + const result = resolvePathFromEnv('relative/path/file.txt'); + expect(result).toEqual({ + isSwitch: false, + value: path.resolve('relative/path/file.txt'), + isDisabled: false, + }); + }); + + it('should expand tilde to home directory', () => { + const homeDir = '/Users/test'; + vi.spyOn(os, 'homedir').mockReturnValue(homeDir); + + const result = resolvePathFromEnv('~/documents/file.txt'); + expect(result).toEqual({ + isSwitch: false, + value: path.resolve(path.join(homeDir, 'documents/file.txt')), + isDisabled: false, + }); + }); + + it('should handle standalone tilde', () => { + const homeDir = '/Users/test'; + vi.spyOn(os, 'homedir').mockReturnValue(homeDir); + + const result = resolvePathFromEnv('~'); + expect(result).toEqual({ + isSwitch: false, + value: path.resolve(homeDir), + isDisabled: false, + }); + }); + + it('should handle os.homedir() errors gracefully', () => { + vi.spyOn(os, 'homedir').mockImplementation(() => { + throw new Error('Cannot resolve home directory'); + }); + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + const result = resolvePathFromEnv('~/documents/file.txt'); + expect(result).toEqual({ + isSwitch: false, + value: null, + isDisabled: false, + }); + expect(consoleSpy).toHaveBeenCalledWith( + 'Could not resolve home directory for path: ~/documents/file.txt', + expect.any(Error), + ); + + consoleSpy.mockRestore(); + }); + }); +}); diff --git a/packages/core/src/core/prompts.ts b/packages/core/src/core/prompts.ts index b4b9a979a4..3de97bec77 100644 --- a/packages/core/src/core/prompts.ts +++ b/packages/core/src/core/prompts.ts @@ -19,29 +19,78 @@ import process from 'node:process'; import { isGitRepository } from '../utils/gitUtils.js'; import { MemoryTool, GEMINI_CONFIG_DIR } from '../tools/memoryTool.js'; +export function resolvePathFromEnv(envVar?: string): { + isSwitch: boolean; + value: string | null; + isDisabled: boolean; +} { + // Handle the case where the environment variable is not set, empty, or just whitespace. + const trimmedEnvVar = envVar?.trim(); + if (!trimmedEnvVar) { + return { isSwitch: false, value: null, isDisabled: false }; + } + + const lowerEnvVar = trimmedEnvVar.toLowerCase(); + // Check if the input is a common boolean-like string. + if (['0', 'false', '1', 'true'].includes(lowerEnvVar)) { + // If so, identify it as a "switch" and return its value. + const isDisabled = ['0', 'false'].includes(lowerEnvVar); + return { isSwitch: true, value: lowerEnvVar, isDisabled }; + } + + // If it's not a switch, treat it as a potential file path. + let customPath = trimmedEnvVar; + + // Safely expand the tilde (~) character to the user's home directory. + if (customPath.startsWith('~/') || customPath === '~') { + try { + const home = os.homedir(); // This is the call that can throw an error. + if (customPath === '~') { + customPath = home; + } else { + customPath = path.join(home, customPath.slice(2)); + } + } catch (error) { + // If os.homedir() fails, we catch the error instead of crashing. + console.warn( + `Could not resolve home directory for path: ${trimmedEnvVar}`, + error, + ); + // Return null to indicate the path resolution failed. + return { isSwitch: false, value: null, isDisabled: false }; + } + } + + // Return it as a non-switch with the fully resolved absolute path. + return { + isSwitch: false, + value: path.resolve(customPath), + isDisabled: false, + }; +} + export function getCoreSystemPrompt(userMemory?: string): string { - // if GEMINI_SYSTEM_MD is set (and not 0|false), override system prompt from file - // default path is .gemini/system.md but can be modified via custom path in GEMINI_SYSTEM_MD + // A flag to indicate whether the system prompt override is active. let systemMdEnabled = false; + // The default path for the system prompt file. This can be overridden. let systemMdPath = path.resolve(path.join(GEMINI_CONFIG_DIR, 'system.md')); - const systemMdVar = process.env['GEMINI_SYSTEM_MD']; - if (systemMdVar) { - const systemMdVarLower = systemMdVar.toLowerCase(); - if (!['0', 'false'].includes(systemMdVarLower)) { - systemMdEnabled = true; // enable system prompt override - if (!['1', 'true'].includes(systemMdVarLower)) { - let customPath = systemMdVar; - if (customPath.startsWith('~/')) { - customPath = path.join(os.homedir(), customPath.slice(2)); - } else if (customPath === '~') { - customPath = os.homedir(); - } - systemMdPath = path.resolve(customPath); // use custom path from GEMINI_SYSTEM_MD - } - // require file to exist when override is enabled - if (!fs.existsSync(systemMdPath)) { - throw new Error(`missing system prompt file '${systemMdPath}'`); - } + // Resolve the environment variable to get either a path or a switch value. + const systemMdResolution = resolvePathFromEnv( + process.env['GEMINI_SYSTEM_MD'], + ); + + // Proceed only if the environment variable is set and is not disabled. + if (systemMdResolution.value && !systemMdResolution.isDisabled) { + systemMdEnabled = true; + + // We update systemMdPath to this new custom path. + if (!systemMdResolution.isSwitch) { + systemMdPath = systemMdResolution.value; + } + + // require file to exist when override is enabled + if (!fs.existsSync(systemMdPath)) { + throw new Error(`missing system prompt file '${systemMdPath}'`); } } const basePrompt = systemMdEnabled @@ -266,25 +315,19 @@ Your core function is efficient and safe assistance. Balance extreme conciseness `.trim(); // if GEMINI_WRITE_SYSTEM_MD is set (and not 0|false), write base system prompt to file - const writeSystemMdVar = process.env['GEMINI_WRITE_SYSTEM_MD']; - if (writeSystemMdVar) { - const writeSystemMdVarLower = writeSystemMdVar.toLowerCase(); - if (!['0', 'false'].includes(writeSystemMdVarLower)) { - if (['1', 'true'].includes(writeSystemMdVarLower)) { - fs.mkdirSync(path.dirname(systemMdPath), { recursive: true }); - fs.writeFileSync(systemMdPath, basePrompt); // write to default path, can be modified via GEMINI_SYSTEM_MD - } else { - let customPath = writeSystemMdVar; - if (customPath.startsWith('~/')) { - customPath = path.join(os.homedir(), customPath.slice(2)); - } else if (customPath === '~') { - customPath = os.homedir(); - } - const resolvedPath = path.resolve(customPath); - fs.mkdirSync(path.dirname(resolvedPath), { recursive: true }); - fs.writeFileSync(resolvedPath, basePrompt); // write to custom path from GEMINI_WRITE_SYSTEM_MD - } - } + const writeSystemMdResolution = resolvePathFromEnv( + process.env['GEMINI_WRITE_SYSTEM_MD'], + ); + + // Check if the feature is enabled. This proceeds only if the environment + // variable is set and is not explicitly '0' or 'false'. + if (writeSystemMdResolution.value && !writeSystemMdResolution.isDisabled) { + const writePath = writeSystemMdResolution.isSwitch + ? systemMdPath + : writeSystemMdResolution.value; + + fs.mkdirSync(path.dirname(writePath), { recursive: true }); + fs.writeFileSync(writePath, basePrompt); } const memorySuffix =