Refactor container-stop to keep Squid pre-shutdown chmod helper internal#6177
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reduces the public API surface of the container shutdown module by making fixSquidLogPermissionsBeforeShutdown an internal helper while keeping the shutdown behavior unchanged. Tests are adjusted to validate behavior and retain branch coverage via the public stopContainers(...) entrypoint instead of importing the helper directly.
Changes:
- Removed the
exportfromfixSquidLogPermissionsBeforeShutdownto keep it internal tosrc/container-stop.ts. - Deleted direct unit tests that imported the helper, aligning tests to the module’s public contract.
- Updated branch-coverage tests to exercise the helper’s non-zero-exit logging branches indirectly through
stopContainers(...).
Show a summary per file
| File | Description |
|---|---|
| src/container-stop.ts | Makes the pre-shutdown Squid chmod helper internal while preserving its invocation from stopContainers(...). |
| src/container-stop.test.ts | Removes direct helper imports/tests; validates helper behavior via stopContainers(...). |
| src/container-stop-coverage.test.ts | Preserves helper branch coverage by driving execution through stopContainers(...) with mocked execa results. |
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: 0
- Review effort level: Low
|
🔌 Smoke Services — All services reachable! ✅ |
|
🚀 Security Guard has started processing this pull request |
|
✅ Contribution Check completed successfully! PR #6177 follows the applicable CONTRIBUTING.md guidelines: the internal refactor preserves TypeScript style, updates tests for the changed API boundary, requires no documentation change, has a clear description, and uses appropriate src/ file organization. |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 |
|
✅ Smoke Claude passed |
|
❌ Smoke Copilot BYOK AOAI (api-key) reports failed. AOAI BYOK (api-key) mode investigation needed... |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
✅ Build Test Suite completed successfully! |
|
✅ Smoke Gemini completed. All facets verified. 💎 Smoke test completed. Results: Connectivity PASS, File Writing PASS, GitHub MCP FAIL (tools missing/unauthenticated). |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
📰 VERDICT: Smoke Docker Sbx has concluded. All systems operational. This is a developing story. 🎤 |
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
Smoke Test: Copilot PAT Auth
Status: INCONCLUSIVE — workflow template variables ( 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.
|
Smoke Test: 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
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: 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.
|
|
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.
|
Chroot Version Comparison
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.
|
Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"
- "registry.npmjs.org"See Network Configuration for more information.
|
Smoke Test: API Proxy OpenTelemetry Tracing
All 5 scenarios pass. OTEL tracing integration is fully functional. 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: PR: Refactor container-stop to keep Squid pre-shutdown chmod helper internal
|
Gemini Engine Smoke Test Results
Recent PRs found:
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.
|
|
Smoke Test: Copilot BYOK (Direct) Mode ✅ Test 1: MCP connectivity 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.
|
fixSquidLogPermissionsBeforeShutdowninsrc/container-stop.tswas exported despite having no external consumers. This PR reduces API surface by making it internal while preserving existing shutdown behavior and branch coverage through public entrypoints.API surface cleanup
exportfromfixSquidLogPermissionsBeforeShutdowninsrc/container-stop.ts.stopContainers(...), so runtime behavior is unaffected.Test alignment to public contract
src/container-stop.test.ts.src/container-stop-coverage.test.tsto exercise the same helper branches indirectly viastopContainers(...), preserving coverage for non-zerodocker execexit paths.Illustrative change