Skip to content

Refactor duplicated main-action Jest harness into shared test utility - #6607

Merged
lpcox merged 3 commits into
mainfrom
copilot/duplicate-code-main-action-harness
Jul 26, 2026
Merged

Refactor duplicated main-action Jest harness into shared test utility#6607
lpcox merged 3 commits into
mainfrom
copilot/duplicate-code-main-action-harness

Conversation

Copilot AI commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

The two main-action test suites carried duplicated harness setup (fs mocks, shared wrapper config fixture, and default dependency wiring in beforeEach). This change centralizes that setup so both suites consume one source of truth and only keep suite-specific overrides.

  • What changed

    • Added /home/runner/work/gh-aw-firewall/gh-aw-firewall/src/commands/main-action.test-utils.ts with:
      • MAIN_ACTION_STUB_CONFIG (shared baseline WrapperConfig fixture)
      • setupMainActionTestHarness(...) (shared default mock/spy wiring)
    • Updated:
      • /home/runner/work/gh-aw-firewall/gh-aw-firewall/src/commands/main-action.test.ts
      • /home/runner/work/gh-aw-firewall/gh-aw-firewall/src/commands/main-action-coverage-gaps.test.ts
    • Removed duplicated inline STUB_CONFIG and duplicated beforeEach default wiring in both suites.
  • Refactor shape

    • Both suites now initialize common test state through the helper and keep local test intent focused on branch behavior.
const harness = setupMainActionTestHarness({
  mockedPreflight,
  mockedValidateOptions,
  mockedDockerManager,
  mockedRedactSecrets,
  mockedOptionParsers,
  mockedDindProbe,
  mockedDindBootstrap,
  mockedSignalHandler,
  mockedCliWorkflow,
  mockedSbxManager,
});

processExitSpy = harness.processExitSpy;
consoleErrorSpy = harness.consoleErrorSpy;
getOptionValueSource = harness.getOptionValueSource;

Copilot AI changed the title [WIP] Refactor duplicated main-action workflow test harness Refactor duplicated main-action Jest harness into shared test utility Jul 26, 2026
Copilot AI requested a review from lpcox July 26, 2026 16:09
Copilot finished work on behalf of lpcox July 26, 2026 16:09
@lpcox
lpcox marked this pull request as ready for review July 26, 2026 16:12
Copilot AI review requested due to automatic review settings July 26, 2026 16:13

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 duplicated main-action Jest setup into a shared test utility.

Changes:

  • Adds a shared configuration fixture and harness setup.
  • Updates both test suites to consume the shared utility.
Show a summary per file
File Description
src/commands/main-action.test-utils.ts Defines the shared fixture and harness.
src/commands/main-action.test.ts Replaces duplicated setup with the harness.
src/commands/main-action-coverage-gaps.test.ts Reuses the shared setup and fixture.

Review details

Tip

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

  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Medium

Comment thread src/commands/main-action.test-utils.ts Outdated
Comment on lines +20 to +24
interface MainActionHarnessDeps {
mockedPreflight: { applyConfigFilePrecedence: unknown };
mockedValidateOptions: { validateOptions: unknown };
mockedDockerManager: { setAwfDockerHost: unknown };
mockedRedactSecrets: { redactSecrets: unknown };
getOptionValueSource: jest.Mock;
}

export function setupMainActionTestHarness(deps: MainActionHarnessDeps): MainActionTestHarness {
@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

⏳ Copilot review left inline comments.

@copilot To proceed:

  1. Ask @copilot to address the review feedback (reply to this comment or the review thread)
  2. Once the fix is pushed, add the ready-for-aw label to trigger agentic CI smoke tests

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 99.12% 99.17% 📈 +0.05%
Statements 99.03% 99.08% 📈 +0.05%
Functions 99.52% 99.53% 📈 +0.01%
Branches 95.60% 95.63% 📈 +0.03%
📁 Per-file Coverage Changes (2 files)
File Lines (Before → After) Statements (Before → After)
src/upstream-proxy.ts 94.5% → 95.9% (+1.37%) 94.9% → 96.2% (+1.27%)
src/log-directory-setup.ts 96.2% → 100.0% (+3.78%) 96.3% → 100.0% (+3.71%)
✨ New Files (1 files)
  • src/commands/main-action.test-utils.ts: 100.0% lines

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

@lpcox

lpcox commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested change, apply the suggestion exactly.

Do not make changes beyond what is described in the linked review thread.

Copilot AI commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested change, app...

Both issues are addressed in commit refactor(test-utils): use typed Pick<jest.Mocked> for harness deps:

Comment 1 (unknown types): MainActionHarnessDeps now uses Pick<jest.Mocked<typeof module>, 'methodName'> for each field (via import type * as ... for every mocked module). The as jest.Mock casts in the function body are removed — the properly-typed mocked methods are called directly.

Comment 2 (fs mocks): jest.mock('fs') with a factory closure cannot be centralized here because Jest hoists jest.mock calls before import statements, so the factory must close over variables declared in the same test file. Added a comment above setupMainActionTestHarness in the test-utils file documenting this constraint.

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Smoke Claude passed

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded.

@github-actions

Copy link
Copy Markdown
Contributor

🚀 Security Guard has started processing this pull request

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

🛡️ Smoke Copilot Network Isolation confirmed the egress allowlist is enforced. ✅

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

📰 VERDICT: Smoke Docker Sbx has concluded. All systems operational. This is a developing story. 🎤

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓

@github-actions

Copy link
Copy Markdown
Contributor

Smoke Test: Claude Engine Validation

Check Result
API status ✅ PASS
GH check ✅ PASS
File status ✅ PASS

Overall result: PASS

Generated by Smoke Claude for #6607 · haiku45 · 54 AIC · ⊞ 3.3K ·
Add label ready-for-aw to run again

@github-actions

Copy link
Copy Markdown
Contributor

🔥 Smoke Test Results

Test Status
GitHub MCP connectivity
GitHub.com HTTP ✅ 200
File write/read

Overall: PASS

📰 BREAKING: Report filed by Smoke Copilot
Add label ready-for-aw to run again

@github-actions

Copy link
Copy Markdown
Contributor

@lpcox — Smoke test: Copilot Network Isolation Egress Enforcement

EGRESS_RESULT allow=pass deny=pass

  • ✅ Allowed domain (api.github.com): allowed=200
  • ✅ Blocked domain (example.com): OK: example.com was blocked (CONNECT tunnel 403)

Overall status: PASS

Warning

Firewall blocked 1 domain

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

  • example.com

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

network:
  allowed:
    - defaults
    - "example.com"

See Network Configuration for more information.

🛡️ Egress verdict from Smoke Copilot Network Isolation
Add label ready-for-aw to run again

@github-actions github-actions Bot added the smoke-copilot-network-isolation Copilot network-isolation egress smoke test label Jul 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Smoke Test: GitHub Actions Services Connectivity

  • Redis PING: ❌ host.docker.internal name resolution failed
  • PostgreSQL pg_isready: ❌ no response
  • PostgreSQL SELECT 1: ❌ name resolution failed

Overall: FAILhost.docker.internal is not resolvable in this environment.

🔌 Service connectivity validated by Smoke Services
Add label ready-for-aw to run again

@github-actions

Copy link
Copy Markdown
Contributor

Smoke Test: Copilot BYOK (Direct) Mode ✅ PASS

  • ✅ GitHub MCP connectivity: Working
  • ✅ GitHub.com connectivity: HTTP 200
  • ✅ File I/O (write/read): Working
  • ✅ BYOK inference: Confirmed (running in direct BYOK mode via api-proxy → api.githubcopilot.com)

Running with COPILOT_PROVIDER_API_KEY — agent receives placeholder, sidecar injects real credential for api.githubcopilot.com requests.

🔑 BYOK report filed by Smoke Copilot BYOK
Add label ready-for-aw to run again

@github-actions

Copy link
Copy Markdown
Contributor

Chroot Version Comparison Results

Runtime Host Version Chroot Version Match?
Python Python 3.12.13 Python 3.12.13 ✅ YES
Node.js v24.18.0 v22.23.1 ❌ NO
Go go1.22.12 go1.22.12 ✅ YES

Overall: ❌ FAILED — Node.js version mismatch between host (v24.18.0) and chroot (v22.23.1).

Tested by Smoke Chroot
Add label ready-for-aw to run again

@github-actions

Copy link
Copy Markdown
Contributor

✅ Refactor API proxy providers to share OIDC/static auth adapter scaffold
✅ chore(deps): bump eslint to 10.8.0
✅ GitHub PR query
✅ GitHub discussion query
✅ Playwright GitHub title
✅ File write check
✅ Build check
Overall: PASS

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
Add label ready-for-aw to run again

@github-actions

Copy link
Copy Markdown
Contributor

Smoke Test: FAIL (Connectivity/MCP issues)

Warning

Firewall blocked 1 domain

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

  • localhost

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

network:
  allowed:
    - defaults
    - "localhost"

See Network Configuration for more information.

💎 Faceted by Smoke Gemini
Add label ready-for-aw to run again

@github-actions

Copy link
Copy Markdown
Contributor

@lpcox
Smoke Test Results:

  • GitHub MCP Testing: ✅
  • GitHub.com Connectivity: ✅
  • File Write/Read Test: ✅
  • BYOK Inference Test: ✅

Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw)

Overall Status: PASS

🔑 BYOK (AOAI api-key) report filed by Smoke Copilot BYOK AOAI (api-key)
Add label ready-for-aw to run again

@github-actions

Copy link
Copy Markdown
Contributor

🔬 Smoke Test: API Proxy OpenTelemetry Tracing

Scenario Result Details
S1: Module Loading ✅ Pass otel.js loads successfully; exports: startRequestSpan, setTokenAttributes, setBudgetAttributes, endSpan, endSpanError, shutdown, isEnabled + test helpers. isEnabled() returns true (file fallback active).
S2: Test Suite ✅ Pass 39/39 tests pass in otel.test.js — spans, attributes, exporters, shutdown all covered.
S3: Env Var Forwarding ✅ Pass src/services/api-proxy-env-config.ts forwards GH_AW_OTLP_ENDPOINTS, OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS, GITHUB_AW_OTEL_TRACE_ID, GITHUB_AW_OTEL_PARENT_SPAN_ID to the api-proxy container. Tests confirmed in api-proxy-env-config.test.ts.
S4: Token Tracker Integration ✅ Pass onUsage callback exists in token-tracker-http.js (line 406: if (typeof onUsage === 'function')). This is the OTEL hook point where setTokenAttributes is called.
S5: OTEL Diagnostics ✅ Pass No OTLP endpoint configured; module falls back to FileSpanExporter writing to /var/log/api-proxy/otel.jsonl — graceful degradation confirmed.

All 5 scenarios pass. OTEL tracing integration is fully functional with correct GenAI semantic conventions, parent context propagation, and graceful degradation when no endpoint is configured.

📡 OTel tracing validated by Smoke OTel Tracing
Add label ready-for-aw to run again

@github-actions

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

Note (Java): Maven failed to create the local repo at ~/.m2/repository on first attempt (permission/path issue). Retried with -Dmaven.repo.local=/tmp/gh-aw/agent/m2-repo — all Java tests passed successfully.

Generated by Build Test Suite for #6607 · sonnet46 · 59.3 AIC · ⊞ 8.3K ·
Add label ready-for-aw to run again

@github-actions

Copy link
Copy Markdown
Contributor

🔥 Smoke Test Results — Docker Sbx

Test Result
GitHub MCP Connectivity ✅ Reachable (secrecy-filtered, expected)
GitHub.com HTTP ⚠️ Pre-step data unavailable (template vars not substituted)
File Write/Read ⚠️ Pre-step data unavailable (template vars not substituted)

Overall: PARTIAL — MCP connectivity confirmed; pre-computed test data was not injected (template substitution issue in workflow).

@lpcox — ping for awareness.

📰 BREAKING: Report filed by Smoke Docker Sbx
Add label ready-for-aw to run again

@lpcox

lpcox commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Copilot AI commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

@copilot fix this failing ci check https://github.com/github/gh-aw-firewall/pull/6607/checks?check_run_id=89842387886

I investigated the failing check but cannot access the GitHub API from this environment (DNS proxy blocks api.github.com), so I can't view the check run log directly. All local checks pass cleanly: npm run build, npm run type-check (0 errors), npm run lint (0 errors), npm run test:coverage (4025 tests pass), and check-file-permissions.

The most likely cause is the PR title check (pr-title.yml). The current title "Refactor duplicated main-action Jest harness into shared test utility" doesn't conform to the required conventional commits format (type: subject). It needs to start with an allowed type, for example:

refactor: centralize main-action jest harness into shared test utility

Could you update the PR title? I don't have a tool to edit it directly. All other CI checks should be passing.

Copilot finished work on behalf of lpcox July 26, 2026 21:54
@lpcox
lpcox merged commit 42610f9 into main Jul 26, 2026
137 of 140 checks passed
@lpcox
lpcox deleted the copilot/duplicate-code-main-action-harness branch July 26, 2026 22:56
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] Main-action workflow test harness is repeated across coverage suites

3 participants