fix: skip chmod/chown on pre-existing dirs in MakeDir#1025
Conversation
MakeDir unconditionally ran chmod+chown after MkdirAll, breaking createDirectories for pre-existing system dirs like /tmp where the sandbox user lacks permission. Now only applies chmod to newly created dirs. Also fixes SetFileOwnership to no-op when no owner/group specified instead of forcing chown to current uid/gid. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 987154ee63
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Skip exact mode assertion on Windows where os.Chmod only maps the read-only attribute - Add os.Stat check in SetFileOwnership no-op path so empty permissions on a missing file still returns ENOENT - Add TestSetFileOwnership_MissingFile test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c181b601d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- SetFileOwnership now returns error when owner/group is specified but lookup fails, instead of silently ignoring - Early return nil only when both owner and group are empty (truly unspecified) - Add Windows skip guards to ownership tests since SetFileOwnership is a no-op placeholder on Windows - Add TestSetFileOwnership_InvalidOwner test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6332b40d8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
3 participants
Summary
MakeDirnow checks if directory exists beforeMkdirAll; only appliesChmodFileto newly created dirsSetFileOwnershipreturns nil when both owner and group are empty, instead of forcing chown to current uid/gidCloses #1024
Test plan
TestMakeDir_PreExistingDir— pre-existing dir not chmod'd, no errorTestMakeDir_NewDir— new dir created with correct permissionsTestSetFileOwnership_EmptyOwnerGroup— empty owner/group is no-op🤖 Generated with Claude Code