Conversation
~/.copilot in agent mount setup
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 84.31% | 84.39% | 📈 +0.08% |
| Statements | 83.55% | 83.63% | 📈 +0.08% |
| Functions | 87.46% | 87.46% | ➡️ +0.00% |
| Branches | 74.72% | 74.70% | 📉 -0.02% |
📁 Per-file Coverage Changes (1 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
86.8% → 87.1% (+0.29%) | 86.4% → 86.7% (+0.29%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Hardens AWF’s agent bind-mount setup to better support self-hosted runners whose $HOME differs from /home/runner and where ~/.copilot may be missing, avoiding startup failures due to missing host mount paths.
Changes:
- Adds a
~/.copilotexistence check + creation attempt before adding the host bind mount, warning and skipping that single mount on failure. - Adds a regression unit test for a non-standard
HOMEwith a missing.copilotdirectory. - Updates environment documentation to clarify
$HOME-derived (sudo-aware) home path handling for self-hosted runners.
Show a summary per file
| File | Description |
|---|---|
src/docker-manager.ts |
Creates ~/.copilot if missing before adding the host bind mount; warns and skips the mount if creation fails. |
src/docker-manager.test.ts |
Adds a test ensuring .copilot is created and mounted when HOME is non-standard and initially missing .copilot. |
docs/environment.md |
Documents that AWF derives the effective home from $HOME (sudo-aware), supporting non-standard self-hosted runner homes. |
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: 3
| if (!fs.existsSync(copilotHomeDir)) { | ||
| try { | ||
| fs.mkdirSync(copilotHomeDir, { recursive: true }); | ||
| logger.debug(`Created missing Copilot directory for bind mount: ${copilotHomeDir}`); |
There was a problem hiding this comment.
generateDockerCompose() is creating the host ~/.copilot directory, but it doesn’t set ownership/permissions like writeConfigs() does for chroot home subdirectories (it chowns to the host UID/GID). If this runs under sudo and ~/.copilot is created as root:root, the agent user may not be able to write Copilot CLI state. Consider either (a) chowning the created directory to getSafeHostUid()/getSafeHostGid() here, or (b) moving all host-home directory creation into writeConfigs() and keeping generateDockerCompose() side-effect-free for host paths.
| logger.debug(`Created missing Copilot directory for bind mount: ${copilotHomeDir}`); | |
| fs.chownSync(copilotHomeDir, getSafeHostUid(), getSafeHostGid()); | |
| logger.debug(`Created missing Copilot directory for bind mount and set ownership: ${copilotHomeDir}`); |
| it('should create missing .copilot directory and mount it when using non-standard HOME path', () => { | ||
| const fakeHome = fs.mkdtempSync(path.join(os.tmpdir(), 'awf-home-')); | ||
| const originalHome = process.env.HOME; | ||
| const originalSudoUser = process.env.SUDO_USER; | ||
| delete process.env.SUDO_USER; | ||
| process.env.HOME = fakeHome; | ||
|
|
||
| try { | ||
| const copilotDir = path.join(fakeHome, '.copilot'); | ||
| expect(fs.existsSync(copilotDir)).toBe(false); | ||
|
|
||
| const result = generateDockerCompose(mockConfig, mockNetworkConfig); | ||
| const volumes = result.services.agent.volumes as string[]; | ||
|
|
||
| expect(fs.existsSync(copilotDir)).toBe(true); | ||
| expect(volumes).toContain(`${fakeHome}/.copilot:/host${fakeHome}/.copilot:rw`); | ||
| } finally { |
There was a problem hiding this comment.
The new test covers the happy path where .copilot can be created, but it doesn’t cover the behavior added in the catch branch (mkdir failure → warn + skip mount). Consider adding a test that mocks fs.mkdirSync (or uses a read-only temp dir) to assert the warning is emitted and the .copilot bind mount is omitted, since that’s the main behavioral change for hardening startup.
|
@copilot update the PR with this review feedback #2114 (review) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
🔥 Smoke Test Results — PASS
PR: Handle non-standard runner HOME and missing Overall: PASS
|
|
Smoke Test Results (run #24673612618) ✅ GitHub MCP: Last 2 merged PRs retrieved Status: PASS
|
🔥 Smoke Test: Copilot BYOK (Offline) Mode
Running in BYOK offline mode ( Overall: PASS — PR: "Handle non-standard runner HOME and missing
|
Smoke Test: GitHub Actions Services Connectivity ✅All checks passed:
|
Chroot Runtime Version Comparison
Overall: FAILED — Python and Node.js versions differ between host and chroot environments.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
|
@copilot rebase to main and recompile |
1 similar comment
|
@copilot rebase to main and recompile |
AWF already derives home paths from runtime environment, but agent volume setup still assumed
~/.copilotwas always present. On self-hosted runners with non-/home/runnerhomes (or fresh homes), this could cause bind-mount failures during startup.Runtime mount hardening (
src/docker-manager.ts)~/.copilotbind mount, AWF now checks whether the directory exists..copilotbind mount instead of failing mount composition.Targeted regression coverage (
src/docker-manager.test.ts)HOMEwhere.copilotdoes not exist.Environment docs update (
docs/environment.md)$HOME(sudo-aware), not hardcoding/home/runner.