refactor: extract shared copyEnvEntries helper to eliminate duplicated env-filtering loops

src/env-utils.test.ts

@@ -2,7 +2,7 @@ import * as fs from 'fs';
 import * as os from 'os';
 import * as path from 'path';
 import { WrapperConfig } from './types';
-import { getConfigEnvValue, getLowerCaseProcessEnvValue, pickEnvVars } from './env-utils';
+import { copyEnvEntries, getConfigEnvValue, getLowerCaseProcessEnvValue, pickEnvVars } from './env-utils';
 
 function makeWrapperConfig(overrides: Partial<WrapperConfig> = {}): WrapperConfig {
   return {
@@ -149,3 +149,109 @@ describe('getLowerCaseProcessEnvValue', () => {
     expect(getLowerCaseProcessEnvValue('TEST_LOWERCASE_ENV_VALUE')).toBeUndefined();
   });
 });
+
+describe('copyEnvEntries', () => {
+  it('copies all defined entries from source to target', () => {
+    const source: Record<string, string | undefined> = { A: 'a', B: 'b' };
+    const target: Record<string, string> = {};
+    copyEnvEntries(source, target);
+    expect(target).toEqual({ A: 'a', B: 'b' });
+  });
+
+  it('skips entries with undefined values', () => {
+    const source: Record<string, string | undefined> = { A: 'a', B: undefined };
+    const target: Record<string, string> = {};
+    copyEnvEntries(source, target);
+    expect(target).toEqual({ A: 'a' });
+  });
+
+  it('skips keys in excludedKeys', () => {
+    const source: Record<string, string | undefined> = { A: 'a', B: 'b', C: 'c' };
+    const target: Record<string, string> = {};
+    copyEnvEntries(source, target, { excludedKeys: new Set(['B']) });
+    expect(target).toEqual({ A: 'a', C: 'c' });
+  });
+
+  it('allows keys in allowKeys even when they are also in excludedKeys', () => {
+    const source: Record<string, string | undefined> = { A: 'a', B: 'b' };
+    const target: Record<string, string> = {};
+    copyEnvEntries(source, target, {
+      excludedKeys: new Set(['A', 'B']),
+      allowKeys: new Set(['B']),
+    });
+    expect(target).toEqual({ B: 'b' });
+  });
+
+  it('does not overwrite existing keys when noOverwrite is true', () => {
+    const source: Record<string, string | undefined> = { A: 'new', B: 'new' };
+    const target: Record<string, string> = { A: 'original' };
+    copyEnvEntries(source, target, { noOverwrite: true });
+    expect(target).toEqual({ A: 'original', B: 'new' });
+  });
+
+  it('overwrites existing keys when noOverwrite is false (default)', () => {
+    const source: Record<string, string | undefined> = { A: 'new' };
+    const target: Record<string, string> = { A: 'original' };
+    copyEnvEntries(source, target);
+    expect(target).toEqual({ A: 'new' });
+  });
+
+  it('only copies keys matching keyPredicate', () => {
+    const source: Record<string, string | undefined> = { OTEL_FOO: 'x', OTHER: 'y' };
+    const target: Record<string, string> = {};
+    copyEnvEntries(source, target, { keyPredicate: (k) => k.startsWith('OTEL_') });
+    expect(target).toEqual({ OTEL_FOO: 'x' });
+  });
+
+  it('skips entries exceeding maxValueSizeBytes and calls onSkippedOversized', () => {
+    const bigValue = 'x'.repeat(200);
+    const source: Record<string, string | undefined> = { SMALL: 'hi', BIG: bigValue };
+    const target: Record<string, string> = {};
+    const skipped: Array<{ key: string; sizeBytes: number }> = [];
+    copyEnvEntries(source, target, {
+      maxValueSizeBytes: 10,
+      onSkippedOversized: (key, sizeBytes) => skipped.push({ key, sizeBytes }),
+    });
+    expect(target).toEqual({ SMALL: 'hi' });
+    expect(skipped).toHaveLength(1);
+    expect(skipped[0].key).toBe('BIG');
+    expect(skipped[0].sizeBytes).toBe(200);
+  });
+
+  it('copies entries at exactly maxValueSizeBytes (boundary is exclusive)', () => {
+    const value = 'x'.repeat(10);
+    const source: Record<string, string | undefined> = { V: value };
+    const target: Record<string, string> = {};
+    copyEnvEntries(source, target, { maxValueSizeBytes: 10 });
+    expect(target).toEqual({ V: value });
+  });
+
+  it('skips entries one byte over maxValueSizeBytes', () => {
+    const value = 'x'.repeat(11);
+    const source: Record<string, string | undefined> = { V: value };
+    const target: Record<string, string> = {};
+    copyEnvEntries(source, target, { maxValueSizeBytes: 10 });
+    expect(target).toEqual({});
+  });
+
+  it('applies all filters together', () => {
+    const source: Record<string, string | undefined> = {
+      OTEL_KEEP: 'ok',
+      OTEL_EXCLUDED: 'no',
+      OTEL_BIG: 'x'.repeat(200),
+      OTEL_EXISTING: 'old',
+      OTHER: 'ignored',
+    };
+    const target: Record<string, string> = { OTEL_EXISTING: 'original' };
+    const skipped: string[] = [];
+    copyEnvEntries(source, target, {
+      excludedKeys: new Set(['OTEL_EXCLUDED']),
+      noOverwrite: true,
+      keyPredicate: (k) => k.startsWith('OTEL_'),
+      maxValueSizeBytes: 10,
+      onSkippedOversized: (key) => skipped.push(key),
+    });
+    expect(target).toEqual({ OTEL_EXISTING: 'original', OTEL_KEEP: 'ok' });
+    expect(skipped).toEqual(['OTEL_BIG']);
+  });
+});

src/env-utils.ts

@@ -1,6 +1,76 @@
 import { readEnvFile } from './github-env';
 import { WrapperConfig } from './types';
 
+/**
+ * Options for {@link copyEnvEntries}.
+ */
+export interface CopyEnvEntriesOptions {
+  /**
+   * Keys to skip.  An entry whose key is in this set is omitted unless it
+   * also appears in `allowKeys`.
+   */
+  excludedKeys?: Set<string>;
+  /**
+   * Keys that bypass `excludedKeys`.  An entry whose key is in both
+   * `excludedKeys` and `allowKeys` is still copied.
+   */
+  allowKeys?: Set<string>;
+  /**
+   * When `true`, entries whose key is already present in `target` are not
+   * overwritten.  Default: `false`.
+   */
+  noOverwrite?: boolean;
+  /**
+   * Additional predicate applied to each key before copying.  Only entries
+   * for which the predicate returns `true` are copied.
+   */
+  keyPredicate?: (key: string) => boolean;
+  /**
+   * Maximum allowed value size in bytes (UTF-8).  Entries whose value
+   * exceeds this limit are skipped; `onSkippedOversized` is called for each
+   * skipped entry when provided.
+   */
+  maxValueSizeBytes?: number;
+  /**
+   * Called for each entry skipped because it exceeded `maxValueSizeBytes`.
+   * @param key       - The environment variable name.
+   * @param sizeBytes - The actual UTF-8 byte length of the value.
+   */
+  onSkippedOversized?: (key: string, sizeBytes: number) => void;
+}
+
+/**
+ * Copies entries from `source` into `target` according to the given options.
+ *
+ * Entries with `undefined` values are always skipped, since they cannot be
+ * represented in a `Record<string, string>`.
+ *
+ * This helper centralises the repeated env-filtering loop that would otherwise
+ * be duplicated across sbx sanitization, host passthrough, OTEL forwarding,
+ * and GitHub Actions env-file / additionalEnv merging.
+ */
+export function copyEnvEntries(
+  source: Record<string, string | undefined>,
+  target: Record<string, string | undefined>,
+  options: CopyEnvEntriesOptions = {},
+): void {
+  const { excludedKeys, allowKeys, noOverwrite = false, keyPredicate, maxValueSizeBytes, onSkippedOversized } = options;
+  for (const [key, value] of Object.entries(source)) {
+    if (value === undefined) continue;
+    if (excludedKeys?.has(key) && !(allowKeys?.has(key))) continue;
+    if (keyPredicate !== undefined && !keyPredicate(key)) continue;
+    if (noOverwrite && Object.prototype.hasOwnProperty.call(target, key)) continue;
+    if (maxValueSizeBytes !== undefined) {
+      const sizeBytes = Buffer.byteLength(value, 'utf8');
+      if (sizeBytes > maxValueSizeBytes) {
+        onSkippedOversized?.(key, sizeBytes);
+        continue;
+      }
+    }
+    target[key] = value;
+  }
+}
+
 function normalizeEnvValue(value: string | undefined): string | undefined {
   const normalizedValue = value?.trim();
   return normalizedValue || undefined;

src/sbx-manager.ts

@@ -25,6 +25,7 @@
 import execa from 'execa';
 import * as fs from 'fs';
 import * as path from 'path';
+import { copyEnvEntries } from './env-utils';
 import { logger } from './logger';
 import { HOME_TOOL_SUBDIRS } from './services/agent-volumes/home-whitelist';
 import { credentialEntriesUnderMountedParents } from './config/mount-policy';
@@ -79,11 +80,9 @@ export function sanitizeEnvForSbx(
   overrides: Record<string, string> = {},
 ): Record<string, string | undefined> {
   const clean: Record<string, string | undefined> = {};
-  for (const [key, value] of Object.entries(process.env)) {
-    if (!SECRET_ENV_PATTERNS.some((p) => p.test(key))) {
-      clean[key] = value;
-    }
-  }
+  copyEnvEntries(process.env, clean, {
+    keyPredicate: (key) => !SECRET_ENV_PATTERNS.some((p) => p.test(key)),
+  });
   return { ...clean, ...overrides };
 }
 

src/services/agent-environment/env-passthrough.ts

@@ -1,4 +1,5 @@
 import { MAX_ENV_VALUE_SIZE } from '../../constants';
+import { copyEnvEntries } from '../../env-utils';
 import { logger } from '../../logger';
 import { WrapperConfig } from '../../types';
 
@@ -13,16 +14,14 @@ export function passthroughHostEnvironment(params: EnvPassthroughParams): void {
 
   if (config.envAll) {
     const skippedLargeVars: string[] = [];
-    for (const [key, value] of Object.entries(process.env)) {
-      if (value !== undefined && !excludedEnvVars.has(key) && !Object.prototype.hasOwnProperty.call(environment, key)) {
-        const valueSizeBytes = Buffer.byteLength(value, 'utf8');
-        if (valueSizeBytes > MAX_ENV_VALUE_SIZE) {
-          skippedLargeVars.push(`${key} (${(valueSizeBytes / 1024).toFixed(0)} KB)`);
-          continue;
-        }
-        environment[key] = value;
-      }
-    }
+    copyEnvEntries(process.env, environment, {
+      excludedKeys: excludedEnvVars,
+      noOverwrite: true,
+      maxValueSizeBytes: MAX_ENV_VALUE_SIZE,
+      onSkippedOversized: (key, sizeBytes) => {
+        skippedLargeVars.push(`${key} (${(sizeBytes / 1024).toFixed(0)} KB)`);
+      },
+    });
 
     if (skippedLargeVars.length > 0) {
       logger.warn(`Skipped ${skippedLargeVars.length} oversized env var(s) from --env-all passthrough (>${(MAX_ENV_VALUE_SIZE / 1024).toFixed(0)} KB each):`);

src/services/agent-environment/github-actions-environment.ts

@@ -5,6 +5,7 @@ import {
 import { logger } from '../../logger';
 import { WrapperConfig } from '../../types';
 import { PROXY_ENV_VARS } from '../../upstream-proxy';
+import { copyEnvEntries } from '../../env-utils';
 
 interface GitHubActionsEnvironmentParams {
   config: WrapperConfig;
@@ -30,23 +31,21 @@ export function buildGitHubActionsEnvironment(params: GitHubActionsEnvironmentPa
 
   if (config.envFile) {
     const fileEnv = readEnvFile(config.envFile);
-    for (const [key, value] of Object.entries(fileEnv)) {
-      if (!excludedEnvVars.has(key) && !Object.prototype.hasOwnProperty.call(environment, key)) {
-        environment[key] = value;
-      }
-    }
+    copyEnvEntries(fileEnv, environment, {
+      excludedKeys: excludedEnvVars,
+      noOverwrite: true,
+    });
   }
 
   if (config.additionalEnv) {
     // Proxy vars are in the exclusion set to prevent host proxy leakage via
     // envAll, but explicit --env overrides (additionalEnv) should still be
     // able to set them (e.g. NO_PROXY customization).
     const proxyVarSet = new Set<string>(PROXY_ENV_VARS);
-    for (const [key, value] of Object.entries(config.additionalEnv)) {
-      if (!excludedEnvVars.has(key) || proxyVarSet.has(key)) {
-        environment[key] = value;
-      }
-    }
+    copyEnvEntries(config.additionalEnv, environment, {
+      excludedKeys: excludedEnvVars,
+      allowKeys: proxyVarSet,
+    });
   }
 
   if (environment.NO_PROXY !== environment.no_proxy) {

src/services/agent-environment/observability-environment.ts

@@ -1,3 +1,4 @@
+import { copyEnvEntries } from '../../env-utils';
 import { SslConfig } from '../../host-env';
 import { WrapperConfig } from '../../types';
 
@@ -13,13 +14,11 @@ export function buildOtelEnvironment(params: OtelEnvironmentParams): void {
     return;
   }
 
-  for (const [key, value] of Object.entries(process.env)) {
-    if (key.startsWith('OTEL_') && value !== undefined
-        && !excludedEnvVars.has(key)
-        && !Object.prototype.hasOwnProperty.call(environment, key)) {
-      environment[key] = value;
-    }
-  }
+  copyEnvEntries(process.env, environment, {
+    excludedKeys: excludedEnvVars,
+    noOverwrite: true,
+    keyPredicate: (key) => key.startsWith('OTEL_'),
+  });
 }
 
 export function buildSslEnvironment(environment: Record<string, string>, sslConfig?: SslConfig): void {