Skip to content

fix(config): load home .env vars before settings ${VAR} resolution (#4466) #4474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 6 commits into from
May 31, 2026
Merged

fix(config): load home .env vars before settings ${VAR} resolution (#4466) #4474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3c8c7da
fix(config): load home .env vars before settings ${VAR} resolution (#…
kagura-agent May 24, 2026
7f4c1a6
test: add env var resolution from .env file test (#4466)
kagura-agent May 24, 2026
80ebd94
fix(config): use customEnv fallback for home .env resolution (#4466)
kagura-agent May 24, 2026
1137083
fix: align .env precedence and fix test mock
kagura-agent May 25, 2026
08e3de9
test: add .env fallback, precedence, and error path tests
kagura-agent May 27, 2026
c6a4a30
docs: clarify customEnv precedence comment per reviewer suggestion
kagura-agent May 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
250 changes: 250 additions & 0 deletions packages/cli/src/config/settings.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2147,6 +2147,256 @@ describe('Settings Loading and Merging', () => {
delete process.env['SHARED_VAR'];
});

it('should resolve ${VAR} in settings from home-level .env file (#4466)', () => {

@wenshao wenshao May 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Test coverage gaps — the single test covers only the happy path. Missing scenarios:

  1. No-override guard: No test asserts that a pre-existing process.env.MY_SECRET_TOKEN is NOT overridden by the .env file value. This is the core invariant (!Object.hasOwn(process.env, key)) and the most regression-prone logic.
  2. QWEN_HOME redirect: No test with QWEN_HOME set to a custom path — which would catch the Critical parent-dir .env bug flagged above.
  3. Error handling: No test for malformed .env content or readFileSync throwing.

Suggested addition for the no-override test:

it('should NOT override existing process.env vars from home .env', () => {
  process.env['MY_SECRET_TOKEN'] = 'from_shell';
  // ... same mock setup ...
  const settings = loadSettings(MOCK_WORKSPACE_DIR);
  expect(mcpServers?.['myServer']?.headers?.['Authorization']).toBe('Bearer from_shell');
  delete process.env['MY_SECRET_TOKEN'];
});

@wenshao wenshao May 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Missing test coverage for the ~/.env fallback positive path. Test 1 only exercises ~/.qwen/.env, and Test 3 only verifies the skip branch when QWEN_HOME is set. No test covers the scenario where QWEN_HOME is NOT set and the key is found via ~/.env (the second candidate).

Consider adding a test:

it('should fall back to ~/.env when key not in ~/.qwen/.env (#4466)', () => {
  // QWEN_HOME not set, ~/.qwen/.env missing or lacks target key,
  // ~/.env has the key → assert resolution
});

— qwen3.7-max via Qwen Code /review

const homeQwenEnvPath = path.join(
path.dirname(USER_SETTINGS_PATH),
'.env',
);
const userSettingsContent = {
mcpServers: {
myServer: {
headers: {
Authorization: 'Bearer ${MY_SECRET_TOKEN}',
},
},
},
};

(mockFsExistsSync as Mock).mockImplementation(
(p: fs.PathLike) => p === USER_SETTINGS_PATH || p === homeQwenEnvPath,
);
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(userSettingsContent);
if (p === homeQwenEnvPath)
return 'MY_SECRET_TOKEN=secret_from_dotenv';
return '{}';
},
);

delete process.env['MY_SECRET_TOKEN'];

const settings = loadSettings(MOCK_WORKSPACE_DIR);
const mcpServers = settings.merged.mcpServers as Record<
string,
{ headers?: Record<string, string> }
>;
expect(mcpServers?.['myServer']?.headers?.['Authorization']).toBe(
'Bearer secret_from_dotenv',
);

delete process.env['MY_SECRET_TOKEN'];
});

it('should not override process.env values with home .env file (#4466)', () => {
const homeQwenEnvPath = path.join(
path.dirname(USER_SETTINGS_PATH),
'.env',

@wenshao wenshao May 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The first-write-wins semantics (??=) between ~/.qwen/.env and ~/.env is not tested. If someone changes ??= to = (last-write-wins), no test would fail.

Consider adding a test where both files define the same key with different values:

it('should give ~/.qwen/.env precedence over ~/.env for duplicate keys (#4466)', () => {
  // ~/.qwen/.env: MY_KEY=from_qwen
  // ~/.env: MY_KEY=from_home
  // Assert: resolves to 'from_qwen'
});

— qwen3.7-max via Qwen Code /review

);
const userSettingsContent = {
mcpServers: {
myServer: {
headers: {
Authorization: 'Bearer ${MY_SECRET_TOKEN}',
},
},
},
};

(mockFsExistsSync as Mock).mockImplementation(
(p: fs.PathLike) => p === USER_SETTINGS_PATH || p === homeQwenEnvPath,
);
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(userSettingsContent);
if (p === homeQwenEnvPath) return 'MY_SECRET_TOKEN=from_dotenv';
return '{}';
},
);

process.env['MY_SECRET_TOKEN'] = 'from_process_env';

const settings = loadSettings(MOCK_WORKSPACE_DIR);
const mcpServers = settings.merged.mcpServers as Record<
string,
{ headers?: Record<string, string> }
>;
expect(mcpServers?.['myServer']?.headers?.['Authorization']).toBe(
'Bearer from_process_env',
);

delete process.env['MY_SECRET_TOKEN'];
});

it('should not search dirname(qwenDir)/.env when QWEN_HOME is set (#4466)', () => {
const customHome = '/custom/qwen/home';
process.env['QWEN_HOME'] = customHome;
const customSettingsPath = path.join(customHome, 'settings.json');
const dirnameEnvPath = path.join(path.dirname(customHome), '.env');
const userSettingsContent = {
mcpServers: {
myServer: {
headers: {
Authorization: 'Bearer ${MY_TOKEN}',
},
},

@wenshao wenshao May 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Error paths in getHomeEnvFallbackVars() are untested. The catch block handles EACCES, parse errors, etc., but no test exercises it. If the catch is accidentally removed, loadSettings would crash on unreadable .env files.

Consider adding a test where fs.readFileSync throws for the .env path and asserting loadSettings still succeeds with unresolved placeholders.

— qwen3.7-max via Qwen Code /review

},
};

(mockFsExistsSync as Mock).mockImplementation(
(p: fs.PathLike) => p === customSettingsPath || p === dirnameEnvPath,
);
(fs.readFileSync as Mock).mockImplementation(

@wenshao wenshao May 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] This test is broken and cascades into 7 pre-existing test failures (vitest run src/config/settings.test.ts reports 8 failed | 103 passed; on origin/main it is 0 failed | 108 passed).

Two defects:

  1. Wrong mock anchor. USER_SETTINGS_PATH is a module-load-time constant (/mock/home/user/.qwen/settings.json). But setting process.env['QWEN_HOME'] = '/custom/qwen/home' redirects Storage.getGlobalQwenDir() → loadSettings() internally looks for settings at /custom/qwen/home/settings.json, not USER_SETTINGS_PATH. The mock keyed to USER_SETTINGS_PATH never matches → mcpServers is undefined → the assertion fails.

  2. No try/finally cleanup. Because the assertion throws, the delete process.env['QWEN_HOME'] at the end never runs. vi.resetAllMocks() in beforeEach does not reset process.env. QWEN_HOME then leaks into every subsequent test in this describe block, breaking 7 of them (each sees redirected settings paths their mocks don't know about).

Separately, tsc/eslint report homeEnvPath is declared but never read at line 2237 (same test) — mockFsExistsSync returns true only for dirnameEnvPath, so the positive-path homeEnvPath is dead code in this test.

Suggested change
(fs.readFileSync as Mock).mockImplementation(
it('should not search dirname(qwenDir)/.env when QWEN_HOME is set (#4466)', async () => {
const customHome = '/custom/qwen/home';
const savedQwenHome = process.env['QWEN_HOME'];
process.env['QWEN_HOME'] = customHome;
try {
const homeEnvPath = path.join(customHome, '.env');
const dirnameEnvPath = path.join(path.dirname(customHome), '.env');
const userSettingsContent = {
mcpServers: {
myServer: {
headers: {
Authorization: 'Bearer ${MY_TOKEN}',
},
},
},
};
// Anchor the user-settings mock at the *redirected* path
// (Storage.getGlobalQwenDir() returns customHome when QWEN_HOME is set).
const redirectedSettingsPath = path.join(customHome, 'settings.json');
(mockFsExistsSync as Mock).mockImplementation(
(p: fs.PathLike) =>
p === redirectedSettingsPath || p === dirnameEnvPath,
);
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === redirectedSettingsPath)
return JSON.stringify(userSettingsContent);
if (p === dirnameEnvPath) return 'MY_TOKEN=should_not_be_found';
return '{}';
},
);
delete process.env['MY_TOKEN'];
const settings = loadSettings(MOCK_WORKSPACE_DIR);
const mcpServers = settings.merged.mcpServers as Record<
string,
{ headers?: Record<string, string> }
>;
expect(mcpServers?.['myServer']?.headers?.['Authorization']).toBe(
'Bearer ${MY_TOKEN}',
);
} finally {
delete process.env['MY_TOKEN'];
if (savedQwenHome === undefined) {
delete process.env['QWEN_HOME'];
} else {
process.env['QWEN_HOME'] = savedQwenHome;
}
}
});

— qwen3.7-max via Qwen Code /review

(p: fs.PathOrFileDescriptor) => {
if (p === customSettingsPath)
return JSON.stringify(userSettingsContent);
if (p === dirnameEnvPath) return 'MY_TOKEN=should_not_be_found';
return '{}';
},
);

delete process.env['MY_TOKEN'];

const settings = loadSettings(MOCK_WORKSPACE_DIR);
const mcpServers = settings.merged.mcpServers as Record<
string,
{ headers?: Record<string, string> }
>;
expect(mcpServers?.['myServer']?.headers?.['Authorization']).toBe(
'Bearer ${MY_TOKEN}',
);

delete process.env['MY_TOKEN'];
delete process.env['QWEN_HOME'];
});

it('should resolve ${VAR} from ~/.env when QWEN_HOME is not set (#4466)', () => {
const homeEnvPath = path.join(
path.dirname(path.dirname(USER_SETTINGS_PATH)),
'.env',
);
const userSettingsContent = {
mcpServers: {
myServer: {
headers: {
Authorization: 'Bearer ${HOME_ENV_TOKEN}',
},
},
},
};

(mockFsExistsSync as Mock).mockImplementation(
(p: fs.PathLike) => p === USER_SETTINGS_PATH || p === homeEnvPath,
);
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(userSettingsContent);
if (p === homeEnvPath) return 'HOME_ENV_TOKEN=from_home_env';
return '{}';
},
);

delete process.env['HOME_ENV_TOKEN'];
delete process.env['QWEN_HOME'];

const settings = loadSettings(MOCK_WORKSPACE_DIR);
const mcpServers = settings.merged.mcpServers as Record<
string,
{ headers?: Record<string, string> }
>;
expect(mcpServers?.['myServer']?.headers?.['Authorization']).toBe(
'Bearer from_home_env',
);

delete process.env['HOME_ENV_TOKEN'];
});

it('should prefer ~/.qwen/.env over ~/.env for the same key (first-write-wins) (#4466)', () => {
const qwenEnvPath = path.join(path.dirname(USER_SETTINGS_PATH), '.env');
const homeEnvPath = path.join(
path.dirname(path.dirname(USER_SETTINGS_PATH)),
'.env',
);
const userSettingsContent = {
mcpServers: {
myServer: {
headers: {
Authorization: 'Bearer ${PRECEDENCE_TOKEN}',
},
},
},
};

(mockFsExistsSync as Mock).mockImplementation(
(p: fs.PathLike) =>
p === USER_SETTINGS_PATH || p === qwenEnvPath || p === homeEnvPath,
);
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(userSettingsContent);
if (p === qwenEnvPath) return 'PRECEDENCE_TOKEN=from_qwen_dir';
if (p === homeEnvPath) return 'PRECEDENCE_TOKEN=from_home_dir';
return '{}';
},
);

delete process.env['PRECEDENCE_TOKEN'];
delete process.env['QWEN_HOME'];

const settings = loadSettings(MOCK_WORKSPACE_DIR);
const mcpServers = settings.merged.mcpServers as Record<
string,
{ headers?: Record<string, string> }
>;
expect(mcpServers?.['myServer']?.headers?.['Authorization']).toBe(
'Bearer from_qwen_dir',
);

delete process.env['PRECEDENCE_TOKEN'];
});

it('should succeed with unresolved placeholder when .env read throws (#4466)', () => {
const qwenEnvPath = path.join(path.dirname(USER_SETTINGS_PATH), '.env');
const userSettingsContent = {
mcpServers: {
myServer: {
headers: {
Authorization: 'Bearer ${ERROR_TOKEN}',
},
},
},
};

(mockFsExistsSync as Mock).mockImplementation(
(p: fs.PathLike) => p === USER_SETTINGS_PATH || p === qwenEnvPath,
);
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(userSettingsContent);
if (p === qwenEnvPath) throw new Error('EACCES: permission denied');
return '{}';
},
);

delete process.env['ERROR_TOKEN'];

const settings = loadSettings(MOCK_WORKSPACE_DIR);
const mcpServers = settings.merged.mcpServers as Record<
string,
{ headers?: Record<string, string> }
>;
expect(mcpServers?.['myServer']?.headers?.['Authorization']).toBe(
'Bearer ${ERROR_TOKEN}',
);

delete process.env['ERROR_TOKEN'];
});

it('should correctly merge dnsResolutionOrder with workspace taking precedence', () => {
(mockFsExistsSync as Mock).mockReturnValue(true);
const userSettingsContent = {
Expand Down
68 changes: 63 additions & 5 deletions packages/cli/src/config/settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,50 @@ export function resetHomeEnvBootstrapForTesting(): void {
homeEnvBootstrapped = false;
}

/**
* Collects environment variables from user-level `.env` files and returns
* them as a plain dictionary **without** mutating `process.env`.
*
* Candidates are iterated most-specific-first (`~/.qwen/.env` before
* `~/.env`). `??=` ensures the first file to define a key wins, matching
* dotenv's first-occurrence-wins semantics used elsewhere.
*
* Note: this dict intentionally does NOT filter PROJECT_ENV_HARDCODED_EXCLUSIONS
* or advanced.excludedEnvVars — substitution scope is narrower than process.env
* population handled by preResolveHomeEnvOverrides / readHomeEnvInto.
*/
function getHomeEnvFallbackVars(): Record<string, string> {

@wenshao wenshao May 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Path discrepancy with loadEnvironment(). getUserLevelEnvPaths() (used by loadEnvironment()) always returns 3 paths including ~/.env — even when QWEN_HOME is set. But getHomeEnvFallbackVars() skips ~/.env when QWEN_HOME is set. This creates a scenario where a var from ~/.env is loaded into process.env by loadEnvironment() but is NOT available for ${VAR} substitution in settings (because getHomeEnvFallbackVars() ran earlier and skipped it).

Consider aligning the candidate paths, or documenting this difference in user-facing docs.

— qwen3.7-max via Qwen Code /review

const globalQwenDir = Storage.getGlobalQwenDir();
const candidates = [path.join(globalQwenDir, '.env')];
// When QWEN_HOME is set, skip ~/.env to avoid surprise cross-contamination
// from a shared home .env. getUserLevelEnvPaths() always includes ~/.env
// because loadEnvironment() populates process.env independently — the two
// scopes are intentionally different.
if (!process.env['QWEN_HOME']) {
candidates.push(path.join(path.dirname(globalQwenDir), '.env'));
}

const result: Record<string, string> = {};
for (const candidate of candidates) {
if (!fs.existsSync(candidate)) {
continue;
}
try {
const parsed = dotenv.parse(fs.readFileSync(candidate, 'utf-8'));
for (const key in parsed) {
if (Object.hasOwn(parsed, key) && !Object.hasOwn(process.env, key)) {
result[key] ??= parsed[key]!;
}
}
} catch (e) {
debugLogger.warn(
`Failed to read home .env candidate ${candidate}: ${getErrorMessage(e)}`,
);
}
}

@wenshao wenshao May 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The catch (_e) silently swallows every failure (EACCES on ~/.qwen/.env, EISDIR if .env is accidentally a directory, encoding errors, etc.) with no trace. The rest of this file uses debugLogger extensively (~13 call sites) for warn/debug reporting; this function is the outlier.

When a user reports "my MCP server auth is broken", there is currently no log line that distinguishes "file not found" from "permission denied" from "parse error" — and ${MY_TOKEN} just stays as the literal placeholder string, which looks like a resolver bug rather than an I/O failure.

Suggested change
}
} catch (e) {
debugLogger.warn(
`Failed to read fallback env from ${candidate}: ${e instanceof Error ? e.message : String(e)}`,
);
}

Consider also logging at debug level when vars are successfully loaded (e.g. Loaded ${Object.keys(parsed).length} fallback vars from ${candidate}).

— qwen3.7-max via Qwen Code /review

return result;
}

/**
* Surfaces a one-shot warning when QWEN_HOME has been redirected but the
* user hasn't migrated their existing global state. Auto-copying OAuth
Expand Down Expand Up @@ -1032,11 +1076,25 @@ export function loadSettings(
const userOriginalSettings = structuredClone(userResult.settings);
const workspaceOriginalSettings = structuredClone(workspaceResult.settings);

// Environment variables for runtime use
systemSettings = resolveEnvVarsInObject(systemResult.settings);
systemDefaultSettings = resolveEnvVarsInObject(systemDefaultsResult.settings);
userSettings = resolveEnvVarsInObject(userResult.settings);
workspaceSettings = resolveEnvVarsInObject(workspaceResult.settings);
// Resolve ${VAR} placeholders in settings using home .env as fallback.
// getHomeEnvFallbackVars() excludes keys already in process.env, so
// effective precedence is: process.env > home .env > unresolved placeholder.
// The resolver checks customEnv before process.env, but since customEnv
// never contains a process.env key, process.env always wins.
const homeEnvFallback = getHomeEnvFallbackVars();
systemSettings = resolveEnvVarsInObject(
systemResult.settings,
homeEnvFallback,
);
systemDefaultSettings = resolveEnvVarsInObject(
systemDefaultsResult.settings,
homeEnvFallback,
);
userSettings = resolveEnvVarsInObject(userResult.settings, homeEnvFallback);
workspaceSettings = resolveEnvVarsInObject(
workspaceResult.settings,
homeEnvFallback,
);

// Support legacy theme names
if (userSettings.ui?.theme === 'VS') {
Expand Down
Loading