fix: devcontainer obj/ files owned by root break incremental builds#534
fix: devcontainer obj/ files owned by root break incremental builds#534joshsmithxrm merged 4 commits intomainfrom
Conversation
postCreateCommand runs dotnet restore as root in some devcontainer CLI versions, creating obj/ directories owned by root:root. MSBuild's incremental build then fails with MSB3374 because utimensat() requires file ownership to set timestamps — 777 permissions aren't sufficient. Fix: explicitly set remoteUser to vscode and add a chown safety net in setup.sh after dotnet restore. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @joshsmithxrm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue in the devcontainer setup where Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with file ownership in the dev container that breaks incremental builds. The fix involves explicitly setting the remoteUser in devcontainer.json and adding a safety net in setup.sh to correct file permissions if the script runs as root. The changes are logical and well-explained. I have one suggestion to improve the robustness of the shell script by using the username vscode instead of a hardcoded UID/GID.
New `rebase` command rebases a feature branch onto origin/main: - Syncs origin refs first (reuses bundle mechanism) - Launches Claude Code for conflict resolution if rebase fails - Guards against running on main (use `sync` instead) Simplified `push` divergence handling: - Removed auto-rebase-onto-origin/branch + Claude logic from push - Replaced with force-with-lease when history diverged (e.g., after rebase) - Uses remoteSha from ls-remote as the expected lease value Also addresses review feedback: use vscode:vscode instead of 1000:1000 in setup.sh chown for robustness across image versions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@gemini-code-assist review |
|
Addressed in 978a40e — using |
There was a problem hiding this comment.
Code Review
The pull request addresses an important issue with file ownership in the devcontainer, which can break incremental builds. The scripts/devcontainer.ps1 introduces several instances where the $workdir variable is unsafely concatenated into bash command strings. While in this local development context, with $workdir derived from trusted sources like git worktree names, the risk is primarily related to shell injection from unescaped special characters rather than a high-severity command injection by an attacker, it is still important to address. It is recommended to use positional arguments with bash -c to safely pass variables and prevent unexpected command execution. The file ownership fix in .devcontainer/devcontainer.json and .devcontainer/setup.sh is robust, and the push command simplification (using force-with-lease) is a positive improvement. Future pull requests should ideally separate unrelated changes to streamline the review process.
Replace string interpolation of $workdir (and other variables) in bash -c commands with positional arguments via -- $arg. This prevents shell metacharacter injection from directory/branch names. Converts all 13 call sites to use 'cd "$1" && ...' -- $workdir pattern, verified working through the PowerShell → devcontainer exec → docker exec → bash argument chain. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed the shell injection feedback across all 13 call sites in 36baafc. Converted all \ to \ using positional args. Verified the \ pattern passes through the PowerShell → devcontainer exec → docker exec → bash chain correctly. |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with file ownership in the devcontainer that was breaking incremental builds. While it successfully hardens scripts/devcontainer.ps1 against command injection vulnerabilities by using parameterized arguments, a new security concern was identified. The recursive chown operation in .devcontainer/setup.sh could be exploited via malicious symbolic links to change the ownership of sensitive system files. Additionally, scripts/devcontainer.ps1 has been refactored to include a new rebase command and a safer push command. I have provided specific feedback regarding the chown vulnerability and a minor suggestion for consistency in the new rebase command.
setup.sh: use -h (no symlink follow) and --from=root (only fix root-owned files from dotnet restore, skip vscode-owned files). devcontainer.ps1: run git log in workdir context for consistency with all other devcontainer exec calls in the rebase block. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
postCreateCommandrunsdotnet restoreas root in some devcontainer CLI versions, creatingobj/directories owned byroot:rootMSB3374becauseutimensat()requires file ownership to set timestamps —777permissions aren't sufficientremoteUser: vscodetodevcontainer.jsonand achownsafety net insetup.shafterdotnet restoreTest plan
dev resetto rebuild the container from scratchdev ppdson main repo — verify build succeedsdev ppdson a worktree — verify build succeedsls -la src/PPDS.Cli/obj/Debug/net10.0/showsvscode:vscodeownership🤖 Generated with Claude Code