Quality: HasCodeowners silently treats permission errors as "file not found"#764
Conversation
…t found" os.Stat is called and the error is unconditionally discarded. This means permission errors (EACCES), I/O errors (EIO), or other non-NotExist errors are treated identically to the file not existing — returning false. If the .github/CODEOWNERS file exists but the process lacks read permission (e.g., running in a restricted CI container, or after a bad chmod), this function silently returns false, causing downstream logic to skip CODEOWNERS enforcement when it should be enforced. Affected files: ghutils.go Signed-off-by: kumburovicbranko682-boop <[email protected]>
Current Aviator status
This pull request is currently open (not queued). How to mergeTo merge this PR, comment
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
🔃 FlexReview StatusCommon Owner:
Review SLO: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves error handling in HasCodeowners by explicitly checking for fs.ErrNotExist instead of assuming any error means the file does not exist. The review feedback suggests adding a defensive check to prevent a potential nil pointer dereference if repo is nil, and using separate arguments in filepath.Join for better platform-agnostic path construction.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func HasCodeowners(repo *git.Repo) bool { | ||
| if stat, _ := os.Stat(filepath.Join(repo.Dir(), ".github/CODEOWNERS")); stat != nil { | ||
| _, err := os.Stat(filepath.Join(repo.Dir(), ".github/CODEOWNERS")) |
There was a problem hiding this comment.
To prevent potential nil pointer dereferences, we should add a defensive check to ensure repo is not nil before calling repo.Dir(). Additionally, we can pass the path segments as separate arguments to filepath.Join for better platform-agnostic path construction.
| func HasCodeowners(repo *git.Repo) bool { | |
| if stat, _ := os.Stat(filepath.Join(repo.Dir(), ".github/CODEOWNERS")); stat != nil { | |
| _, err := os.Stat(filepath.Join(repo.Dir(), ".github/CODEOWNERS")) | |
| func HasCodeowners(repo *git.Repo) bool { | |
| if repo == nil { | |
| return false | |
| } | |
| _, err := os.Stat(filepath.Join(repo.Dir(), ".github", "CODEOWNERS")) |
1 participant
✨ Code Quality
Problem
os.Stat is called and the error is unconditionally discarded. This means permission errors (EACCES), I/O errors (EIO), or other non-NotExist errors are treated identically to the file not existing — returning false. If the .github/CODEOWNERS file exists but the process lacks read permission (e.g., running in a restricted CI container, or after a bad chmod), this function silently returns false, causing downstream logic to skip CODEOWNERS enforcement when it should be enforced.
Severity:
mediumFile:
internal/utils/ghutils/ghutils.goSolution
Replace the current check with proper error discrimination:
func HasCodeowners(repo *git.Repo) bool {
_, err := os.Stat(filepath.Join(repo.Dir(), ".github/CODEOWNERS"))
if err == nil {
return true
}
if errors.Is(err, fs.ErrNotExist) {
return false
}
// For permission/IO errors, assume the file exists to be safe.
return true
}
Changes
internal/utils/ghutils/ghutils.go(modified)Testing
🤖 About this PR
This pull request was generated by ContribAI, an AI agent
that helps improve open source projects. The change was:
If you have questions or feedback about this PR, please comment below.
We appreciate your time reviewing this contribution!
Closes #763