fix: add diagnostic EACCES guard to workdir setup by lpcox · Pull Request #5984 · github/gh-aw-firewall

lpcox · 2026-07-07T12:53:52Z

Problem

When AWF's mkdirSync fails with EACCES (stale root-owned directories from a previous run), the raw Node.js error is opaque:

EACCES: permission denied, mkdir '/tmp/gh-aw/sandbox/firewall/logs'

This gives no indication of why it failed or how to fix it.

Solution

Wraps directory creation with a diagnostic EACCES catch that reports:

EACCES: cannot create work directory: /tmp/gh-aw/sandbox/firewall/logs
Path diagnosis:
  /tmp/gh-aw/sandbox/firewall: uid=0 gid=0 mode=755 writable=false
  └─ BLOCKED HERE: current process (uid=1001) cannot write to this directory
This typically happens on persistent runners when a previous AWF run left
directories owned by root. The calling process (e.g., gh-aw setup) must
remove or chown the stale directory before invoking AWF.
  Suggested fix: sudo rm -rf /tmp/gh-aw/sandbox/firewall && mkdir -p /tmp/gh-aw/sandbox/firewall/logs

Applied to both:

validateAndPrepareWorkDir() in config-writer.ts (workDir)
ensureDirectory() in workdir-setup.ts (all log/state directories)

Design decision

AWF intentionally does not attempt self-repair (no sudo rm, no Docker container workaround). In --network-isolation mode, AWF should not need elevated privileges. The actual fix belongs in the orchestrator (gh-aw setup action) which owns the paths and has the context to reclaim them. A companion PR will be opened in gh-aw.

Testing

All 3505 existing tests pass
Type-checks cleanly (tsc --noEmit)

When mkdirSync fails with EACCES (typically caused by stale root-owned directories from a previous AWF run), the error message now includes: - The blocking ancestor directory path - Its uid/gid/mode for immediate diagnosis - The current process UID - A clear explanation that the orchestrator must clean up stale directories - A suggested fix command (sudo rm -rf) This makes the failure actionable without requiring log archaeology. The guard is applied to both validateAndPrepareWorkDir (config-writer.ts) and ensureDirectory (workdir-setup.ts) which covers all directory creation paths. The actual fix for this class of failures belongs in the calling process (e.g., gh-aw setup action) which owns the paths and has the context to reclaim them. AWF intentionally does not attempt self-repair here because it should not require sudo in --network-isolation mode. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-07-07T12:56:10Z

⚠️ Coverage Regression Detected

This PR decreases test coverage. Please add tests to maintain coverage levels.

Overall Coverage

Metric	Base	PR	Delta
Lines	99.00%	98.41%	📉 -0.59%
Statements	98.95%	98.34%	📉 -0.61%
Functions	99.72%	99.44%	📉 -0.28%
Branches	95.44%	94.85%	📉 -0.59%

📁 Per-file Coverage Changes (2 files)

File	Lines (Before → After)	Statements (Before → After)
`src/config-writer.ts`	96.8% → 78.8% (-17.96%)	96.8% → 78.8% (-17.96%)
`src/workdir-setup.ts`	97.8% → 90.6% (-7.21%)	97.8% → 89.5% (-8.35%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

Copilot

Pull request overview

This PR improves AWF’s developer/operator experience by adding targeted diagnostics when workdir/log/state directory creation fails with EACCES, helping users identify the blocking ancestor directory and the likely “stale root-owned directory on persistent runner” root cause.

Changes:

Added EACCES-specific error handling to directory creation in ensureDirectory() to surface a clearer, actionable failure message.
Added diagnoseEacces() path-walk diagnostics and wrapped validateAndPrepareWorkDir() to emit richer EACCES context during workdir hardening.
Kept the design choice to avoid self-repair and instead instruct the orchestrator to clean up stale directories.

Show a summary per file

File	Description
src/workdir-setup.ts	Wraps recursive mkdir with an `EACCES` diagnostic path-walk to identify the likely blocking ancestor directory during log/state directory setup.
src/config-writer.ts	Adds `diagnoseEacces()` and wraps workDir creation/hardening to produce a more actionable `EACCES` error message.

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: 5
Review effort level: Low

+        `This typically happens on persistent runners when a previous AWF run ` +
+        `left directories owned by root. The calling process (e.g., gh-aw setup) ` +
+        `must remove or chown the stale directory before invoking AWF.\n` +
+        `  Suggested fix: sudo rm -rf ${path.dirname(config.workDir)} && mkdir -p ${config.workDir}`
+      );


+function isWritable(dirPath: string): boolean {
+  try {
+    fs.accessSync(dirPath, fs.constants.W_OK);
+    return true;


+      while (current !== path.dirname(current)) {
+        if (fs.existsSync(current)) {
+          try { fs.accessSync(current, fs.constants.W_OK); } catch { blocker = current; }
+          break;
+        }
+        current = path.dirname(current);
+      }


+  } catch (error: unknown) {
+    if (error && typeof error === 'object' && 'code' in error && error.code === 'EACCES') {
+      const uid = process.getuid?.() ?? '?';
+      // Identify the blocking ancestor for actionable diagnostics
+      let blocker = dirPath;
+      let current = path.resolve(dirPath);
+      while (current !== path.dirname(current)) {
+        if (fs.existsSync(current)) {
+          try { fs.accessSync(current, fs.constants.W_OK); } catch { blocker = current; }
+          break;
+        }
+        current = path.dirname(current);
+      }
+      throw new Error(
+        `EACCES: cannot create directory ${dirPath} (running as uid=${uid}).\n` +
+        `  Blocked by: ${blocker}\n` +
+        `  This is typically caused by a previous AWF run leaving root-owned directories.\n` +
+        `  The orchestrator must clean up stale directories before invoking AWF.`
+      );


+  try {
+    const workDirCreated = Boolean(
+      fs.mkdirSync(config.workDir, { recursive: true, mode: 0o700 })
+    );
+    const workDirLstat = fs.lstatSync(config.workDir);
+    if (workDirLstat.isSymbolicLink()) {
+      throw new Error(`Refusing to use symlink as directory: ${config.workDir}`);
+    }
+    const workDirStat = fs.statSync(config.workDir);
+    if (!workDirStat.isDirectory()) {
+      throw new Error(`Expected directory but found non-directory path: ${config.workDir}`);
+    }
+    if (!workDirCreated) {
+      fs.chmodSync(config.workDir, 0o700);
+    }
+  } catch (error: unknown) {
+    if (error && typeof error === 'object' && 'code' in error && error.code === 'EACCES') {
+      const diagnostic = diagnoseEacces(config.workDir);
+      throw new Error(
+        `EACCES: cannot create work directory: ${config.workDir}\n` +
+        `${diagnostic}\n` +
+        `This typically happens on persistent runners when a previous AWF run ` +
+        `left directories owned by root. The calling process (e.g., gh-aw setup) ` +
+        `must remove or chown the stale directory before invoking AWF.\n` +
+        `  Suggested fix: sudo rm -rf ${path.dirname(config.workDir)} && mkdir -p ${config.workDir}`
+      );
+    }
+    throw error;


lpcox · 2026-07-07T13:09:57Z

@copilot address review feedback

github-actions · 2026-07-07T13:14:20Z

✅ Copilot review passed with no inline comments.

@lpcox Add the ready-for-aw label to this PR to trigger agentic CI smoke tests.

- Fix isWritable() to check W_OK|X_OK instead of just W_OK, so the blocker is correctly identified when a directory lacks execute/search permission (config-writer.ts, workdir-setup.ts) - Fix suggested remediation in validateAndPrepareWorkDir: use the identified blocker path instead of path.dirname(workDir), preventing an overly broad sudo rm -rf (e.g. /tmp) when workDir is shallow - Fix ensureDirectory blocker detection: default blocker to the nearest existing ancestor rather than dirPath (which may not exist) - Add fsMockFactory entries for mkdirSync/accessSync/statSync so tests can mock them without hitting 'Cannot redefine property' - Add unit tests for EACCES diagnostic in ensureDirectory (workdir-setup.test.ts) - Add unit tests for EACCES diagnostic in validateAndPrepareWorkDir (config-writer-branches.test.ts)"

Copilot · 2026-07-07T13:23:13Z

@copilot address review feedback

Addressed all review feedback in commit 6e2c73d:

isWritable() — W_OK | X_OK (raised in both file threads): accessSync now checks fs.constants.W_OK | fs.constants.X_OK in both config-writer.ts and workdir-setup.ts, so a directory lacking execute/search permission is correctly identified as blocking.

Suggested fix path (config-writer.ts:114-118): diagnoseEacces() now returns { diagnosis, blockerPath }. The suggested fix uses blockerPath (the specific non-writable ancestor, e.g. /tmp/gh-aw/sandbox/firewall) when identified, falling back to workDir itself when none is found — no longer path.dirname(workDir) which could expand to /tmp.

blocker default (workdir-setup.ts:29-35): blocker now starts as null and is set to the nearest existing ancestor as soon as existsSync returns true — even if that ancestor appears writable. This gives a more actionable message than a non-existent dirPath.

Unit tests: Added 5 tests in workdir-setup.test.ts covering ensureDirectory's EACCES branch (blocker named, writable-ancestor case, null-blocker fallback, W_OK|X_OK probe, non-EACCES rethrow); and 3 tests in config-writer-branches.test.ts covering validateAndPrepareWorkDir's EACCES branch (blocker in suggested fix, null-blocker fallback, W_OK|X_OK probe). Added mkdirSync, accessSync, statSync to fsMockFactory so the mocks are jest-configurable without hitting Cannot redefine property.

github-actions · 2026-07-07T13:49:41Z

⚠️ Coverage Regression Detected

This PR decreases test coverage. Please add tests to maintain coverage levels.

Overall Coverage

Metric	Base	PR	Delta
Lines	99.00%	98.92%	📉 -0.08%
Statements	98.95%	98.87%	📉 -0.08%
Functions	99.72%	99.44%	📉 -0.28%
Branches	95.44%	95.35%	📉 -0.09%

📁 Per-file Coverage Changes (4 files)

File	Lines (Before → After)	Statements (Before → After)
`src/test-helpers/fs-mock-factory.test-utils.ts`	100.0% → 92.8% (-7.15%)	100.0% → 92.8% (-7.15%)
`src/test-helpers/config-writer-test-harness.test-utils.ts`	100.0% → 96.7% (-3.34%)	100.0% → 96.8% (-3.23%)
`src/config-writer.ts`	96.8% → 95.1% (-1.69%)	96.8% → 95.1% (-1.69%)
`src/workdir-setup.ts`	97.8% → 98.0% (+0.19%)	97.8% → 98.0% (+0.20%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

lpcox · 2026-07-07T13:55:24Z

@copilot fix this failing ci check https://github.com/github/gh-aw-firewall/actions/runs/28869556618/job/85634163143?pr=5984

Copilot · 2026-07-07T14:09:31Z

@copilot fix this failing ci check https://github.com/github/gh-aw-firewall/actions/runs/28869556618/job/85634163143?pr=5984

Fixed in commit 16fab42.

The failing test strips client-supplied auth headers before forwarding (and the adjacent forwards buffered head bytes test) used setTimeout(..., 30) to wait for async events. Under CI load, the two-deep setImmediate chain occasionally takes longer than 30ms, causing the expect assertion to throw before resolve() is called, leaving the Promise unresolved and triggering Jest's 5 s timeout.

Changed both tests to the same event-driven pattern used by the already-passing "injects Authorization header" test:

// before (flaky)
return new Promise(resolve => setTimeout(() => { /* assertions */ }, 30));

// after (deterministic)
return new Promise(resolve => {
  tlsSocket.once('secureConnect', () => setImmediate(() => {
    /* assertions */
    resolve();
  }));
});

setImmediate here ensures websocket-tunnel.js's own secureConnect listener (which calls tlsSocket.write) runs synchronously before the assertions check mock.calls.

Copilot AI review requested due to automatic review settings July 7, 2026 12:53

Copilot started reviewing on behalf of lpcox July 7, 2026 12:54 View session

Copilot AI reviewed Jul 7, 2026

View reviewed changes

lpcox mentioned this pull request Jul 7, 2026

fix: reclaim stale root-owned /tmp/gh-aw before AWF invocation github/gh-aw#44022

Merged

Copilot started work on behalf of lpcox July 7, 2026 13:10 View session

Copilot finished work on behalf of lpcox July 7, 2026 13:23

Copilot started work on behalf of lpcox July 7, 2026 13:56 View session

fix: use event-based waiting in server.websocket tests

16fab42

Copilot finished work on behalf of lpcox July 7, 2026 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: add diagnostic EACCES guard to workdir setup#5984

fix: add diagnostic EACCES guard to workdir setup#5984
lpcox wants to merge 3 commits into
mainfrom
fix/eacces-diagnostic-guard

lpcox commented Jul 7, 2026

Uh oh!

github-actions Bot commented Jul 7, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

lpcox commented Jul 7, 2026

Uh oh!

github-actions Bot commented Jul 7, 2026 •

edited

Loading

Uh oh!

Copilot AI commented Jul 7, 2026

Uh oh!

github-actions Bot commented Jul 7, 2026

Uh oh!

lpcox commented Jul 7, 2026

Uh oh!

Copilot AI commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

lpcox commented Jul 7, 2026

Problem

Solution

Design decision

Testing

Related

Uh oh!

github-actions Bot commented Jul 7, 2026

⚠️ Coverage Regression Detected

Overall Coverage

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

lpcox commented Jul 7, 2026

Uh oh!

github-actions Bot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI commented Jul 7, 2026

Uh oh!

github-actions Bot commented Jul 7, 2026

⚠️ Coverage Regression Detected

Overall Coverage

Uh oh!

lpcox commented Jul 7, 2026

Uh oh!

Copilot AI commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Jul 7, 2026 •

edited

Loading