fix: prevent .cargo volume/tmpfs mount conflict in chroot mode#740
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
|
📰 DEVELOPING STORY: Smoke Copilot reports failed. Our correspondents are investigating the incident... |
|
Chroot tests failed Smoke Chroot failed - See logs for details. |
* Initial plan * fix: handle missing docker-compose.yml during cleanup gracefully Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
edbfdf3
into
claude/fix-github-actions-workflow-another-one
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
|
📰 DEVELOPING STORY: Smoke Copilot reports failed. Our correspondents are investigating the incident... |
|
Chroot tests failed Smoke Chroot failed - See logs for details. |
* Initial plan * fix: replace /dev/null mounts with tmpfs for credential hiding This fixes Docker mount errors like "read-only file system" that occur when mounting /dev/null over credential files whose parent directories don't exist in the container's filesystem. The solution uses tmpfs mounts instead, which create empty in-memory filesystems that overlay directories without requiring the target path to exist first. Changes: - Normal mode: Hide credential directories (~/.docker, ~/.ssh, ~/.aws, etc.) using tmpfs - Chroot mode: Hide credential directories at /host paths using tmpfs - Updated DockerService type to include tmpfs property - Updated tests to verify tmpfs behavior instead of /dev/null mounts - Fixed config mutation bug by using local variable instead of mutating config object Closes the GitHub Actions failure where cargo credentials mounting failed with: "error mounting "/dev/null" to rootfs at "/host/home/runner/.cargo/credentials": create mountpoint ... read-only file system" Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: prevent .cargo volume/tmpfs mount conflict in chroot mode (#740) * Initial plan * fix: prevent .cargo volume/tmpfs mount conflict in chroot mode Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: handle missing docker-compose.yml during cleanup gracefully (#741) * Initial plan * fix: handle missing docker-compose.yml during cleanup gracefully Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
* Initial plan * fix: only hide credential files if parent directory exists Prevents Docker mount errors when credential file parent directories don't exist on the host. This fixes the "read-only file system" error when Docker tries to create mountpoints for non-existent parents. The fix checks if the parent directory exists before adding /dev/null mounts for credential files in both normal and chroot modes. This maintains security while avoiding mount failures. Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: replace /dev/null mounts with tmpfs for credential hiding (#738) * Initial plan * fix: replace /dev/null mounts with tmpfs for credential hiding This fixes Docker mount errors like "read-only file system" that occur when mounting /dev/null over credential files whose parent directories don't exist in the container's filesystem. The solution uses tmpfs mounts instead, which create empty in-memory filesystems that overlay directories without requiring the target path to exist first. Changes: - Normal mode: Hide credential directories (~/.docker, ~/.ssh, ~/.aws, etc.) using tmpfs - Chroot mode: Hide credential directories at /host paths using tmpfs - Updated DockerService type to include tmpfs property - Updated tests to verify tmpfs behavior instead of /dev/null mounts - Fixed config mutation bug by using local variable instead of mutating config object Closes the GitHub Actions failure where cargo credentials mounting failed with: "error mounting "/dev/null" to rootfs at "/host/home/runner/.cargo/credentials": create mountpoint ... read-only file system" Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: prevent .cargo volume/tmpfs mount conflict in chroot mode (#740) * Initial plan * fix: prevent .cargo volume/tmpfs mount conflict in chroot mode Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: handle missing docker-compose.yml during cleanup gracefully (#741) * Initial plan * fix: handle missing docker-compose.yml during cleanup gracefully Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
* Initial plan * fix: create .copilot/logs mountpoint before docker mount The agent container mount at ~/.copilot:ro followed by ~/agent-logs:~/.copilot/logs:rw fails in GitHub Actions because Docker cannot create the logs subdirectory inside a read-only mount. Fix by creating ~/.copilot/logs on the host before mounting, so Docker doesn't need to create the mountpoint inside the read-only parent. This affects GitHub Actions runners where ~/.copilot doesn't exist before AWF runs. Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: only hide credential files if parent directory exists (#737) * Initial plan * fix: only hide credential files if parent directory exists Prevents Docker mount errors when credential file parent directories don't exist on the host. This fixes the "read-only file system" error when Docker tries to create mountpoints for non-existent parents. The fix checks if the parent directory exists before adding /dev/null mounts for credential files in both normal and chroot modes. This maintains security while avoiding mount failures. Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: replace /dev/null mounts with tmpfs for credential hiding (#738) * Initial plan * fix: replace /dev/null mounts with tmpfs for credential hiding This fixes Docker mount errors like "read-only file system" that occur when mounting /dev/null over credential files whose parent directories don't exist in the container's filesystem. The solution uses tmpfs mounts instead, which create empty in-memory filesystems that overlay directories without requiring the target path to exist first. Changes: - Normal mode: Hide credential directories (~/.docker, ~/.ssh, ~/.aws, etc.) using tmpfs - Chroot mode: Hide credential directories at /host paths using tmpfs - Updated DockerService type to include tmpfs property - Updated tests to verify tmpfs behavior instead of /dev/null mounts - Fixed config mutation bug by using local variable instead of mutating config object Closes the GitHub Actions failure where cargo credentials mounting failed with: "error mounting "/dev/null" to rootfs at "/host/home/runner/.cargo/credentials": create mountpoint ... read-only file system" Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: prevent .cargo volume/tmpfs mount conflict in chroot mode (#740) * Initial plan * fix: prevent .cargo volume/tmpfs mount conflict in chroot mode Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: handle missing docker-compose.yml during cleanup gracefully (#741) * Initial plan * fix: handle missing docker-compose.yml during cleanup gracefully Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request fixes a Docker mount conflict where ~/.cargo was being mounted as both a volume and tmpfs in chroot mode with credential hiding enabled (the default configuration). The PR also includes unrelated changes to stopContainers() that add fallback logic for cleaning up containers when the docker-compose.yml file is missing.
Changes:
- Conditional .cargo volume mount: only mount when
allowFullFilesystemAccess=trueto prevent conflict with tmpfs credential hiding - Defensive stopContainers implementation: fall back to stopping containers by name if docker-compose.yml is missing
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/docker-manager.ts | Added config.allowFullFilesystemAccess check to .cargo volume mount (line 507) to prevent volume/tmpfs conflict; added fallback logic in stopContainers() to stop containers by name when compose file is missing (lines 1304-1333) |
| src/docker-manager.test.ts | Added two test cases verifying .cargo mount behavior with different allowFullFilesystemAccess settings (lines 658-694) |
Comments suppressed due to low confidence (2)
src/docker-manager.ts:1330
- The fallback path only removes containers but doesn't clean up associated volumes and networks. The normal path uses
docker compose down -vwhich removes volumes, but the fallback only usesdocker rm -fwhich leaves volumes and networks orphaned. Consider also cleaning up the network (awf-net) and volumes in the fallback path, or document this limitation.
// Stop and remove containers by name
const containerNames = ['awf-agent', 'awf-squid'];
for (const name of containerNames) {
try {
// Check if container exists
const { stdout } = await execa('docker', ['ps', '-aq', '-f', `name=^${name}$`]);
if (stdout.trim()) {
logger.debug(`Stopping container: ${name}`);
await execa('docker', ['rm', '-f', name], { stdio: 'inherit' });
}
} catch (err) {
logger.debug(`Could not stop container ${name}:`, err);
}
}
src/docker-manager.ts:1330
- The new fallback logic in stopContainers (lines 1314-1330) lacks test coverage. Consider adding tests for scenarios where docker-compose.yml is missing or workDir doesn't exist, to verify the fallback container stopping logic works correctly.
// Fallback: compose file missing, stop containers by name
logger.debug('Compose file not found, stopping containers by name');
// Stop and remove containers by name
const containerNames = ['awf-agent', 'awf-squid'];
for (const name of containerNames) {
try {
// Check if container exists
const { stdout } = await execa('docker', ['ps', '-aq', '-f', `name=^${name}$`]);
if (stdout.trim()) {
logger.debug(`Stopping container: ${name}`);
await execa('docker', ['rm', '-f', name], { stdio: 'inherit' });
}
} catch (err) {
logger.debug(`Could not stop container ${name}:`, err);
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if workDir and docker-compose.yml exist before using docker compose | ||
| const composeFile = path.join(workDir, 'docker-compose.yml'); | ||
| if (fs.existsSync(workDir) && fs.existsSync(composeFile)) { | ||
| // Normal path: use docker compose down | ||
| await execa('docker', ['compose', 'down', '-v'], { | ||
| cwd: workDir, | ||
| stdio: 'inherit', | ||
| }); | ||
| logger.success('Containers stopped successfully'); | ||
| } else { | ||
| // Fallback: compose file missing, stop containers by name | ||
| logger.debug('Compose file not found, stopping containers by name'); | ||
|
|
||
| // Stop and remove containers by name | ||
| const containerNames = ['awf-agent', 'awf-squid']; | ||
| for (const name of containerNames) { | ||
| try { | ||
| // Check if container exists | ||
| const { stdout } = await execa('docker', ['ps', '-aq', '-f', `name=^${name}$`]); | ||
| if (stdout.trim()) { | ||
| logger.debug(`Stopping container: ${name}`); | ||
| await execa('docker', ['rm', '-f', name], { stdio: 'inherit' }); | ||
| } | ||
| } catch (err) { | ||
| logger.debug(`Could not stop container ${name}:`, err); | ||
| } | ||
| } | ||
|
|
||
| logger.success('Containers stopped successfully'); | ||
| } |
There was a problem hiding this comment.
The changes to stopContainers() are not mentioned in the PR title or description. The PR is titled "fix: prevent .cargo volume/tmpfs mount conflict in chroot mode" and the description only discusses the .cargo mount fix. The stopContainers changes appear to be a separate defensive improvement to handle missing docker-compose.yml files by falling back to stopping containers by name. Consider splitting this into a separate PR or updating the PR description to document this additional change.
This issue also appears in the following locations of the same file:
- line 1317
- line 1314
| } | ||
|
|
||
| // Mount ~/.cargo for Rust binaries (read-only) if it exists | ||
| // SKIP if allowFullFilesystemAccess is false (credentials will be hidden via tmpfs) |
There was a problem hiding this comment.
Consider making the comment more explicit about WHY the mount is skipped. The current comment explains the condition but not the underlying reason. Suggestion: "SKIP if allowFullFilesystemAccess is false to avoid Docker conflict (credentials will be hidden via tmpfs mount instead - Docker doesn't allow the same path to be both a volume and tmpfs)"
| // SKIP if allowFullFilesystemAccess is false (credentials will be hidden via tmpfs) | |
| // SKIP if allowFullFilesystemAccess is false to avoid Docker conflict: | |
| // credentials will instead be hidden via a tmpfs mount, and Docker does not | |
| // allow the same path to be both a volume and a tmpfs. |
Docker fails when the same path is mounted both as a volume and tmpfs. In chroot mode with credential hiding enabled (default),
~/.cargowas mounted twice:Changes
~/.cargovolume mount whenallowFullFilesystemAccess=false(default)allowFullFilesystemAccess=trueBehavior
Default (secure mode):
~/.cargohidden via tmpfs onlyWith
--allow-full-filesystem-access:/:/host:rwmount provides access to everything including~/.cargo