Skip to content

Conversation

@mawad-amd
Copy link
Collaborator

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

Copilot AI review requested due to automatic review settings January 19, 2026 00:36
@mawad-amd mawad-amd changed the title Muhaawad/dev container @mawad-amd Add dev container setup Jan 19, 2026
@mawad-amd mawad-amd changed the title @mawad-amd Add dev container setup Add dev container setup Jan 19, 2026
@mawad-amd mawad-amd merged commit bf93968 into main Jan 19, 2026
10 checks passed
@github-actions github-actions bot added in-progress We are working on it iris Iris project issue labels Jan 19, 2026
@mawad-amd mawad-amd deleted the muhaawad/dev-container branch January 19, 2026 00:36
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds VS Code Dev Container support to the Iris project, enabling developers to use the "Remote - Containers" extension for a consistent development environment. The changes include dev container configuration files, SSH agent setup for Git operations, and necessary group permissions for GPU access.

Changes:

  • Added .devcontainer/devcontainer.json with configuration for VS Code Dev Containers
  • Added .devcontainer/ensure-ssh-agent.sh script to manage SSH agent on the host
  • Updated copyright year in docker/Dockerfile to include 2026
  • Added video and render group creation in the Dockerfile
  • Modified .gitignore to explicitly include the devcontainer.json file

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 7 comments.

File Description
.devcontainer/devcontainer.json Configures VS Code Dev Container with GPU device access, SSH agent forwarding, and user setup
.devcontainer/ensure-ssh-agent.sh Shell script to ensure SSH agent is running on host with stable socket for container mounting
docker/Dockerfile Updated copyright year and added video/render group creation for GPU access
.gitignore Added exception to allow tracking of devcontainer.json despite blanket *.json ignore

mkdir -p "${HOME}/.ssh"

if [[ -S "${SOCK}" ]]; then
exit 0
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script exits successfully if the socket already exists (line 18), but it doesn't verify that the existing socket is actually functional or connected to a running ssh-agent. A stale socket file could exist from a previous crashed agent. Consider checking if the socket is functional by testing SSH_AUTH_SOCK before exiting early, or document this limitation.

Suggested change
exit 0
# Socket exists; verify that an ssh-agent is actually responding on it.
SSH_AUTH_SOCK="${SOCK}" ssh-add -l >/dev/null 2>&1 || status=$?
# ssh-add -l exit codes:
# 0: agent running, identities listed
# 1: agent running, no identities
# 2: agent not running / unreachable
if [[ ${status:-0} -eq 0 || ${status:-0} -eq 1 ]]; then
exit 0
fi

Copilot uses AI. Check for mistakes.
Comment on lines +23 to +28

if [[ -f "${HOME}/.ssh/id_rsa" ]]; then
SSH_AUTH_SOCK="${SOCK}" ssh-add "${HOME}/.ssh/id_rsa" >/dev/null 2>&1 || true
fi

SSH_AUTH_SOCK="${SOCK}" ssh-add -l >/dev/null 2>&1 || true
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SSH_AUTH_SOCK environment variable is set inline for individual commands (lines 25, 28) rather than being exported once. While this works, it's less efficient and harder to maintain. Consider exporting SSH_AUTH_SOCK="${SOCK}" once after starting the agent and before the ssh-add commands for better readability and consistency.

Suggested change
if [[ -f "${HOME}/.ssh/id_rsa" ]]; then
SSH_AUTH_SOCK="${SOCK}" ssh-add "${HOME}/.ssh/id_rsa" >/dev/null 2>&1 || true
fi
SSH_AUTH_SOCK="${SOCK}" ssh-add -l >/dev/null 2>&1 || true
export SSH_AUTH_SOCK="${SOCK}"
if [[ -f "${HOME}/.ssh/id_rsa" ]]; then
ssh-add "${HOME}/.ssh/id_rsa" >/dev/null 2>&1 || true
fi
ssh-add -l >/dev/null 2>&1 || true

Copilot uses AI. Check for mistakes.
Comment on lines +25 to +28
SSH_AUTH_SOCK="${SOCK}" ssh-add "${HOME}/.ssh/id_rsa" >/dev/null 2>&1 || true
fi

SSH_AUTH_SOCK="${SOCK}" ssh-add -l >/dev/null 2>&1 || true
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script silently ignores errors when adding SSH keys with the || true operator. While this prevents the script from failing when keys are passphrase-protected, it also masks other potential errors (e.g., corrupt key files, permission issues). Consider logging a warning message when ssh-add fails to help users understand why their keys may not be loaded.

Suggested change
SSH_AUTH_SOCK="${SOCK}" ssh-add "${HOME}/.ssh/id_rsa" >/dev/null 2>&1 || true
fi
SSH_AUTH_SOCK="${SOCK}" ssh-add -l >/dev/null 2>&1 || true
if ! SSH_AUTH_SOCK="${SOCK}" ssh-add "${HOME}/.ssh/id_rsa" >/dev/null 2>&1; then
echo "Warning: Failed to add SSH key ${HOME}/.ssh/id_rsa to ssh-agent at ${SOCK}. The key may be passphrase-protected or there may be a problem with the key file or its permissions." >&2
fi
fi
if ! SSH_AUTH_SOCK="${SOCK}" ssh-add -l >/dev/null 2>&1; then
echo "Warning: ssh-agent at ${SOCK} is running, but no keys are currently loaded or the agent is not responding as expected." >&2
fi

Copilot uses AI. Check for mistakes.
Comment on lines +15 to +17
"--device=/dev/dri",
"--cap-add=SYS_PTRACE",
"--group-add=video",
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The devcontainer configuration lacks inline comments explaining why certain security options are needed (e.g., --cap-add=SYS_PTRACE, --security-opt=seccomp=unconfined). These security-relaxing options could raise concerns during security reviews. Consider adding comments explaining that these are required for ROCm GPU debugging and profiling.

Suggested change
"--device=/dev/dri",
"--cap-add=SYS_PTRACE",
"--group-add=video",
"--device=/dev/dri",
// Required for ROCm GPU debugging/profiling tools that rely on ptrace inside the container.
"--cap-add=SYS_PTRACE",
"--group-add=video",
// ROCm debugging/profiling may use syscalls not allowed by the default seccomp profile; unconfined is needed for full ROCm GPU tooling support.

Copilot uses AI. Check for mistakes.
"--device=/dev/kfd",
"--device=/dev/dri",
"--cap-add=SYS_PTRACE",
"--group-add=video",
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The runArgs include --group-add=video on line 17, but the Dockerfile creates both video and render groups (lines 27-28). The render group is not added in runArgs, which could cause permission issues when accessing render devices. Consider adding --group-add=render to the runArgs for consistency with the groups created in the Dockerfile.

Suggested change
"--group-add=video",
"--group-add=video",
"--group-add=render",

Copilot uses AI. Check for mistakes.
"SSH_AUTH_SOCK": "/tmp/ssh-agent.sock"
},
"remoteUser": "vscode",
"postStartCommand": "bash -lc 'set -e; if ! getent group video >/dev/null; then sudo groupadd -r video || true; fi; if ! getent group render >/dev/null; then sudo groupadd -r render || true; fi; sudo usermod -aG video,render vscode || true'",
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The postStartCommand attempts to create video and render groups and add the vscode user to them. However, these groups are already created in the Dockerfile (lines 27-28). This creates redundancy and potential confusion. Consider removing the group creation from either the Dockerfile or the postStartCommand to maintain a single source of truth for group setup.

Suggested change
"postStartCommand": "bash -lc 'set -e; if ! getent group video >/dev/null; then sudo groupadd -r video || true; fi; if ! getent group render >/dev/null; then sudo groupadd -r render || true; fi; sudo usermod -aG video,render vscode || true'",
"postStartCommand": "bash -lc 'set -e; sudo usermod -aG video,render vscode || true'",

Copilot uses AI. Check for mistakes.
Comment on lines +36 to +39
"source=${localEnv:HOME}/.ssh/ssh-agent.sock,target=/tmp/ssh-agent.sock,type=bind"
],
"remoteEnv": {
"SSH_AUTH_SOCK": "/tmp/ssh-agent.sock"
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SSH agent socket mount relies on the ensure-ssh-agent.sh script to create the socket at ~/.ssh/ssh-agent.sock on the host. However, if the initializeCommand fails or the socket is not properly created, the container will attempt to mount a non-existent file, which could cause the container to fail to start. Consider adding error handling or verification that the socket exists before attempting to mount it.

Suggested change
"source=${localEnv:HOME}/.ssh/ssh-agent.sock,target=/tmp/ssh-agent.sock,type=bind"
],
"remoteEnv": {
"SSH_AUTH_SOCK": "/tmp/ssh-agent.sock"
"source=${localEnv:HOME}/.ssh,target=/tmp/ssh,type=bind"
],
"remoteEnv": {
"SSH_AUTH_SOCK": "/tmp/ssh/ssh-agent.sock"

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in-progress We are working on it iris Iris project issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants