fix: tolerate EROFS when chmod on pre-existing mcp-logs dir fails#5960
Conversation
The prepareLogDirectories function only caught EPERM when chmod failed on a pre-existing /tmp/gh-aw/mcp-logs directory, but EROFS (read-only file system) is equally benign — it just means we cannot change permissions on a directory managed by another process. Also fix test mock leakage: jest.clearAllMocks() does not reset mock implementations, so the chmodSync mock override from one test leaked into subsequent tests. Explicitly restore the passthrough implementation in beforeEach. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
This PR hardens prepareLogDirectories to avoid crashing on systems where the pre-existing MCP logs directory (/tmp/gh-aw/mcp-logs) cannot be chmod’d due to a read-only filesystem, and it stabilizes the associated Jest tests by preventing mock implementation leakage between test cases.
Changes:
- Tolerate
EROFS(in addition toEPERM) when best-effortchmodSyncfails for an already-existing/tmp/gh-aw/mcp-logs. - Improve debug logging to report the specific skipped error code.
- Fix Jest test isolation by restoring the default passthrough
chmodSyncimplementation inbeforeEach, and add coverage for theEROFSbehavior while preserving “rethrow on other errors”.
Show a summary per file
| File | Description |
|---|---|
| src/workdir-setup.ts | Extends the existing “best-effort chmod” error tolerance to include EROFS and logs the skipped errno code. |
| src/workdir-setup.test.ts | Prevents chmodSync mock implementation leakage across tests and adds explicit tests for EROFS tolerance and non-EPERM/EROFS rethrow behavior. |
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
|
🚀 Security Guard has started processing this pull request |
|
✅ Build Test Suite completed successfully! |
|
Chroot tests failed Smoke Chroot failed - See logs for details. |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
✅ Contribution Check completed successfully! PR #5960 follows the applicable CONTRIBUTING.md guidelines; no review comment needed. |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
✅ Smoke Gemini completed. All facets verified. 💎 |
|
❌ Smoke Copilot BYOK AOAI (api-key) reports failed. AOAI BYOK (api-key) mode investigation needed... |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🔌 Smoke Services — All services reachable! ✅ |
|
❌ Smoke Copilot BYOK AOAI (Entra) reports failed. AOAI BYOK (Entra) mode investigation needed... |
|
✅ Smoke Claude passed |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
Smoke Test: Services Connectivity
Result: 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: Copilot BYOK (Direct) Mode — PASS
Running in direct BYOK mode ( @lpcox — Overall: 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: API Proxy OpenTelemetry Tracing
All 5 scenarios 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 Results
Overall Status: FAIL PR Titles found:
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 PAT Auth — PASS ✅
Auth mode: PAT (COPILOT_GITHUB_TOKEN) | Author: @lpcox | 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.
|
🔬 Smoke Test Results
Overall: PASS cc @lpcox 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.
|
🏗️ 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.
|
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.
|
|
fix: handle EPERM when chmod'ing pre-existing mcp-logs directory ✅ 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.
|
Problem
prepareLogDirectoriesonly caughtEPERMwhenchmodSyncfailed on a pre-existing/tmp/gh-aw/mcp-logsdirectory. On systems where that path lives on a read-only filesystem (e.g., Docker volume mounts),EROFSwas thrown and crashed the entire setup.This also caused 12 test failures in
workdir-setup.test.tsbecause:chmodSyncmock delegates to the real filesystem, which throws EROFSjest.clearAllMocks()doesn't reset mock implementations, so a mock override from one test leaked into subsequent testsFix
EROFSin addition toEPERMin the mcp-logs chmod catch blockchmodSyncpassthrough implementation inbeforeEachto prevent mock leakage, and add a dedicated test for EROFS tolerance