Refactor optional sidecar assembly into focused helpers in optional-services#5833
Conversation
optional-services
There was a problem hiding this comment.
Pull request overview
Refactors assembleOptionalServices into smaller internal helpers to isolate sidecar/sysroot wiring concerns, while keeping the exported coordinator function as a thin orchestrator.
Changes:
- Extracted env pre-set logic, sysroot volume filtering, sysroot staging wiring, and per-sidecar assembly into focused helper functions.
- Added
testHelpersexports for unit testing of extracted internal logic. - Added new unit tests covering env pre-set behavior and sysroot volume filtering behavior.
Show a summary per file
| File | Description |
|---|---|
| src/services/optional-services.ts | Splits optional sidecar/sysroot assembly into internal helpers and keeps assembleOptionalServices as an orchestrator. |
| src/services/optional-services.test.ts | Adds focused unit tests for the extracted helper logic via testHelpers. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
✅ Copilot review passed with no inline comments. @copilot Add the |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓 |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
✅ Smoke Claude passed |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
✅ Contribution Check completed successfully! PR #5833 follows the applicable CONTRIBUTING.md guidelines based on the provided metadata, diff, and contribution guide; no comment needed. |
|
✅ Build Test Suite completed successfully! |
|
🔌 Smoke Services — All services reachable! ✅ |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
🚀 Security Guard has started processing this pull request |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Chroot tests failed Smoke Chroot failed - See logs for details. |
|
✅ Smoke Gemini completed. All facets verified. 💎 Smoke test completed with partial failures. Results reported to PR #5833. |
Smoke Test: Claude Engine Validation
Overall Result: PASS ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🔥 Smoke Test: Copilot PAT Auth
Overall: PASS | Auth mode: PAT (COPILOT_GITHUB_TOKEN)
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
${{ steps.smoke-data.outputs.SMOKE_PR_DATA }} ✅ GitHub MCP Testing Note: 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 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🔬 Smoke Test Results
Overall: PASS PR author: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test: GitHub Actions Services Connectivity
Overall: FAIL — Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test Results: Copilot BYOK ✅Tests:
Mode: Direct BYOK via api-proxy sidecar Status: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
GitHub MCP Testing: ✅ Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test: API Proxy OpenTelemetry Tracing
All 5 scenarios passed. OTEL tracing integration is working correctly. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test Results
Overall status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
assembleOptionalServiceshad grown into a 200+ line mixed-responsibility function spanning env pre-sets, sysroot staging, iptables init, and multiple sidecar wiring paths. This change keeps it as a thin coordinator and splits sidecar/sysroot assembly into focused internal helpers to improve reviewability and isolate security-relevant paths.What changed
presetSidecarIpEnvVars(...)(setsAWF_API_PROXY_IP,AWF_CLI_PROXY_IP,AWF_NETWORK_ISOLATIONbefore init-container construction).filterAgentVolumesForSysroot(...).assembleSysrootService(...).assembleIptablesInitService(...)assembleApiProxyService(...)assembleDohProxyService(...)assembleCliProxyService(...)finalizeSysrootVolumes(...).assembleOptionalServices(...)to orchestration only, preserving existing behavior and call surface.Targeted test coverage added
src/services/optional-services.test.tswith focused tests for:testHelpersfor precise unit coverage of extracted logic.Example (coordinator shape after split)