Refactor preflight allowed-domain resolution into explicit pipeline stages#5757
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the preflight allowed-domain resolution logic by extracting the parsing/merge stage and the domain-validation stage into dedicated helpers, while keeping resolveAllowedDomains() as the orchestrator that composes the full pipeline (including localhost handling and API-target auto-allowlisting). This improves readability and reviewability of a security-sensitive path without changing the externally visible behavior.
Changes:
- Extracted
parseDomainOptions(options)to encapsulate--allow-domains,--allow-domains-file, and--ruleset-fileparsing/merge (including existingprocess.exit(1)error behavior). - Extracted
validateAllowedDomains(domains)to centralize per-domain validation and failure handling. - Added focused unit tests covering the new helpers, while retaining existing
resolveAllowedDomainscoverage.
Show a summary per file
| File | Description |
|---|---|
| src/commands/preflight.ts | Split allowed-domain parsing/merge and validation into parseDomainOptions / validateAllowedDomains, and updated resolveAllowedDomains to compose these stages. |
| src/commands/preflight.test.ts | Added direct unit coverage for parseDomainOptions and validateAllowedDomains and updated imports accordingly. |
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: 0
- Review effort level: Low
|
✅ Copilot review passed with no inline comments. @copilot Add the |
|
✅ Smoke Gemini completed. All facets verified. 💎 |
|
🔌 Smoke Services — All services reachable! ✅ |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
✅ Build Test Suite completed successfully! |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 |
|
🚀 Security Guard has started processing this pull request |
|
✅ Smoke Claude passed |
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓 |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
✅ Contribution Check completed successfully! Contribution check complete for PR #5757: the refactor appears to follow the applicable CONTRIBUTING.md guidelines. It includes focused tests, keeps changes in the appropriate src/ command files, has a clear PR description, and does not require documentation updates because behavior is unchanged. |
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
Smoke Test: Copilot BYOK (Direct) Mode ✅ PASS
Mode: Direct BYOK (COPILOT_PROVIDER_API_KEY) via api-proxy → api.githubcopilot.com
|
🔥 Smoke Test ResultsPR: Refactor preflight allowed-domain resolution into explicit pipeline stages
Overall: FAIL — pre-step template variables were not substituted; connectivity tests could not be verified.
|
Smoke Test: Claude Engine Validation
Overall result: PASS
|
Smoke Test — Auth mode: PAT (COPILOT_GITHUB_TOKEN)
Overall: FAIL PR: Refactor preflight allowed-domain resolution into explicit pipeline stages —
|
Smoke Test: API Proxy OpenTelemetry Tracing
All scenarios pass. OTEL integration is complete and functional.
|
|
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
|
|
Refactor preflight allowed-domain resolution into explicit pipeline stages ✅ 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) Overall: PASS cc
|
Chroot Version Comparison
Result: 1/3 passed — Python and Node.js versions differ between host and chroot. The
|
|
Merged PRs reviewed:
Checks:
Overall: PASS
|
Smoke Test Results: GitHub Actions Services Connectivity
Overall: FAIL —
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Smoke Test Results: Gemini Engine
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.
|
resolveAllowedDomainsinsrc/commands/preflight.tshad grown into a long security-critical orchestrator that mixed parsing, merge logic, localhost side-effects, API target resolution, and validation. This refactor extracts the inline parsing and validation stages into dedicated helpers, keeping behavior unchanged while making the allowlist path easier to reason about and review.Pipeline stage extraction
parseDomainOptions(options)to encapsulate:--allow-domainsparsing--allow-domains-fileparsing--ruleset-filemergevalidateAllowedDomains(domains)to encapsulate domain/pattern validation and centralized error handling.Orchestrator simplification
resolveAllowedDomainsnow composes stages:parseDomainOptionsprocessLocalhostKeywordflow (including option mutations)validateAllowedDomainsFocused unit coverage
parseDomainOptionsandvalidateAllowedDomainsinsrc/commands/preflight.test.ts.resolveAllowedDomainscoverage to ensure integration behavior remains aligned.