fix: add tests to recover branch coverage regression in cli.ts

[Copilot]

The previous commit added new code to src/cli.ts (the --env-file option and catch block in handlePredownloadAction) without sufficient test coverage, dropping overall branch coverage from 76.06% → 76.02% and failing the Test Coverage Report CI job.

Changes

• handlePredownloadAction error path — the catch block on lines 1951–1952 was never exercised; added two tests covering both sides of the ?? operator:

  • Error with exitCode property → process.exit(exitCode)
  • Error without exitCode → process.exit(1) (default)

• Default parameter branches — extractGhecDomainsFromServerUrl() and extractGhesDomainsFromEngineApiTarget() were always called with an explicit env argument in tests, leaving the env = process.env default branch uncovered; added tests that call each without arguments

// Previously every test passed an explicit env, leaving branch 0 uncovered
extractGhecDomainsFromServerUrl({})  // covered branch 1 only

// New test exercises the default parameter branch
extractGhecDomainsFromServerUrl()    // uses process.env default

Result: overall branch coverage 75.96% → 76.19% (+0.23%), src/cli.ts branch coverage 58.41% → 59.37%.

Original prompt

Fix the failing GitHub Actions workflow Test Coverage Report
Analyze the workflow logs, identify the root cause of the failure, and implement a fix.
Job ID: 68781676235
Job URL: https://github.com/github/gh-aw-firewall/actions/runs/23615187925/job/68781676235

---

💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.

[Copilot]

Pull request overview

Adds missing unit tests to restore/raise branch coverage in src/cli.ts by exercising previously-uncovered error/exit handling and default-parameter branches, addressing the Test Coverage Report CI failure.

Changes:

• Added tests for handlePredownloadAction’s catch path to cover both error.exitCode ?? 1 branches.
• Added tests calling extractGhesDomainsFromEngineApiTarget() / extractGhecDomainsFromServerUrl() without arguments to cover the env = process.env default branch.

Comments suppressed due to low confidence (1)

src/cli.test.ts:2534

• This test mutates process.env.GITHUB_SERVER_URL / process.env.GITHUB_API_URL and restores them only at the end. To guarantee cleanup even if the expectation fails, restore the saved env values in a finally block (or in afterEach).

    it('should use process.env by default when no env argument is provided', () => {
      const savedServerUrl = process.env.GITHUB_SERVER_URL;
      const savedApiUrl = process.env.GITHUB_API_URL;
      delete process.env.GITHUB_SERVER_URL;
      delete process.env.GITHUB_API_URL;
      const domains = extractGhecDomainsFromServerUrl();
      expect(domains).toEqual([]);
      if (savedServerUrl !== undefined) process.env.GITHUB_SERVER_URL = savedServerUrl;
      if (savedApiUrl !== undefined) process.env.GITHUB_API_URL = savedApiUrl;
    });

---

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

[Copilot]

These tests mutate global state (process.exit spy) and only restore it at the end of the test body. If an assertion throws before mockRestore() runs, the spy can leak into subsequent tests and cause cascading failures. Prefer wrapping the test body in try/finally, or move the mockRestore() into an afterEach within this describe block so cleanup is guaranteed.

Suggested change

	const errorWithExitCode = Object.assign(new Error('pull failed'), { exitCode: 2 });
	jest.resetModules();
	jest.doMock('./commands/predownload', () => ({
	predownloadCommand: jest.fn().mockRejectedValue(errorWithExitCode),
	}));
	await expect(handlePredownloadAction({
	imageRegistry: 'test',
	imageTag: 'latest',
	agentImage: 'default',
	enableApiProxy: false,
	})).rejects.toThrow('process.exit called');
	expect(mockExit).toHaveBeenCalledWith(2);
	mockExit.mockRestore();
	});
	it('should default to exit code 1 when error has no exitCode property', async () => {
	const mockExit = jest.spyOn(process, 'exit').mockImplementation(() => {
	throw new Error('process.exit called');
	});
	const errorWithoutExitCode = new Error('unexpected failure');
	jest.resetModules();
	jest.doMock('./commands/predownload', () => ({
	predownloadCommand: jest.fn().mockRejectedValue(errorWithoutExitCode),
	}));
	await expect(handlePredownloadAction({
	imageRegistry: 'test',
	imageTag: 'latest',
	agentImage: 'default',
	enableApiProxy: false,
	})).rejects.toThrow('process.exit called');
	expect(mockExit).toHaveBeenCalledWith(1);
	mockExit.mockRestore();
	try {
	const errorWithExitCode = Object.assign(new Error('pull failed'), { exitCode: 2 });
	jest.resetModules();
	jest.doMock('./commands/predownload', () => ({
	predownloadCommand: jest.fn().mockRejectedValue(errorWithExitCode),
	}));
	await expect(handlePredownloadAction({
	imageRegistry: 'test',
	imageTag: 'latest',
	agentImage: 'default',
	enableApiProxy: false,
	})).rejects.toThrow('process.exit called');
	expect(mockExit).toHaveBeenCalledWith(2);
	} finally {
	mockExit.mockRestore();
	}
	});
	it('should default to exit code 1 when error has no exitCode property', async () => {
	const mockExit = jest.spyOn(process, 'exit').mockImplementation(() => {
	throw new Error('process.exit called');
	});
	try {
	const errorWithoutExitCode = new Error('unexpected failure');
	jest.resetModules();
	jest.doMock('./commands/predownload', () => ({
	predownloadCommand: jest.fn().mockRejectedValue(errorWithoutExitCode),
	}));
	await expect(handlePredownloadAction({
	imageRegistry: 'test',
	imageTag: 'latest',
	agentImage: 'default',
	enableApiProxy: false,
	})).rejects.toThrow('process.exit called');
	expect(mockExit).toHaveBeenCalledWith(1);
	} finally {
	mockExit.mockRestore();
	}

[Copilot]

This test deletes/modifies process.env.ENGINE_API_TARGET and restores it only at the end of the test. To avoid leaking environment changes when an assertion fails (which can make later tests flaky), restore the original value in a finally block (or use beforeEach/afterEach around these env-mutation tests).

This issue also appears on line 2525 of the same file.

Suggested change

	delete process.env.ENGINE_API_TARGET;
	const domains = extractGhesDomainsFromEngineApiTarget();
	expect(domains).toEqual([]);
	if (saved !== undefined) process.env.ENGINE_API_TARGET = saved;
	try {
	delete process.env.ENGINE_API_TARGET;
	const domains = extractGhesDomainsFromEngineApiTarget();
	expect(domains).toEqual([]);
	} finally {
	if (saved !== undefined) {
	process.env.ENGINE_API_TARGET = saved;
	} else {
	delete process.env.ENGINE_API_TARGET;
	}
	}