fix(devcontainer/setup): remove chown workspace flag for UID/GID permissions

[skevetter]

The devcontainer specification states that the UID/GID should be established when the container launches to prevent mount issues. Chown was implemented as a workaround to modify file permissions after the container had started. This resulted in inconsistent UID/GID permissions in the workspace and led to modified file permissions on the host. The file ownership on the host should not be modified by the devcontainer.

Signed-off-by: Samuel K skevetter@pm.me

[Copilot]

Pull request overview

This PR removes the --chown-workspace flag and associated functionality to fix UID/GID permission issues in devcontainers. The change prevents the devcontainer from modifying file permissions on the host filesystem, which was occurring due to a workaround that recursively changed ownership after container startup. The UID/GID updates now focus solely on container-internal user configuration.

Key changes:

• Refactored UID/GID update logic into a separate updateContainerUserUID() function with improved user resolution via resolveContainerUser()
• Removed the ChownWorkspace function and --chown-workspace flag from the setup process
• Simplified test file handling by removing unnecessary permission modifications

Reviewed changes

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

Show a summary per file

File	Description
pkg/driver/docker/docker.go	Refactored UID/GID update logic into dedicated functions; extracted inline logic to updateContainerUserUID(), added resolveContainerUser() for user detection hierarchy, and added extractRemoteUserFromMetadata() helper
pkg/devcontainer/setup/setup.go	Removed ChownWorkspace() function and updated SetupContainer() signature to remove the chownProjects parameter
pkg/devcontainer/setup.go	Removed the conditional logic that added --chown-workspace flag to setup command
cmd/agent/container/setup.go	Removed ChownWorkspace flag from CLI and updated SetupContainer() function call
e2e/tests/up/docker_compose_build.go	Simplified test by removing unnecessary chown/chmod operations when modifying devcontainer.json file

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The chown operation is still being performed recursively on the container home directory. According to the PR description, the goal is to prevent modifying file permissions that could affect the host. If the container home directory is mounted from the host, this recursive chown could still modify host file permissions, which contradicts the stated purpose of this PR.

Suggested change

	args = []string{"exec", "-u", "root", container.ID, "chown", "-R", fmt.Sprintf("%s:%s", localUid, localGid), containerHome}
	args = []string{"exec", "-u", "root", container.ID, "chown", fmt.Sprintf("%s:%s", localUid, localGid), containerHome}

[Copilot]

The workspace mount target is being chowned non-recursively, which is a change from the previous implementation. However, based on the PR description, the workspace mount should not be chowned at all since it's typically a bind mount from the host, and changing its ownership would modify host file permissions. Lines 752-759 should likely be removed entirely.

Suggested change

	if workspaceMount != nil && workspaceMount.Target != "" {
	args = []string{"exec", "-u", "root", container.ID, "chown", fmt.Sprintf("%s:%s", localUid, localGid), workspaceMount.Target}
	d.Log.WithFields(logrus.Fields{"command": d.Docker.DockerCommand, "args": strings.Join(args, " ")}).Debug("running docker chown workspace parent command")
	err = d.Docker.Run(ctx, args, nil, writer, writer)
	if err != nil {
	d.Log.WithFields(logrus.Fields{"error": err, "command": d.Docker.DockerCommand, "args": strings.Join(args, " ")}).Warn("failed to chown workspace parent directory")
	}
	}

[Copilot]

The condition on line 491 is redundant since line 486 already confirms the substring exists in the metadata. If the Contains check passes, the Index call on line 490 is guaranteed to return a valid position, making this check unnecessary. Consider removing this check or replacing it with a more defensive check like start < 0.

Suggested change

	if !strings.Contains(metadata, `"remoteUser":"`) {
	return ""
	}
	start := strings.Index(metadata, `"remoteUser":"`) + len(`"remoteUser":"`)
	if start <= len(`"remoteUser":"`) {
	return ""
	}
	prefix := `"remoteUser":"`
	idx := strings.Index(metadata, prefix)
	if idx < 0 {
	return ""
	}
	start := idx + len(prefix)