Refactor generatePolicyManifest into composable policy section builders#5446
Conversation
generatePolicyManifest into composable policy section builders
There was a problem hiding this comment.
Pull request overview
Refactors Squid policy manifest generation by decomposing the previously large generatePolicyManifest() implementation into composable, ordered “section builder” functions, while keeping the manifest schema and rule semantics/order stable in this security-sensitive path.
Changes:
- Added
src/squid/policy-rules/section-builders.tswith focused helpers that append policy rules while maintaining a shared monotonicorder. - Simplified
src/squid/policy-manifest.tsinto an orchestrator that parses domain config once and invokes builders in canonical evaluation order. - Added a regression test to assert contiguous global rule ordering and boundary rules (first/last) across mixed sections.
Show a summary per file
| File | Description |
|---|---|
| src/squid/policy-rules/section-builders.ts | Introduces section-builder functions and shared PolicyRuleState/pushRule() to append rules with consistent ordering. |
| src/squid/policy-manifest.ts | Replaces inline rule construction with ordered calls to the new section builders; keeps output shape unchanged. |
| src/squid/policy-manifest.test.ts | Adds an assertion that rule order values are sequential across all emitted sections and that boundary rule IDs are preserved. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
|
✅ Copilot review passed with no inline comments. @copilot Add the |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
✅ Build Test Suite completed successfully! |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🚀 Security Guard has started processing this pull request |
|
✅ Smoke Gemini completed. All facets verified. 💎 Smoke test complete. PASS. |
|
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. 🌟 |
|
✅ Contribution Check completed successfully! Contribution guideline review complete: PR #5446 follows the applicable CONTRIBUTING.md requirements; no comment needed. |
|
🔌 Smoke Services — All services reachable! ✅ |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 |
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓 |
|
✅ Smoke Claude passed |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
🤖 Smoke Test ResultsPR: Refactor
Overall: PASS
|
🔬 Smoke Test: Copilot PAT Auth — PASS
Overall: PASS · Auth mode: PAT (COPILOT_GITHUB_TOKEN)
|
Smoke Test: Claude Engine Validation
Overall result: PASS
|
|
Smoke test:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
Chroot Version Comparison Results
Overall: ❌ Not all versions match. Go matches, but Python and Node.js differ between host and chroot environments.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
🔍 Smoke Test: API Proxy OpenTelemetry Tracing
All scenarios pass. OTEL tracing integration is fully functional.
|
Smoke Test Results: Copilot BYOK (Direct Mode)
Mode: Direct BYOK (COPILOT_PROVIDER_API_KEY) → api-proxy sidecar → api.githubcopilot.com Status: PASS Cc:
|
Smoke Test: GitHub Actions Services Connectivity
Overall: FAIL
|
|
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
|
Overall status: PASS 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.
|
|
PR titles tested:
|
generatePolicyManifest()insrc/squid/policy-manifest.tshad grown into a single 200+ line function that combined eight independent policy concerns in one control flow. This change decomposes policy assembly into focused section builders while preserving rule semantics and ordering in this security-sensitive path.What changed
src/squid/policy-rules/section-builders.ts.src/squid/policy-manifest.tsas a thin orchestrator that:rules,order),Section builders introduced
addPortSafetyRulesaddApiProxyAllowRulesaddRawIpBlockRulesaddDlpRulesaddBlockedDomainRulesaddProtocolAllowRulesaddBothProtocolAllowRulesaddDefaultDenyRuleBehavioral guarantees retained
generatePolicyManifestsignature/output unchanged).ordercounter.Test updates
src/squid/policy-manifest.test.tsto verify global sequential ordering across mixed sections and retained boundary rules (deny-unsafe-portsfirst,deny-defaultlast).