feat: add --env-file support for injecting env vars from a file#1457
feat: add --env-file support for injecting env vars from a file#1457
Conversation
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 82.73% | 82.75% | 📈 +0.02% |
| Statements | 82.39% | 82.42% | 📈 +0.03% |
| Functions | 81.44% | 81.50% | 📈 +0.06% |
| Branches | 76.06% | 76.02% | 📉 -0.04% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
61.4% → 60.9% (-0.59%) | 61.9% → 61.3% (-0.57%) |
src/docker-manager.ts |
85.7% → 86.4% (+0.73%) | 85.2% → 85.9% (+0.73%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
* Initial plan * fix: add tests to resolve branch coverage regression in cli.ts Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/41835a48-8b69-46e0-9485-8fb41265babf --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
Coverage comparison generated by |
|
Smoke Test Results — PASS
|
Smoke Test Results✅ GitHub MCP — Last 2 merged PRs:
✅ Playwright — github.com title contains "GitHub" Overall: PASS | PR author:
|
Chroot Version Comparison Results
Result: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
|
Smoke Test Report (Codex)
Warning
|
There was a problem hiding this comment.
Pull request overview
Adds --env-file support so AWF can inject environment variables into the agent container from a file (useful for values written to $GITHUB_OUTPUT and other file-based sources), extending the existing --env-all / --env mechanisms.
Changes:
- Extend
WrapperConfigwithenvFile?: string. - Add
--env-file <path>CLI option with startup existence validation. - Implement
readEnvFile()+ integrate env-file injection intogenerateDockerCompose, plus unit/integration tests and documentation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Adds envFile?: string to wrapper config and describes intended behavior/precedence. |
| src/cli.ts | Adds --env-file flag, validates path existence, and threads option into WrapperConfig. |
| src/docker-manager.ts | Implements env-file parser and injects parsed vars into container environment. |
| src/docker-manager.test.ts | Adds tests for readEnvFile() and compose env injection behavior. |
| docs/environment.md | Documents env-file format and precedence expectations. |
Comments suppressed due to low confidence (2)
src/docker-manager.test.ts:3513
- These
readEnvFiletest names say “should reject …”, but the function currently ignores invalid keys/lines (it doesn’t throw or report). Renaming these tests (or changing the function to actually reject with an error) would make the behavior clearer and keep wording consistent across code/docs.
it('should reject keys starting with a digit', () => {
const envFile = path.join(tmpDir, '.env');
fs.writeFileSync(envFile, '123KEY=value\nFOO=bar\n');
expect(readEnvFile(envFile)).toEqual({ FOO: 'bar' });
});
it('should reject keys containing hyphens', () => {
const envFile = path.join(tmpDir, '.env');
fs.writeFileSync(envFile, 'KEY-NAME=value\nFOO=bar\n');
expect(readEnvFile(envFile)).toEqual({ FOO: 'bar' });
});
src/docker-manager.ts:235
readEnvFile()trims each whole line (raw.trim()), but the docstring says “Values are taken literally”. With the current implementation, trailing spaces in values (and leading spaces before the key) are stripped, so the parsed VALUE isn’t actually literal. Consider only trimming for comment/blank detection (e.g., trimStart for#) and splitting on the first=while preserving the value bytes (also handle\r\nwithout relying on trim).
for (const raw of content.split('\n')) {
const line = raw.trim();
// Skip comments and blank lines
if (line === '' || line.startsWith('#')) continue;
const match = line.match(/^([A-Za-z_][A-Za-z0-9_]*)=(.*)$/);
if (match) {
result[match[1]] = match[2];
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @throws {Error} If the file cannot be read | ||
| */ | ||
| export function readEnvFile(filePath: string): Record<string, string> { | ||
| const content = fs.readFileSync(filePath, 'utf-8'); | ||
| const result: Record<string, string> = {}; | ||
| for (const raw of content.split('\n')) { | ||
| const line = raw.trim(); | ||
| // Skip comments and blank lines | ||
| if (line === '' || line.startsWith('#')) continue; | ||
| const match = line.match(/^([A-Za-z_][A-Za-z0-9_]*)=(.*)$/); | ||
| if (match) { | ||
| result[match[1]] = match[2]; | ||
| } |
There was a problem hiding this comment.
The readEnvFile() JSDoc says each non-comment line “must match” KEY=VALUE, but the implementation silently ignores non-matching lines (including invalid keys) instead of throwing or surfacing an error. Either update the docs to say invalid lines are ignored, or change the function to fail fast with a helpful error including the line number/content.
This issue also appears on line 228 of the same file.
| * @throws {Error} If the file cannot be read | |
| */ | |
| export function readEnvFile(filePath: string): Record<string, string> { | |
| const content = fs.readFileSync(filePath, 'utf-8'); | |
| const result: Record<string, string> = {}; | |
| for (const raw of content.split('\n')) { | |
| const line = raw.trim(); | |
| // Skip comments and blank lines | |
| if (line === '' || line.startsWith('#')) continue; | |
| const match = line.match(/^([A-Za-z_][A-Za-z0-9_]*)=(.*)$/); | |
| if (match) { | |
| result[match[1]] = match[2]; | |
| } | |
| * @throws {Error} If the file cannot be read or contains an invalid line | |
| */ | |
| export function readEnvFile(filePath: string): Record<string, string> { | |
| const content = fs.readFileSync(filePath, 'utf-8'); | |
| const result: Record<string, string> = {}; | |
| const lines = content.split('\n'); | |
| for (let i = 0; i < lines.length; i++) { | |
| const raw = lines[i]; | |
| const line = raw.trim(); | |
| // Skip comments and blank lines | |
| if (line === '' || line.startsWith('#')) continue; | |
| const match = line.match(/^([A-Za-z_][A-Za-z0-9_]*)=(.*)$/); | |
| if (!match) { | |
| throw new Error( | |
| `Invalid env file line in ${filePath} at line ${i + 1}: "${raw}" ` + | |
| '(expected KEY=VALUE with KEY matching /^[A-Za-z_][A-Za-z0-9_]*$/)' | |
| ); | |
| } | |
| result[match[1]] = match[2]; |
| // Environment variables from --env-file (injected before --env flags so explicit flags win) | ||
| if (config.envFile) { | ||
| const fileEnv = readEnvFile(config.envFile); | ||
| for (const [key, value] of Object.entries(fileEnv)) { | ||
| if (!EXCLUDED_ENV_VARS.has(key) && !Object.prototype.hasOwnProperty.call(environment, key)) { |
There was a problem hiding this comment.
--env-file is documented as having higher precedence than --env-all, but this block only sets values when environment does not already have the key. Since --env-all runs earlier and also populates environment, env-file entries won’t override env-all entries. Either adjust the precedence (allow env-file to override env-all while still protecting framework/excluded vars) or update the docs/comments/tests to reflect the actual “first writer wins” behavior.
| // Environment variables from --env-file (injected before --env flags so explicit flags win) | |
| if (config.envFile) { | |
| const fileEnv = readEnvFile(config.envFile); | |
| for (const [key, value] of Object.entries(fileEnv)) { | |
| if (!EXCLUDED_ENV_VARS.has(key) && !Object.prototype.hasOwnProperty.call(environment, key)) { | |
| // Environment variables from --env-file (applied after --env-all but before --env flags; | |
| // env-file entries override env-all for non-excluded vars, while explicit --env flags still win) | |
| if (config.envFile) { | |
| const fileEnv = readEnvFile(config.envFile); | |
| for (const [key, value] of Object.entries(fileEnv)) { | |
| if (!EXCLUDED_ENV_VARS.has(key)) { |
| if (!fs.existsSync(options.envFile)) { | ||
| logger.error(`--env-file: file not found: ${options.envFile}`); |
There was a problem hiding this comment.
The --env-file validation only checks existsSync. If the path exists but is a directory or is unreadable, the error will surface later (likely as an uncaught read error) and the CLI message won’t be as clear. Consider validating that the path is a readable regular file (e.g., statSync().isFile() + accessSync(R_OK)) and exiting with a consistent error message.
| if (!fs.existsSync(options.envFile)) { | |
| logger.error(`--env-file: file not found: ${options.envFile}`); | |
| try { | |
| if (!fs.existsSync(options.envFile)) { | |
| logger.error(`--env-file: file not found: ${options.envFile}`); | |
| process.exit(1); | |
| } | |
| const envFileStat = fs.statSync(options.envFile); | |
| if (!envFileStat.isFile()) { | |
| logger.error(`--env-file: not a regular file: ${options.envFile}`); | |
| process.exit(1); | |
| } | |
| fs.accessSync(options.envFile, fs.constants.R_OK); | |
| } catch (error) { | |
| logger.error(`--env-file: cannot read file: ${options.envFile}`); |
| - Values are taken literally (no quote stripping, no variable expansion) | ||
|
|
||
| **Precedence (lowest → highest):** | ||
| 1. Built-in framework variables (proxy, DNS, etc.) | ||
| 2. `--env-all` host variables | ||
| 3. `--env-file` variables | ||
| 4. `--env` / `-e` explicit variables (highest priority) |
There was a problem hiding this comment.
The --env-file docs claim (1) env-file values are “taken literally” and (2) precedence is built-in → env-all → env-file → env. In the current implementation, lines are trimmed during parsing, framework vars are protected from being overwritten by env-all/env-file, and env-file does not override env-all. Please adjust this section to match actual behavior (or update the implementation to match the documented precedence).
| - Values are taken literally (no quote stripping, no variable expansion) | |
| **Precedence (lowest → highest):** | |
| 1. Built-in framework variables (proxy, DNS, etc.) | |
| 2. `--env-all` host variables | |
| 3. `--env-file` variables | |
| 4. `--env` / `-e` explicit variables (highest priority) | |
| - Values are read as raw text (no variable expansion). Leading and trailing whitespace on each line is trimmed by the parser, and quotes inside the value are preserved (no quote stripping). | |
| **Precedence (lowest → highest):** | |
| 1. Built-in framework variables (proxy, DNS, etc.). For these keys, framework-provided values are always used and are **not** overridden by `--env-all` or `--env-file`. | |
| 2. `--env-file` variables | |
| 3. `--env-all` host variables | |
| 4. `--env` / `-e` explicit variables (highest priority for non-framework variables) |
| // AWF_DNS_SERVERS is set before envFile processing; file should not clobber it | ||
| fs.writeFileSync(envFile, 'AWF_DNS_SERVERS=1.1.1.1\n'); | ||
|
|
||
| const config = { ...mockConfig, envFile }; | ||
| const result = generateDockerCompose(config, mockNetworkConfig); | ||
| const env = result.services.agent.environment as Record<string, string>; | ||
|
|
||
| // AWF_DNS_SERVERS is set by the framework; file should NOT override it | ||
| expect(env.AWF_DNS_SERVERS).not.toBe('1.1.1.1'); |
There was a problem hiding this comment.
This test’s comment/intent doesn’t match the actual ordering in generateDockerCompose: AWF_DNS_SERVERS is assigned after the env-file injection block, so the env-file value would be overwritten later regardless of the “don’t clobber existing vars” guard. If the goal is to verify env-file doesn’t override already-set framework vars, use a variable that is definitely set before env-file processing (e.g., one of the proxy vars or AWF_HOST_PATH) or assert the exact precedence rules you want.
This issue also appears on line 3503 of the same file.
| // AWF_DNS_SERVERS is set before envFile processing; file should not clobber it | |
| fs.writeFileSync(envFile, 'AWF_DNS_SERVERS=1.1.1.1\n'); | |
| const config = { ...mockConfig, envFile }; | |
| const result = generateDockerCompose(config, mockNetworkConfig); | |
| const env = result.services.agent.environment as Record<string, string>; | |
| // AWF_DNS_SERVERS is set by the framework; file should NOT override it | |
| expect(env.AWF_DNS_SERVERS).not.toBe('1.1.1.1'); | |
| // AWF_HOST_PATH is set before envFile processing; file should not clobber it | |
| fs.writeFileSync(envFile, 'AWF_HOST_PATH=/malicious/path\n'); | |
| const config = { ...mockConfig, envFile }; | |
| const result = generateDockerCompose(config, mockNetworkConfig); | |
| const env = result.services.agent.environment as Record<string, string>; | |
| // AWF_HOST_PATH is set by the framework; file should NOT override it | |
| expect(env.AWF_HOST_PATH).not.toBe('/malicious/path'); |
Variables written only to
$GITHUB_OUTPUT(e.g.GH_AW_SAFE_OUTPUTS_CONFIG_PATH) are never inprocess.env, so--env-allcannot forward them to the agent container. Adding--env-fileprovides a file-based injection path that works regardless of how variables were set.Changes
src/types.ts—envFile?: stringadded toWrapperConfigsrc/cli.ts—--env-file <path>option; validates file existence at startupsrc/docker-manager.ts—readEnvFile()parsesKEY=VALUEfiles (skips comments/blank lines, trims whitespace, rejects invalid keys); injected after--env-allbut before--envso explicit flags always win; excluded system vars (PATH,HOME, etc.) are never injectedsrc/docker-manager.test.ts— 10 unit tests forreadEnvFile()+ 5 integration tests forgenerateDockerComposewithenvFiledocs/environment.md— documents format, precedence, and a Safe Outputs MCP exampleUsage
Precedence (low → high): built-in framework vars →
--env-all→--env-file→--env💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.