fix: pre-flight reclaim of stale root-owned directories before writeConfigs#5983
Closed
lpcox wants to merge 1 commit into
Closed
fix: pre-flight reclaim of stale root-owned directories before writeConfigs#5983lpcox wants to merge 1 commit into
lpcox wants to merge 1 commit into
Conversation
…onfigs On persistent GitHub Actions runners, a previous AWF run (or Docker daemon) can leave /tmp/gh-aw/sandbox/firewall/ owned by root. When the next AWF invocation runs as non-root (rootless/network-isolation mode), mkdirSync fails with EACCES before any container is started: EACCES: permission denied, mkdir '/tmp/gh-aw/sandbox/firewall/logs' This adds a pre-flight reclaim step that detects non-writable ancestor directories and removes them (via sudo rm -rf, then fs.rmSync fallback) before mkdirSync is called. The reclaim is applied to: - workDir (validateAndPrepareWorkDir in config-writer.ts) - All log directories (prepareLogDirectories in workdir-setup.ts) - /tmp/gh-aw/mcp-logs (hardcoded MCP gateway path) Safety guards: - Only acts when running as non-root (root never gets EACCES) - Only removes the narrowest non-writable ancestor - Refuses to operate on protected system paths (/, /tmp, /home/runner, etc.) - Logs all actions for auditability - Falls back gracefully if neither sudo nor rmSync succeeds This is the AWF-side complement to gh-aw's compiled workflow chmod steps (#30414, #27742) which fix artifact upload permissions post-run. This fix addresses the pre-run failure path that those PRs cannot cover. Fixes: github/gh-aw#42400 (supersedes the closed, unmerged approach) Ref: https://github.com/github/gh-aw/actions/runs/28856007222/job/85583002595 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.82% | 98.70% | 📉 -0.12% |
| Statements | 98.76% | 98.63% | 📉 -0.13% |
| Functions | 99.72% | 99.72% | ➡️ +0.00% |
| Branches | 95.19% | 95.05% | 📉 -0.14% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/workdir-setup.ts |
97.8% → 97.9% (+0.06%) | 97.8% → 97.9% (+0.06%) |
src/config-writer.ts |
91.4% → 91.6% (+0.18%) | 91.4% → 91.6% (+0.18%) |
✨ New Files (1 files)
src/preflight-reclaim.ts: 85.1% lines
Coverage comparison generated by scripts/ci/compare-coverage.ts
Collaborator
Author
|
Closing: the pre-flight reclaim belongs in gh-aw (which owns the paths and has the setup phase), not in AWF. AWF will get a defensive guard with clear error messages instead. The actual fix will be a PR in gh-aw. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a pre-flight “stale directory reclaim” mechanism intended to prevent EACCES failures on persistent GitHub Actions runners when previously root-owned paths under /tmp/gh-aw/... block subsequent non-root AWF runs, before writeConfigs() and log directory setup occur.
Changes:
- Introduces
reclaimStaleDirectory()to detect and remove the narrowest non-writable ancestor directory viasudo rm -rfwith afs.rmSyncfallback. - Invokes the pre-flight reclaim step before creating the workDir and before creating the various log/session directories (including
/tmp/gh-aw/mcp-logs). - Adds unit tests covering protected-path handling and reclaim flows.
Show a summary per file
| File | Description |
|---|---|
| src/workdir-setup.ts | Runs pre-flight reclaim on log/session/MCP log directories before ensuring they exist. |
| src/preflight-reclaim.ts | New reclaim implementation (ancestor detection, protected-path checks, sudo/fs removal attempts). |
| src/preflight-reclaim.test.ts | New unit tests for reclaim behavior and guardrails. |
| src/config-writer.ts | Runs pre-flight reclaim on workDir before creating it with 0o700. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Low
Comment on lines
+96
to
+108
| // Collect path segments from target down to the first existing directory | ||
| const nonExistentSegments: string[] = []; | ||
| while (!fs.existsSync(current)) { | ||
| nonExistentSegments.unshift(path.basename(current)); | ||
| const parent = path.dirname(current); | ||
| if (parent === current) break; // reached root | ||
| current = parent; | ||
| } | ||
|
|
||
| // `current` is now the first existing ancestor of targetDir | ||
| if (!fs.existsSync(current)) { | ||
| return undefined; | ||
| } |
Comment on lines
+14
to
+18
| * This function detects non-writable ancestors in the workDir path and attempts | ||
| * to reclaim them via: | ||
| * 1. `sudo rm -rf` (available on GitHub-hosted runners without a password) | ||
| * 2. Plain `rm -rf` fallback (may work if only leaf permissions are wrong) | ||
| * |
Comment on lines
+127
to
+151
| const protectedPaths = new Set([ | ||
| '/', | ||
| '/tmp', | ||
| '/var', | ||
| '/var/tmp', | ||
| '/home', | ||
| '/root', | ||
| '/usr', | ||
| '/etc', | ||
| '/opt', | ||
| '/bin', | ||
| '/sbin', | ||
| '/lib', | ||
| '/lib64', | ||
| '/dev', | ||
| '/sys', | ||
| '/proc', | ||
| '/run', | ||
| // GitHub Actions runner paths (protect the runner itself) | ||
| '/home/runner', | ||
| '/home/runner/work', | ||
| ]); | ||
|
|
||
| return protectedPaths.has(resolved); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On persistent GitHub Actions runners, a previous AWF run (or Docker daemon) can leave
/tmp/gh-aw/sandbox/firewall/owned by root. When the next AWF invocation runs as non-root (rootless/network-isolation mode),mkdirSyncfails with EACCES before any container is started:Root cause: With
--network-isolationnow the default, AWF runs without sudo. But Docker containers from previous runs (or the Docker daemon itself) leave directories owned by root. The next non-root AWF invocation cannot create subdirectories under these root-owned paths.CI failure: https://github.com/github/gh-aw/actions/runs/28856007222/job/85583002595
Solution
Adds a pre-flight reclaim step that detects non-writable ancestor directories and removes them before
mkdirSyncis called. Applied to:validateAndPrepareWorkDir()inconfig-writer.tsprepareLogDirectories()inworkdir-setup.ts/tmp/gh-aw/mcp-logs— hardcoded MCP gateway pathReclaim strategy
sudo rm -rf(works passwordless on GitHub-hosted runners)fs.rmSync(handles cases where only leaf permissions are wrong)Safety guards
/,/tmp,/home/runner, etc.)Relationship to existing fixes
chmod -R a+rXfor artifact uploadThis is the AWF-side fix. The previous approach (github/gh-aw#42400) added a bash pre-flight in gh-aw's setup scripts, but was closed without merge. Fixing it in AWF itself is more robust — it covers all invocations regardless of how AWF is called.
Testing
preflight-reclaim.tscovering:tsc --noEmit)