Skip to content

Refactor writeConfigs cleanup tests to remove duplicated config + tolerant-failure scaffolding#5352

Open
Copilot wants to merge 4 commits into
mainfrom
copilot/duplicate-code-writeconfigs-tests
Open

Refactor writeConfigs cleanup tests to remove duplicated config + tolerant-failure scaffolding#5352
Copilot wants to merge 4 commits into
mainfrom
copilot/duplicate-code-writeconfigs-tests

Conversation

Copilot AI commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

src/docker-manager-cleanup.test.ts repeated the same minimal WrapperConfig construction and identical writeConfigs try/catch blocks across multiple writeConfigs tests. This PR consolidates that pattern so tests focus on filesystem/permission assertions rather than setup boilerplate.

  • What changed

    • Added a local createWriteConfig(overrides?) helper in the writeConfigs test block to centralize baseline config creation.
    • Added a local writeConfigsAllowingFailure(config) helper to encapsulate the shared “allow failure, keep side-effect assertions” behavior.
    • Replaced repeated inline config literals and try/catch blocks in the targeted tests (work dir creation, log dirs, config file checks, permission checks, proxyLogsDir variants, chroot-home and gemini-home precreation).
    • Clarified helper/comment wording to avoid implying failures are always late.
    • Corrected the squid.conf test comment to avoid claiming an incorrect write order.
  • Why this structure

    • Keeps partial-success semantics in one place.
    • Reduces repeated setup code and makes each test’s assertion intent easier to scan.
const createWriteConfig = (overrides: Partial<WrapperConfig> = {}): WrapperConfig => ({
  allowedDomains: ['github.com'],
  agentCommand: 'echo test',
  logLevel: 'info',
  keepContainers: false,
  workDir: getDir(),
  ...overrides,
});

const writeConfigsAllowingFailure = async (config: WrapperConfig): Promise<void> => {
  try {
    await writeConfigs(config);
  } catch {
    // Some tests assert side effects that may exist even when writeConfigs throws.
  }
};

Copilot AI changed the title [WIP] Refactor duplicated writeConfigs tests with shared helper Refactor writeConfigs cleanup tests to remove duplicated config + tolerant-failure scaffolding Jun 21, 2026
Copilot finished work on behalf of lpcox June 21, 2026 16:06
Copilot AI requested a review from lpcox June 21, 2026 16:06
@lpcox lpcox marked this pull request as ready for review June 21, 2026 17:12
Copilot AI review requested due to automatic review settings June 21, 2026 17:12

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

This PR refactors src/docker-manager-cleanup.test.ts to reduce repeated WrapperConfig setup and repeated writeConfigs try/catch scaffolding, keeping the tests focused on filesystem side effects and permissions.

Changes:

  • Added createWriteConfig(overrides) helper to centralize baseline WrapperConfig construction for writeConfigs tests.
  • Added writeConfigsAllowingLateFailure(config) helper to consolidate the “ignore errors but keep side-effect assertions” pattern.
  • Updated multiple writeConfigs tests to use these helpers (work dir creation, log dirs, config file checks, permission checks, proxyLogsDir, and gemini/chroot-home precreation).
Show a summary per file
File Description
src/docker-manager-cleanup.test.ts Consolidates repeated config setup and error-tolerant writeConfigs invocation into local helpers to reduce boilerplate in cleanup/config tests.

Copilot's findings

Tip

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

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

Comment thread src/docker-manager-cleanup.test.ts Outdated
Comment on lines 41 to 46
const writeConfigsAllowingLateFailure = async (config: WrapperConfig): Promise<void> => {
try {
await writeConfigs(config);
} catch {
// Expected to fail if seccomp profile not found, but directories should still be created
// writeConfigs may fail late (e.g., seccomp/config writes), but earlier side effects should remain
}
Comment thread src/docker-manager-cleanup.test.ts Outdated
const config = createWriteConfig({ allowedDomains: ['github.com', 'example.com'] });
await writeConfigsAllowingLateFailure(config);

// Verify squid.conf was created (it's created before seccomp check)
@lpcox

lpcox commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

@copilot address review feedback

Copilot AI commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

@copilot address review feedback

Addressed. I updated src/docker-manager-cleanup.test.ts to clarify failure semantics by renaming the helper to writeConfigsAllowingFailure with an accurate catch comment, and fixed the squid.conf assertion comment to avoid incorrect ordering claims. Commit: 24b1669.

@github-actions

github-actions Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

🔌 Smoke Services — All services reachable! ✅

@github-actions

github-actions Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actions Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Smoke Claude passed

@github-actions

github-actions Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actions Bot commented Jun 21, 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 Jun 21, 2026

Copy link
Copy Markdown
Contributor

Smoke Gemini completed. All facets verified. 💎

Testing GitHub MCP connectivity

@github-actions

github-actions Bot commented Jun 21, 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 Jun 21, 2026

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actions Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓

@github-actions

github-actions Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Build Test Suite completed successfully!

@github-actions

github-actions Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓

@github-actions

github-actions Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅

@github-actions

Copy link
Copy Markdown
Contributor

Smoke Test: Copilot BYOK (Direct Mode) — PASS

  • ✅ GitHub MCP connectivity verified
  • ✅ GitHub.com connectivity (HTTP 200)
  • ✅ File write/read test passed
  • ✅ BYOK inference working (agent → api-proxy → api.githubcopilot.com)

Running in direct BYOK mode via COPILOT_PROVIDER_API_KEY with api-proxy sidecar. @lpcox

🔑 BYOK report filed by Smoke Copilot BYOK

@github-actions

Copy link
Copy Markdown
Contributor

🔬 Smoke Test Results

PR: Refactor writeConfigs cleanup tests to remove duplicated config + tolerant-failure scaffolding
Author: @Copilot | Assignees: @lpcox, @Copilot

Test Result
GitHub MCP connectivity
GitHub.com connectivity (HTTP 200)
File write/read ⚠️ pre-step vars unexpanded

Overall: PASS (2/3 verified; file test skipped due to unexpanded workflow template vars)

📰 BREAKING: Report filed by Smoke Copilot

@github-actions

Copy link
Copy Markdown
Contributor

Smoke Test Summary:

  • GitHub Connectivity: ❌ (SSL Error 35)
  • File Writing: ✅
  • Bash Tool: ✅
  • GitHub MCP: ✅ (Verified via safeoutputs)

PRs:

  1. Pending
  2. Pending

Overall Status: FAIL (Connectivity test failed)

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

@github-actions

Copy link
Copy Markdown
Contributor

🔬 Smoke Test: Copilot PAT Auth — PASS

Test Result
GitHub MCP connectivity
GitHub.com HTTP ✅ 200
File write/read ⚠️ pre-step data unsubstituted

Auth mode: PAT (COPILOT_GITHUB_TOKEN)

cc @lpcox @Copilot

🔑 PAT report filed by Smoke Copilot PAT

@github-actions

Copy link
Copy Markdown
Contributor

Smoke test results:

  • perf(security-guard): prioritize security-relevant files in PR diff build-test ready-for-aw smoke-claude smoke-codex smoke-copilot-byok smoke-copilot-byok-aoai-apikey smoke-copilot-byok-aoai-entra smoke-copilot-pat
  • fix(smoke-claude): raise turn budget to 8 and fix add_comment usage build-test ready-for-aw smoke-claude smoke-codex smoke-copilot-byok smoke-copilot-byok-aoai-apikey smoke-copilot-byok-aoai-entra
  • GitHub reads: ✅
  • Browser title: ✅
  • File write/read: ✅
  • Build: ✅
  • 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

@github-actions

Copy link
Copy Markdown
Contributor

🔭 Smoke Test: API Proxy OpenTelemetry Tracing

Scenario Status Details
1. Module Loading otel.js loads cleanly; exports 14 symbols: startRequestSpan, setTokenAttributes, setBudgetAttributes, endSpan, endSpanError, shutdown, isEnabled, plus internal helpers
2. Test Suite 59/59 tests pass across otel.test.js and otel-fanout.test.js; 0 failures
3. Env Var Forwarding api-proxy-service-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, OTEL_SERVICE_NAME to the sidecar container
4. Token Tracker Integration onUsage callback present in token-tracker-http.js (lines 283, 324, 374) as the OTEL hook point
5. OTEL Diagnostics No OTLP endpoint configured → graceful degradation to FileSpanExporter (/var/log/api-proxy/otel.jsonl); no errors

All scenarios pass. The OTEL tracing integration is fully functional with correct span creation, token usage attributes, parent context propagation support, and graceful degradation when unconfigured.

📡 OTel tracing validated by Smoke OTel Tracing

@github-actions

Copy link
Copy Markdown
Contributor

Chroot Runtime Version Comparison

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

Result: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.

Tested by Smoke Chroot

@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 1/1 passed ✅ PASS
Go env 1/1 passed ✅ PASS
Go uuid 1/1 passed ✅ PASS
Java gson 1/1 passed ✅ PASS
Java caffeine 1/1 passed ✅ PASS
Node.js clsx 1/1 passed ✅ PASS
Node.js execa 1/1 passed ✅ PASS
Node.js p-limit 1/1 passed ✅ PASS
Rust fd 1/1 passed ✅ PASS
Rust zoxide 1/1 passed ✅ PASS

Overall: 8/8 ecosystems passed — ✅ PASS

Generated by Build Test Suite for issue #5352 · 79.8 AIC · ⊞ 7.7K ·

@github-actions

Copy link
Copy Markdown
Contributor

Smoke Test Results — FAIL ❌

Check Result
Redis PING ❌ TCP timeout (port 6379 unreachable)
PostgreSQL pg_isready ❌ No response (port 5432 unreachable)
PostgreSQL SELECT 1 ❌ Skipped (port unreachable)

host.docker.internal resolves to 172.17.0.1 but both service ports timed out at the TCP level. Service containers appear not to be running or ports are blocked.

Overall: FAIL

🔌 Service connectivity validated by Smoke Services

@github-actions

Copy link
Copy Markdown
Contributor

@Copilot @lpcox

Smoke test results:

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

Running in direct BYOK mode (AWF_AUTH_TYPE=github-oidc + AWF_AUTH_AZURE_* + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) authenticated via Microsoft Entra

Overall: PASS

🪪 BYOK (AOAI Entra) report filed by Smoke Copilot BYOK AOAI (Entra)

@github-actions

Copy link
Copy Markdown
Contributor

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 97.85% 97.89% 📈 +0.04%
Statements 97.78% 97.82% 📈 +0.04%
Functions 99.50% 99.50% ➡️ +0.00%
Branches 93.55% 93.58% 📈 +0.03%
📁 Per-file Coverage Changes (1 files)
File Lines (Before → After) Statements (Before → After)
src/workdir-setup.ts 92.7% → 94.5% (+1.82%) 92.7% → 94.5% (+1.82%)

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

@github-actions

Copy link
Copy Markdown
Contributor

@lpcox @Copilot

Split docker-manager cleanup tests by concern ✅
Split api-proxy token parser tests by JSON, SSE, and normalization concerns ✅
GitHub.com connectivity ✅
File I/O ✅
BYOK inference ✅

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

Overall: PASS

🔑 BYOK (AOAI api-key) report filed by Smoke Copilot BYOK AOAI (api-key)

@lpcox

lpcox commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

@copilot resolve the merge conflicts in this pull request

…e-writeconfigs-tests

# Conflicts:
#	src/docker-manager-cleanup.test.ts

Copilot AI commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts in this pull request

Resolved the merge conflicts and completed the merge with origin/main in commit bcb795c.

auto-merge was automatically disabled June 21, 2026 18:10

Head branch was pushed to by a user without write access

Copilot finished work on behalf of lpcox June 21, 2026 18:10
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.

3 participants