Skip to content

refactor: consolidate env-var forwarding in docker-manager.ts into typed arrays + loops#2434

Merged
lpcox merged 2 commits into
mainfrom
copilot/refactor-env-var-forwarding
May 3, 2026
Merged

refactor: consolidate env-var forwarding in docker-manager.ts into typed arrays + loops#2434
lpcox merged 2 commits into
mainfrom
copilot/refactor-env-var-forwarding

Conversation

Copilot AI commented May 3, 2026

Copy link
Copy Markdown
Contributor

15 individual if-guard lines for env-var passthrough were inconsistent with the existing loop pattern used for Docker client vars, and made the !config.enableApiProxy credential-isolation boundary easy to violate when adding new API-key vars.

Changes

  • alwaysForwardVars array + single loop — merges GitHub auth tokens, user env, enterprise/Actions URLs, and the previously separate dockerClientEnvVars array into one typed as const array iterated by a single loop
  • API key vars under one !config.enableApiProxy guardOPENAI_API_KEY, CODEX_API_KEY, ANTHROPIC_API_KEY, COPILOT_GITHUB_TOKEN, COPILOT_API_KEY grouped explicitly so the credential-isolation boundary is structurally visible
  • TERM kept as a standalone special case — retains its distinct !config.tty guard
// Before: 15 scattered if-statements + a separate loop
if (process.env.GITHUB_TOKEN) environment.GITHUB_TOKEN = process.env.GITHUB_TOKEN;
if (process.env.OPENAI_API_KEY && !config.enableApiProxy) environment.OPENAI_API_KEY = process.env.OPENAI_API_KEY;
// ...12 more lines...
const dockerClientEnvVars = ['DOCKER_HOST', ...] as const;
for (const v of dockerClientEnvVars) { ... }

// After: two typed arrays + loops, one guard
const alwaysForwardVars = ['GITHUB_TOKEN', 'GH_TOKEN', ..., 'DOCKER_HOST', ...] as const;
for (const v of alwaysForwardVars) {
  if (process.env[v]) environment[v] = process.env[v]!;
}
if (!config.enableApiProxy) {
  for (const v of ['OPENAI_API_KEY', 'CODEX_API_KEY', 'ANTHROPIC_API_KEY',
                   'COPILOT_GITHUB_TOKEN', 'COPILOT_API_KEY'] as const) {
    if (process.env[v]) environment[v] = process.env[v]!;
  }
}

Copilot AI changed the title [WIP] Refactor environment variable forwarding in docker-manager.ts refactor: consolidate env-var forwarding in docker-manager.ts into typed arrays + loops May 3, 2026
Copilot AI requested a review from lpcox May 3, 2026 13:58
Copilot finished work on behalf of lpcox May 3, 2026 13:58
@lpcox lpcox marked this pull request as ready for review May 3, 2026 22:58
@lpcox lpcox requested a review from Mossaka as a code owner May 3, 2026 22:58
Copilot AI review requested due to automatic review settings May 3, 2026 22:58
@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 85.76% 85.79% 📈 +0.03%
Statements 85.64% 85.73% 📈 +0.09%
Functions 88.11% 88.11% ➡️ +0.00%
Branches 78.65% 78.77% 📈 +0.12%
📁 Per-file Coverage Changes (1 files)
File Lines (Before → After) Statements (Before → After)
src/docker-manager.ts 87.4% → 87.6% (+0.16%) 87.0% → 87.4% (+0.36%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

Copilot AI 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.

Pull request overview

Refactors generateDockerCompose()’s default environment-variable passthrough logic in docker-manager.ts to reduce scattered conditional forwarding and make the “don’t leak API keys when api-proxy is enabled” boundary more structurally obvious.

Changes:

  • Consolidates non-secret passthrough vars (GitHub auth, user env, enterprise URLs, Actions OIDC, Docker client env) into a single alwaysForwardVars as const list and loop.
  • Groups provider API key passthrough under a single if (!config.enableApiProxy) guard with a dedicated loop.
  • Keeps TERM passthrough as a standalone special case behind !config.tty.
Show a summary per file
File Description
src/docker-manager.ts Consolidates env-var forwarding into typed arrays + loops while preserving api-proxy credential isolation behavior.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor

Smoke Test Results

✅ GitHub MCP: #2417, #2409
✅ Playwright: github.com title verified
✅ File writing: /tmp/gh-aw/agent test created
✅ Bash verification: content correct

Status: PASS

💥 [THE END] — Illustrated by Smoke Claude

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor

🔥 Smoke Test Results

Test Result
GitHub MCP connectivity
GitHub.com HTTP ⚠️ pre-step data unavailable (template vars not substituted)
File write/read ⚠️ pre-step data unavailable (template vars not substituted)

PR: refactor: consolidate env-var forwarding in docker-manager.ts into typed arrays + loops
Author: @Copilot · Assignees: @lpcox, @Copilot

Overall: PARTIAL — MCP ✅, other checks indeterminate due to missing pre-step outputs.

📰 BREAKING: Report filed by Smoke Copilot

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor

Smoke Test: Copilot BYOK (Offline) Mode

Test Result
GitHub MCP connectivity
GitHub.com HTTP connectivity ⚠️ (template vars unexpanded)
File write/read ⚠️ (template vars unexpanded)
BYOK inference (this response)

Running in BYOK offline mode (COPILOT_OFFLINE=true) via api-proxy → api.githubcopilot.com

Author: @Copilot · Assignees: @lpcox, @Copilot

Overall: PARTIAL PASS — BYOK inference and MCP work; pre-step smoke data ($\{\{ steps.smoke-data.outputs.* }}) was not substituted at runtime.

🔑 BYOK report filed by Smoke Copilot BYOK

@github-actions github-actions Bot mentioned this pull request May 3, 2026
@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor

Chroot Smoke Test Results

Runtime Host Version Chroot Version Match?
Python Python 3.12.13 Python 3.12.3 ❌ NO
Node.js v24.14.1 v20.20.2 ❌ NO
Go go1.22.12 go1.22.12 ✅ YES

Result: ❌ Not all runtimes match — Python and Node.js versions differ between host and chroot.

Tested by Smoke Chroot

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor

Merged PR titles: feat: add 4 agentic code quality workflows; refactor(api-proxy): extract ProviderAdapter abstraction, make core provider-agnostic
Queried PR titles: Make DockerService and DockerNetwork interfaces internal (remove export); Remove unused export from SslBumpConfig in security-critical module
GitHub MCP review: ❌ | safeinputs-gh query: ❌
Playwright title check: ✅ | Tavily search: ❌
Temp file write: ✅ | bash cat: ✅
Discussion lookup/comment: ✅ | npm ci && npm run build: ✅
Overall: FAIL

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • registry.npmjs.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "registry.npmjs.org"

See Network Configuration for more information.

🔮 The oracle has spoken through Smoke Codex

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor

🏗️ Build Test Suite Results

Ecosystem Project Build/Install Tests Status
Bun elysia 1/1 passed ✅ PASS
Bun hono 1/1 passed ✅ PASS
C++ fmt N/A ✅ PASS
C++ json N/A ✅ PASS
Deno oak N/A 1/1 passed ✅ PASS
Deno std N/A 1/1 passed ✅ PASS
.NET hello-world N/A ✅ PASS
.NET json-parse N/A ✅ PASS
Go color passed ✅ PASS
Go env passed ✅ PASS
Go uuid passed ✅ PASS
Java gson 1/1 passed ✅ PASS
Java caffeine 1/1 passed ✅ PASS
Node.js clsx passed ✅ PASS
Node.js execa passed ✅ PASS
Node.js p-limit passed ✅ PASS
Rust fd 1/1 passed ✅ PASS
Rust zoxide 1/1 passed ✅ PASS

Overall: 8/8 ecosystems passed — ✅ PASS

Generated by Build Test Suite for issue #2434 · ● 575.8K ·

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor

Smoke Test Results

  • Redis PING: ❌ (timeout — host.docker.internal:6379 unreachable)
  • PostgreSQL pg_isready: ❌ (host.docker.internal:5432 — no response)
  • PostgreSQL SELECT 1: ❌ (not attempted; pg_isready failed)

Overall: FAIL — service containers are not reachable from this environment via host.docker.internal.

🔌 Service connectivity validated by Smoke Services

@lpcox lpcox merged commit 1b3d570 into main May 3, 2026
61 of 71 checks passed
@lpcox lpcox deleted the copilot/refactor-env-var-forwarding branch May 3, 2026 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Duplicate Code] docker-manager.ts: env-var forwarding uses 15 manual if-statements instead of a loop pattern

3 participants