refactor: eliminate duplicate code patterns (#2681)

* refactor: eliminate duplicate code patterns (#2673, #2674, #2675)

1. Extract shared jest.mock('execa') factory into test-helpers/mock-execa.test-utils.ts
   and update 12 test files to use it (was copy-pasted verbatim in each).

2. Extract parseProviderBaseUrl() helper in copilot-api-resolver.ts to deduplicate
   the 7-line URL normalization prologue shared by two functions.

3. Extract cache_and_unset_token() helper in one-shot-token.c to deduplicate the
   18-line cache-store/unsetenv/log/mark-accessed tail in getenv and secure_getenv.

All tests pass. Build passes.

Closes #2673
Closes #2674
Closes #2675

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: unused mock imports, jest.mock require lint, strdup null check

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>

Author: 3 people

containers/agent/one-shot-token/one-shot-token.c

@@ -327,6 +327,43 @@ static int get_token_index(const char *name) {
  *
  * For all other variables: passes through to real getenv
  */
+
+/**
+ * Shared cache-and-unset logic for sensitive token interception.
+ * Caches the value, removes it from the environment, logs if debug is on,
+ * and marks the token as accessed. Must be called with token_mutex held.
+ */
+static char *cache_and_unset_token(int token_idx, const char *name, char *result, const char *via_suffix) {
+    if (result != NULL) {
+        /* Cache the value so subsequent reads succeed after unsetenv */
+        /* Note: This memory is intentionally never freed - it must persist
+         * for the lifetime of the process */
+        char *cached = strdup(result);
+        if (cached == NULL) {
+            fprintf(stderr, "[one-shot-token] Failed to cache token %s: out of memory\n", name);
+            /* Still unset and mark as accessed, but return NULL to signal failure */
+            unsetenv(name);
+            token_accessed[token_idx] = 1;
+            return NULL;
+        }
+        token_cache[token_idx] = cached;
+
+        /* Unset the variable from the environment so /proc/self/environ is cleared */
+        unsetenv(name);
+
+        if (debug_enabled) {
+            fprintf(stderr, "[one-shot-token] Token %s accessed and cached (length: %zu)%s\n",
+                    name, strlen(token_cache[token_idx]), via_suffix);
+        }
+
+        result = token_cache[token_idx];
+    }
+
+    /* Mark as accessed even if NULL (prevents repeated log messages) */
+    token_accessed[token_idx] = 1;
+    return result;
+}
+
 __attribute__((visibility("default")))
 char *getenv(const char *name) {
     ensure_real_getenv();
@@ -354,26 +391,7 @@ char *getenv(const char *name) {
     if (!token_accessed[token_idx]) {
         /* First access - get the real value and cache it */
         result = real_getenv(name);
-
-        if (result != NULL) {
-            /* Cache the value so subsequent reads succeed after unsetenv */
-            /* Note: This memory is intentionally never freed - it must persist
-             * for the lifetime of the process */
-            token_cache[token_idx] = strdup(result);
-
-            /* Unset the variable from the environment so /proc/self/environ is cleared */
-            unsetenv(name);
-
-            if (debug_enabled) {
-                fprintf(stderr, "[one-shot-token] Token %s accessed and cached (length: %zu)\n",
-                        name, strlen(token_cache[token_idx]));
-            }
-
-            result = token_cache[token_idx];
-        }
-
-        /* Mark as accessed even if NULL (prevents repeated log messages) */
-        token_accessed[token_idx] = 1;
+        result = cache_and_unset_token(token_idx, name, result, "");
     } else {
         /* Already accessed - return cached value */
         result = token_cache[token_idx];
@@ -428,26 +446,7 @@ char *secure_getenv(const char *name) {
     if (!token_accessed[token_idx]) {
         /* First access - get the real value using secure_getenv */
         result = real_secure_getenv(name);
-
-        if (result != NULL) {
-            /* Cache the value so subsequent reads succeed after unsetenv */
-            /* Note: This memory is intentionally never freed - it must persist
-             * for the lifetime of the process */
-            token_cache[token_idx] = strdup(result);
-
-            /* Unset the variable from the environment so /proc/self/environ is cleared */
-            unsetenv(name);
-
-            if (debug_enabled) {
-                fprintf(stderr, "[one-shot-token] Token %s accessed and cached (length: %zu) (via secure_getenv)\n",
-                        name, strlen(token_cache[token_idx]));
-            }
-
-            result = token_cache[token_idx];
-        }
-
-        /* Mark as accessed even if NULL (prevents repeated log messages) */
-        token_accessed[token_idx] = 1;
+        result = cache_and_unset_token(token_idx, name, result, " (via secure_getenv)");
     } else {
         /* Already accessed - return cached value */
         result = token_cache[token_idx];

src/compose-generator.test.ts

@@ -6,15 +6,10 @@ import * as os from 'os';
 import * as path from 'path';
 
 // Create mock functions (must remain per-file — jest.mock() is hoisted before imports)
-const mockExecaFn = jest.fn();
-const mockExecaSync = jest.fn();
 
 // Mock execa module
-jest.mock('execa', () => {
-  const fn = (...args: any[]) => mockExecaFn(...args);
-  fn.sync = (...args: any[]) => mockExecaSync(...args);
-  return fn;
-});
+// eslint-disable-next-line @typescript-eslint/no-require-imports
+jest.mock('execa', () => require('./test-helpers/mock-execa.test-utils').execaMockFactory());
 
 let mockConfig: WrapperConfig;
 

src/copilot-api-resolver.ts

@@ -9,12 +9,10 @@ export function resolveCopilotApiKey(
 }
 
 /**
- * Derive a Copilot API target hostname from COPILOT_PROVIDER_BASE_URL.
- * Returns undefined when the value is empty or not a valid URL/host.
+ * Parse a provider base URL into a URL object, handling missing schemes.
+ * Returns undefined if the input is empty or unparseable.
  */
-export function deriveCopilotApiTargetFromProviderBaseUrl(
-  providerBaseUrl: string | undefined
-): string | undefined {
+function parseProviderBaseUrl(providerBaseUrl: string | undefined): URL | undefined {
   const trimmed = providerBaseUrl?.trim();
   if (!trimmed) return undefined;
 
@@ -23,33 +21,35 @@ export function deriveCopilotApiTargetFromProviderBaseUrl(
     : `https://${trimmed}`;
 
   try {
-    return new URL(candidate).hostname || undefined;
+    return new URL(candidate);
   } catch {
     return undefined;
   }
 }
 
+/**
+ * Derive a Copilot API target hostname from COPILOT_PROVIDER_BASE_URL.
+ * Returns undefined when the value is empty or not a valid URL/host.
+ */
+export function deriveCopilotApiTargetFromProviderBaseUrl(
+  providerBaseUrl: string | undefined
+): string | undefined {
+  return parseProviderBaseUrl(providerBaseUrl)?.hostname || undefined;
+}
+
 /**
  * Derive a Copilot API base-path prefix from COPILOT_PROVIDER_BASE_URL.
  * Returns undefined when the value is empty, invalid, or has no path.
  */
 export function deriveCopilotApiBasePathFromProviderBaseUrl(
   providerBaseUrl: string | undefined
 ): string | undefined {
-  const trimmed = providerBaseUrl?.trim();
-  if (!trimmed) return undefined;
-
-  const candidate = trimmed.includes('://')
-    ? trimmed
-    : `https://${trimmed}`;
+  const url = parseProviderBaseUrl(providerBaseUrl);
+  if (!url) return undefined;
 
-  try {
-    const pathname = new URL(candidate).pathname.replace(/\/+$/, '');
-    if (!pathname || pathname === '/') return undefined;
-    return pathname.startsWith('/') ? pathname : `/${pathname}`;
-  } catch {
-    return undefined;
-  }
+  const pathname = url.pathname.replace(/\/+$/, '');
+  if (!pathname || pathname === '/') return undefined;
+  return pathname.startsWith('/') ? pathname : `/${pathname}`;
 }
 
 /**

src/docker-manager-cleanup.test.ts

@@ -5,15 +5,11 @@ import * as path from 'path';
 import * as os from 'os';
 
 // Create mock functions
-const mockExecaFn = jest.fn();
-const mockExecaSync = jest.fn();
 
 // Mock execa module
-jest.mock('execa', () => {
-  const fn = (...args: any[]) => mockExecaFn(...args);
-  fn.sync = (...args: any[]) => mockExecaSync(...args);
-  return fn;
-});
+import { mockExecaFn, mockExecaSync } from './test-helpers/mock-execa.test-utils';
+// eslint-disable-next-line @typescript-eslint/no-require-imports
+jest.mock('execa', () => require('./test-helpers/mock-execa.test-utils').execaMockFactory());
 
 describe('docker-manager writeConfigs and cleanup', () => {
   describe('writeConfigs', () => {

src/docker-manager-lifecycle.test.ts

@@ -5,15 +5,11 @@ import * as path from 'path';
 import * as os from 'os';
 
 // Create mock functions
-const mockExecaFn = jest.fn();
-const mockExecaSync = jest.fn();
 
 // Mock execa module
-jest.mock('execa', () => {
-  const fn = (...args: any[]) => mockExecaFn(...args);
-  fn.sync = (...args: any[]) => mockExecaSync(...args);
-  return fn;
-});
+import { mockExecaFn } from './test-helpers/mock-execa.test-utils';
+// eslint-disable-next-line @typescript-eslint/no-require-imports
+jest.mock('execa', () => require('./test-helpers/mock-execa.test-utils').execaMockFactory());
 
 describe('docker-manager lifecycle', () => {
   describe('startContainers', () => {

src/docker-manager-utils.test.ts

@@ -4,15 +4,10 @@ import * as path from 'path';
 import * as os from 'os';
 
 // Create mock functions
-const mockExecaFn = jest.fn();
-const mockExecaSync = jest.fn();
 
 // Mock execa module
-jest.mock('execa', () => {
-  const fn = (...args: any[]) => mockExecaFn(...args);
-  fn.sync = (...args: any[]) => mockExecaSync(...args);
-  return fn;
-});
+// eslint-disable-next-line @typescript-eslint/no-require-imports
+jest.mock('execa', () => require('./test-helpers/mock-execa.test-utils').execaMockFactory());
 
 describe('docker-manager utilities', () => {
   describe('subnetsOverlap', () => {

src/services/agent-environment.test.ts

@@ -6,15 +6,10 @@ import * as path from 'path';
 import * as os from 'os';
 
 // Create mock functions (must remain per-file — jest.mock() is hoisted before imports)
-const mockExecaFn = jest.fn();
-const mockExecaSync = jest.fn();
 
 // Mock execa module
-jest.mock('execa', () => {
-  const fn = (...args: any[]) => mockExecaFn(...args);
-  fn.sync = (...args: any[]) => mockExecaSync(...args);
-  return fn;
-});
+// eslint-disable-next-line @typescript-eslint/no-require-imports
+jest.mock('execa', () => require('../test-helpers/mock-execa.test-utils').execaMockFactory());
 
 let mockConfig: WrapperConfig;
 

src/services/agent-service.test.ts

@@ -6,15 +6,10 @@ import * as path from 'path';
 import * as os from 'os';
 
 // Create mock functions (must remain per-file — jest.mock() is hoisted before imports)
-const mockExecaFn = jest.fn();
-const mockExecaSync = jest.fn();
 
 // Mock execa module
-jest.mock('execa', () => {
-  const fn = (...args: any[]) => mockExecaFn(...args);
-  fn.sync = (...args: any[]) => mockExecaSync(...args);
-  return fn;
-});
+// eslint-disable-next-line @typescript-eslint/no-require-imports
+jest.mock('execa', () => require('../test-helpers/mock-execa.test-utils').execaMockFactory());
 
 let mockConfig: WrapperConfig;
 

src/services/agent-volumes.test.ts

@@ -6,15 +6,11 @@ import * as path from 'path';
 import * as os from 'os';
 
 // Create mock functions (must remain per-file — jest.mock() is hoisted before imports)
-const mockExecaFn = jest.fn();
-const mockExecaSync = jest.fn();
 
 // Mock execa module
-jest.mock('execa', () => {
-  const fn = (...args: any[]) => mockExecaFn(...args);
-  fn.sync = (...args: any[]) => mockExecaSync(...args);
-  return fn;
-});
+import { mockExecaSync } from '../test-helpers/mock-execa.test-utils';
+// eslint-disable-next-line @typescript-eslint/no-require-imports
+jest.mock('execa', () => require('../test-helpers/mock-execa.test-utils').execaMockFactory());
 
 let mockConfig: WrapperConfig;
 

src/services/api-proxy-service.test.ts

@@ -6,15 +6,10 @@ import * as os from 'os';
 import * as path from 'path';
 
 // Create mock functions (must remain per-file — jest.mock() is hoisted before imports)
-const mockExecaFn = jest.fn();
-const mockExecaSync = jest.fn();
 
 // Mock execa module
-jest.mock('execa', () => {
-  const fn = (...args: any[]) => mockExecaFn(...args);
-  fn.sync = (...args: any[]) => mockExecaSync(...args);
-  return fn;
-});
+// eslint-disable-next-line @typescript-eslint/no-require-imports
+jest.mock('execa', () => require('../test-helpers/mock-execa.test-utils').execaMockFactory());
 
 let mockConfig: WrapperConfig;