Merge branch 'main' into copilot/fix-code-for-review-comments

Author: lpcox

.github/workflows/firewall-issue-dispatcher.md

@@ -80,11 +80,12 @@ For each **unprocessed** issue:
    - Reference specific source files. See `AGENTS.md` for component descriptions.
 
 2. **Comment on the original `github/gh-aw` issue** linking to the newly created tracking issue. Use this exact format:
-
    > 🔗 AWF tracking issue: https://github.com/github/gh-aw-firewall/issues/{NUMBER}
 
    where `{NUMBER}` is replaced with **only the numeric issue number** (e.g., `1896`). Do NOT include the repository name, hash symbols, or any other text — just the number in the URL path. Use the `add_comment` safe output tool with `repo: "github/gh-aw"` and the original issue number.
 
+   where `{NUMBER}` is replaced with **only the numeric issue number** (e.g., `1896`). Do NOT include the repository name, hash symbols, or any other text — just the number in the URL path. Use the `add_comment` safe output tool with `repo: "github/gh-aw"` and the original issue number.
+
 ### 4. Report Results
 
 Report: issues found, skipped (already audited), tracking issues created.

containers/agent/entrypoint.sh

@@ -783,6 +783,21 @@ AWFEOF
     fi
   fi
 
+  # Ensure ~/.gemini exists and is owned by the chroot user.
+  # If Docker created this directory as root:root (because it did not exist on the
+  # host at container start time), the Gemini CLI cannot write its project registry
+  # (atomic rename of projects.json.tmp → projects.json fails with ENOENT).
+  # AWF pre-creates this directory host-side in writeConfigs(), but on first run or
+  # after a previous failed run the directory may still be root-owned.
+  # We fix ownership here (as root, before privilege drop) as a defense-in-depth measure.
+  GEMINI_DIR="/host${HOME}/.gemini"
+  mkdir -p "${GEMINI_DIR}" 2>/dev/null || true
+  if chown "${HOST_UID}:${HOST_GID}" "${GEMINI_DIR}" 2>/dev/null; then
+    echo "[entrypoint] Ensured ~/.gemini ownership for chroot user (${HOST_UID}:${HOST_GID})"
+  else
+    echo "[entrypoint][WARN] Could not set ~/.gemini ownership to chroot user (${HOST_UID}:${HOST_GID})"
+  fi
+
   # Build LD_PRELOAD command for one-shot token protection
   LD_PRELOAD_CMD=""
   if [ -n "${ONE_SHOT_TOKEN_LIB}" ]; then

docs/environment.md

@@ -88,6 +88,32 @@ sudo -E awf --allow-domains github.com 'copilot --prompt "..."'
 - Working with untrusted code
 - In production/CI environments
 
+## `COPILOT_GITHUB_TOKEN` and Classic PAT Compatibility
+
+When `COPILOT_GITHUB_TOKEN` is set in the host environment, AWF injects it into the agent container so the Copilot CLI can authenticate against the GitHub Copilot API.
+
+### ⚠️ Classic PAT + `COPILOT_MODEL` Incompatibility (Copilot CLI 1.0.21+)
+
+Copilot CLI 1.0.21 introduced a startup model validation step: when `COPILOT_MODEL` is set, the CLI calls `GET /models` before executing any task. **This endpoint does not accept classic PATs** (`ghp_*` tokens), causing the agent to fail at startup with exit code 1 — before any useful work begins.
+
+**Affected combination:**
+- `COPILOT_GITHUB_TOKEN` is a classic PAT (prefixed with `ghp_`)
+- `COPILOT_MODEL` is set in the agent environment (e.g., via `--env COPILOT_MODEL=...`, `--env-file`, or `--env-all`)
+
+**Unaffected:** Workflows that do not set `COPILOT_MODEL` are not affected — the `/models` validation is only triggered when `COPILOT_MODEL` is set.
+
+**AWF detects this combination at startup** and emits a `[WARN]` message:
+```
+⚠️  COPILOT_MODEL is set with a classic PAT (ghp_* token)
+   Copilot CLI 1.0.21+ validates COPILOT_MODEL via GET /models at startup.
+   Classic PATs are rejected by this endpoint — the agent will likely fail with exit code 1.
+   Use a fine-grained PAT or OAuth token, or unset COPILOT_MODEL to skip model validation.
+```
+
+**Remediation options:**
+1. Replace the classic PAT with a **fine-grained PAT** or **OAuth token** (these are accepted by the `/models` endpoint).
+2. Remove `COPILOT_MODEL` from the agent environment to skip model validation entirely.
+
 ## Internal Environment Variables
 
 The following environment variables are set internally by the firewall and used by container scripts:

src/cli-workflow.test.ts

@@ -224,4 +224,90 @@ describe('runMainWorkflow', () => {
     expect(logger.warn).toHaveBeenCalledWith('Command completed with exit code: 42');
     expect(logger.success).not.toHaveBeenCalled();
   });
+
+  it('calls collectDiagnosticLogs before cleanup on non-zero exit when diagnosticLogs is enabled', async () => {
+    const callOrder: string[] = [];
+    const configWithDiagnostics: WrapperConfig = {
+      ...baseConfig,
+      diagnosticLogs: true,
+    };
+    const dependencies: WorkflowDependencies = {
+      ensureFirewallNetwork: jest.fn().mockResolvedValue({ squidIp: '172.30.0.10' }),
+      setupHostIptables: jest.fn().mockResolvedValue(undefined),
+      writeConfigs: jest.fn().mockResolvedValue(undefined),
+      startContainers: jest.fn().mockResolvedValue(undefined),
+      runAgentCommand: jest.fn().mockImplementation(async () => {
+        callOrder.push('runAgentCommand');
+        return { exitCode: 1 };
+      }),
+      collectDiagnosticLogs: jest.fn().mockImplementation(async () => {
+        callOrder.push('collectDiagnosticLogs');
+      }),
+    };
+    const performCleanup = jest.fn().mockImplementation(async () => {
+      callOrder.push('performCleanup');
+    });
+    const logger = createLogger();
+
+    await runMainWorkflow(configWithDiagnostics, dependencies, { logger, performCleanup });
+
+    expect(callOrder).toEqual(['runAgentCommand', 'collectDiagnosticLogs', 'performCleanup']);
+    expect(dependencies.collectDiagnosticLogs).toHaveBeenCalledWith(configWithDiagnostics.workDir);
+  });
+
+  it('does not call collectDiagnosticLogs when diagnosticLogs is disabled', async () => {
+    const collectDiagnosticLogs = jest.fn().mockResolvedValue(undefined);
+    const dependencies: WorkflowDependencies = {
+      ensureFirewallNetwork: jest.fn().mockResolvedValue({ squidIp: '172.30.0.10' }),
+      setupHostIptables: jest.fn().mockResolvedValue(undefined),
+      writeConfigs: jest.fn().mockResolvedValue(undefined),
+      startContainers: jest.fn().mockResolvedValue(undefined),
+      runAgentCommand: jest.fn().mockResolvedValue({ exitCode: 1 }),
+      collectDiagnosticLogs,
+    };
+    const logger = createLogger();
+
+    await runMainWorkflow(baseConfig, dependencies, { logger, performCleanup: jest.fn() });
+
+    expect(collectDiagnosticLogs).not.toHaveBeenCalled();
+  });
+
+  it('does not call collectDiagnosticLogs on zero exit even when diagnosticLogs is enabled', async () => {
+    const collectDiagnosticLogs = jest.fn().mockResolvedValue(undefined);
+    const configWithDiagnostics: WrapperConfig = {
+      ...baseConfig,
+      diagnosticLogs: true,
+    };
+    const dependencies: WorkflowDependencies = {
+      ensureFirewallNetwork: jest.fn().mockResolvedValue({ squidIp: '172.30.0.10' }),
+      setupHostIptables: jest.fn().mockResolvedValue(undefined),
+      writeConfigs: jest.fn().mockResolvedValue(undefined),
+      startContainers: jest.fn().mockResolvedValue(undefined),
+      runAgentCommand: jest.fn().mockResolvedValue({ exitCode: 0 }),
+      collectDiagnosticLogs,
+    };
+    const logger = createLogger();
+
+    await runMainWorkflow(configWithDiagnostics, dependencies, { logger, performCleanup: jest.fn() });
+
+    expect(collectDiagnosticLogs).not.toHaveBeenCalled();
+  });
+
+  it('does not call collectDiagnosticLogs when dependency is not provided', async () => {
+    const configWithDiagnostics: WrapperConfig = {
+      ...baseConfig,
+      diagnosticLogs: true,
+    };
+    const dependencies: WorkflowDependencies = {
+      ensureFirewallNetwork: jest.fn().mockResolvedValue({ squidIp: '172.30.0.10' }),
+      setupHostIptables: jest.fn().mockResolvedValue(undefined),
+      writeConfigs: jest.fn().mockResolvedValue(undefined),
+      startContainers: jest.fn().mockResolvedValue(undefined),
+      runAgentCommand: jest.fn().mockResolvedValue({ exitCode: 1 }),
+      // collectDiagnosticLogs not provided
+    };
+    const logger = createLogger();
+
+    await expect(runMainWorkflow(configWithDiagnostics, dependencies, { logger, performCleanup: jest.fn() })).resolves.toBe(1);
+  });
 });

src/cli-workflow.ts

@@ -14,6 +14,7 @@ export interface WorkflowDependencies {
     proxyLogsDir?: string,
     agentTimeoutMinutes?: number
   ) => Promise<{ exitCode: number }>;
+  collectDiagnosticLogs?: (workDir: string) => Promise<void>;
 }
 
 export interface WorkflowCallbacks {
@@ -76,6 +77,16 @@ export async function runMainWorkflow(
   // Step 3: Wait for agent to complete
   const result = await dependencies.runAgentCommand(config.workDir, config.allowedDomains, config.proxyLogsDir, config.agentTimeout);
 
+  // Step 3.5: Collect diagnostic logs before containers are stopped
+  // Must run BEFORE performCleanup() which calls docker compose down -v.
+  if (config.diagnosticLogs && result.exitCode !== 0 && dependencies.collectDiagnosticLogs) {
+    try {
+      await dependencies.collectDiagnosticLogs(config.workDir);
+    } catch (error) {
+      logger.warn('Failed to collect diagnostic logs; continuing with cleanup.', error);
+    }
+  }
+
   // Step 4: Cleanup (logs will be preserved automatically if they exist)
   await performCleanup();
 

src/cli.test.ts

@@ -1,5 +1,5 @@
 import { Command } from 'commander';
-import { parseEnvironmentVariables, parseDomains, parseDomainsFile, escapeShellArg, joinShellArgs, parseVolumeMounts, isValidIPv4, isValidIPv6, parseDnsServers, parseDnsOverHttps, validateAgentImage, isAgentImagePreset, AGENT_IMAGE_PRESETS, processAgentImageOption, processLocalhostKeyword, validateSkipPullWithBuildLocal, validateAllowHostPorts, validateAllowHostServicePorts, applyHostServicePortsConfig, parseMemoryLimit, validateFormat, validateApiProxyConfig, buildRateLimitConfig, validateRateLimitFlags, hasRateLimitOptions, collectRulesetFile, validateApiTargetInAllowedDomains, DEFAULT_OPENAI_API_TARGET, DEFAULT_ANTHROPIC_API_TARGET, DEFAULT_COPILOT_API_TARGET, DEFAULT_GEMINI_API_TARGET, emitApiProxyTargetWarnings, emitCliProxyStatusLogs, formatItem, program, parseAgentTimeout, applyAgentTimeout, handlePredownloadAction, resolveApiTargetsToAllowedDomains, extractGhesDomainsFromEngineApiTarget, extractGhecDomainsFromServerUrl } from './cli';
+import { parseEnvironmentVariables, parseDomains, parseDomainsFile, escapeShellArg, joinShellArgs, parseVolumeMounts, isValidIPv4, isValidIPv6, parseDnsServers, parseDnsOverHttps, validateAgentImage, isAgentImagePreset, AGENT_IMAGE_PRESETS, processAgentImageOption, processLocalhostKeyword, validateSkipPullWithBuildLocal, validateAllowHostPorts, validateAllowHostServicePorts, applyHostServicePortsConfig, parseMemoryLimit, validateFormat, validateApiProxyConfig, buildRateLimitConfig, validateRateLimitFlags, hasRateLimitOptions, collectRulesetFile, validateApiTargetInAllowedDomains, DEFAULT_OPENAI_API_TARGET, DEFAULT_ANTHROPIC_API_TARGET, DEFAULT_COPILOT_API_TARGET, DEFAULT_GEMINI_API_TARGET, emitApiProxyTargetWarnings, emitCliProxyStatusLogs, warnClassicPATWithCopilotModel, formatItem, program, parseAgentTimeout, applyAgentTimeout, handlePredownloadAction, resolveApiTargetsToAllowedDomains, extractGhesDomainsFromEngineApiTarget, extractGhecDomainsFromServerUrl } from './cli';
 import { redactSecrets } from './redact-secrets';
 import * as fs from 'fs';
 import * as path from 'path';
@@ -2112,6 +2112,46 @@ describe('cli', () => {
     });
   });
 
+  describe('warnClassicPATWithCopilotModel', () => {
+    it('should emit warnings when classic PAT and COPILOT_MODEL are both set', () => {
+      const warns: string[] = [];
+      warnClassicPATWithCopilotModel(true, true, (msg) => warns.push(msg));
+      expect(warns.length).toBeGreaterThan(0);
+      expect(warns[0]).toContain('COPILOT_MODEL');
+      expect(warns.some(w => w.includes('classic PAT'))).toBe(true);
+    });
+
+    it('should not warn when token is not a classic PAT', () => {
+      const warns: string[] = [];
+      warnClassicPATWithCopilotModel(false, true, (msg) => warns.push(msg));
+      expect(warns).toHaveLength(0);
+    });
+
+    it('should not warn when COPILOT_MODEL is not set', () => {
+      const warns: string[] = [];
+      warnClassicPATWithCopilotModel(true, false, (msg) => warns.push(msg));
+      expect(warns).toHaveLength(0);
+    });
+
+    it('should not warn when neither condition holds', () => {
+      const warns: string[] = [];
+      warnClassicPATWithCopilotModel(false, false, (msg) => warns.push(msg));
+      expect(warns).toHaveLength(0);
+    });
+
+    it('should mention /models endpoint in warning', () => {
+      const warns: string[] = [];
+      warnClassicPATWithCopilotModel(true, true, (msg) => warns.push(msg));
+      expect(warns.some(w => w.includes('/models'))).toBe(true);
+    });
+
+    it('should mention exit code 1 in warning', () => {
+      const warns: string[] = [];
+      warnClassicPATWithCopilotModel(true, true, (msg) => warns.push(msg));
+      expect(warns.some(w => w.includes('exit code 1'))).toBe(true);
+    });
+  });
+
   describe('resolveApiTargetsToAllowedDomains', () => {
     it('should add copilot-api-target option to allowed domains', () => {
       const domains: string[] = ['github.com'];

src/cli.ts

@@ -15,6 +15,7 @@ import {
   cleanup,
   preserveIptablesAudit,
   fastKillAgentContainer,
+  collectDiagnosticLogs,
 } from './docker-manager';
 import {
   ensureFirewallNetwork,
@@ -419,6 +420,30 @@ export function emitCliProxyStatusLogs(
   }
 }
 
+/**
+ * Warns when a classic GitHub PAT (ghp_* prefix) is used alongside COPILOT_MODEL.
+ * Copilot CLI 1.0.21+ performs a GET /models validation at startup when COPILOT_MODEL
+ * is set. This endpoint rejects classic PATs, causing the agent to fail with exit code 1
+ * before any useful work begins.
+ * Accepts booleans (not actual tokens/values) to prevent sensitive data from flowing
+ * through to log output (CodeQL: clear-text logging of sensitive information).
+ * @param isClassicPAT - Whether COPILOT_GITHUB_TOKEN starts with 'ghp_' (classic PAT)
+ * @param hasCopilotModel - Whether COPILOT_MODEL is set in the agent environment
+ * @param warn - Function to emit a warning message
+ */
+export function warnClassicPATWithCopilotModel(
+  isClassicPAT: boolean,
+  hasCopilotModel: boolean,
+  warn: (msg: string) => void,
+): void {
+  if (!isClassicPAT || !hasCopilotModel) return;
+
+  warn('⚠️  COPILOT_MODEL is set with a classic PAT (ghp_* token)');
+  warn('   Copilot CLI 1.0.21+ validates COPILOT_MODEL via GET /models at startup.');
+  warn('   Classic PATs are rejected by this endpoint — the agent will likely fail with exit code 1.');
+  warn('   Use a fine-grained PAT or OAuth token, or unset COPILOT_MODEL to skip model validation.');
+}
+
 /**
  * Extracts GHEC domains from GITHUB_SERVER_URL and GITHUB_API_URL environment variables.
  * When GITHUB_SERVER_URL points to a GHEC tenant (*.ghe.com), returns the tenant hostname,
@@ -1484,6 +1509,13 @@ program
     '--session-state-dir <path>',
     'Directory to save Copilot CLI session state (events.jsonl, session data)'
   )
+  .option(
+    '--diagnostic-logs',
+    'Collect container logs, exit state, and sanitized config on non-zero exit.\n' +
+    '                                       Useful for debugging container startup failures (e.g. Squid crashes in DinD).\n' +
+    '                                       Written to <workDir>/diagnostics/ (or <audit-dir>/diagnostics/ when set).',
+    false
+  )
   .argument('[args...]', 'Command and arguments to execute (use -- to separate from options)')
   .action(async (args: string[], options) => {
     // Require -- separator for passing command arguments
@@ -1826,6 +1858,7 @@ program
       difcProxyHost: options.difcProxyHost,
       difcProxyCaCert: options.difcProxyCaCert,
       githubToken: process.env.GITHUB_TOKEN || process.env.GH_TOKEN,
+      diagnosticLogs: options.diagnosticLogs || false,
     };
 
     // Parse and validate --agent-timeout
@@ -1926,6 +1959,39 @@ program
     // Log CLI proxy status
     emitCliProxyStatusLogs(config, logger.info.bind(logger), logger.warn.bind(logger));
 
+    // Warn if a classic PAT is combined with COPILOT_MODEL (Copilot CLI 1.0.21+ incompatibility)
+    const hasCopilotModelInEnvFiles = (envFile: unknown): boolean => {
+      const envFiles = Array.isArray(envFile) ? envFile : envFile ? [envFile] : [];
+      for (const candidate of envFiles) {
+        if (typeof candidate !== 'string' || candidate.trim() === '') continue;
+        try {
+          const envFilePath = path.isAbsolute(candidate) ? candidate : path.resolve(process.cwd(), candidate);
+          const envFileContents = fs.readFileSync(envFilePath, 'utf8');
+          for (const line of envFileContents.split(/\r?\n/)) {
+            const trimmedLine = line.trim();
+            if (!trimmedLine || trimmedLine.startsWith('#')) continue;
+            if (/^(?:export\s+)?COPILOT_MODEL\s*=/.test(trimmedLine)) {
+              return true;
+            }
+          }
+        } catch {
+          // Ignore unreadable env files here; this check is only for a pre-flight warning.
+        }
+      }
+      return false;
+    };
+
+    // Warn if a classic PAT is combined with COPILOT_MODEL (Copilot CLI 1.0.21+ incompatibility)
+    // Check if COPILOT_MODEL is set via --env/-e flags, host env (when --env-all is active), or --env-file
+    const copilotModelFromFlags = !!(additionalEnv['COPILOT_MODEL']);
+    const copilotModelInHostEnv = !!(config.envAll && process.env.COPILOT_MODEL);
+    const copilotModelInEnvFile = hasCopilotModelInEnvFiles((config as { envFile?: unknown }).envFile);
+    warnClassicPATWithCopilotModel(
+      config.copilotGithubToken?.startsWith('ghp_') ?? false,
+      copilotModelFromFlags || copilotModelInHostEnv || copilotModelInEnvFile,
+      logger.warn.bind(logger)
+    );
+
     // Log config with redacted secrets - remove API keys entirely
     // to prevent sensitive data from flowing to logger (CodeQL sensitive data logging)
     const redactedConfig: Record<string, unknown> = {};
@@ -2006,6 +2072,7 @@ program
           writeConfigs,
           startContainers,
           runAgentCommand,
+          collectDiagnosticLogs,
         },
         {
           logger,