fix(cloud): request required organization scopes#164
Conversation
WalkthroughThis PR introduces OAuth scope-based access control to organization membership API operations. It adds version sorting utilities, token refresh and scope validation helpers, refactors client construction to require scopes, and integrates scope enforcement into stack and region commands. ChangesOAuth Scope-Based Access Control
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/stack/version_sort_test.go (1)
52-56: 💤 Low valueConsider adding test cases for invalid version inputs.
The test only covers valid semver cases. Given the behavior in
isVersionNewerThanCurrentfor invalid inputs (always returnstrue), adding test cases for edge scenarios would help document and verify intended behavior:
- Invalid candidate vs valid current
- Valid candidate vs invalid current
- Both invalid
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/stack/version_sort_test.go` around lines 52 - 56, Add tests to cover invalid-version edge cases for isVersionNewerThanCurrent: assert that when the candidate is invalid and current is valid the function returns true, when the candidate is valid and current is invalid it returns true, and when both candidate and current are invalid it returns true; place these new assertions alongside the existing cases in TestIsVersionNewerThanCurrent so the intended "invalid inputs => true" behavior is documented and verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/stack/version_sort.go`:
- Around line 41-48: The function isVersionNewerThanCurrent currently treats any
non-parseable candidate as newer; change it so that if normalizeSemver reports
the candidate is invalid you return false (do not treat invalid candidates as
upgrades). Keep the existing behavior of comparing normalizedCandidate and
normalizedCurrent when both valid, and if candidate is valid but current is not
you may still return true; update the logic in isVersionNewerThanCurrent (which
calls normalizeSemver and semver.Compare) so invalid candidate -> false,
validCandidate && validCurrent -> use semver.Compare, otherwise (validCandidate
&& !validCurrent) -> true.
---
Nitpick comments:
In `@cmd/stack/version_sort_test.go`:
- Around line 52-56: Add tests to cover invalid-version edge cases for
isVersionNewerThanCurrent: assert that when the candidate is invalid and current
is valid the function returns true, when the candidate is valid and current is
invalid it returns true, and when both candidate and current are invalid it
returns true; place these new assertions alongside the existing cases in
TestIsVersionNewerThanCurrent so the intended "invalid inputs => true" behavior
is documented and verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9ac3bf7-3778-43bc-9ba2-de3d526cf53d
📒 Files selected for processing (8)
cmd/cloud/regions/list.gocmd/stack/create.gocmd/stack/upgrade.gocmd/stack/version_sort.gocmd/stack/version_sort_test.gopkg/authentication.gopkg/authentication_test.gopkg/clients.go
| func isVersionNewerThanCurrent(candidate, current string) bool { | ||
| normalizedCandidate, validCandidate := normalizeSemver(candidate) | ||
| normalizedCurrent, validCurrent := normalizeSemver(current) | ||
| if validCandidate && validCurrent { | ||
| return semver.Compare(normalizedCandidate, normalizedCurrent) > 0 | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Invalid candidate versions are always treated as newer.
When candidate is not semver-parseable but current is valid, the function returns true, incorrectly presenting invalid versions as upgrade candidates. This could lead to users selecting non-semver versions (e.g., typos, placeholder strings) as upgrade targets.
Consider returning false when the candidate is invalid:
🐛 Proposed fix
func isVersionNewerThanCurrent(candidate, current string) bool {
normalizedCandidate, validCandidate := normalizeSemver(candidate)
normalizedCurrent, validCurrent := normalizeSemver(current)
if validCandidate && validCurrent {
return semver.Compare(normalizedCandidate, normalizedCurrent) > 0
}
- return true
+ // If candidate is not parseable, don't treat it as newer
+ // If current is not parseable, any valid candidate is acceptable
+ return validCandidate
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func isVersionNewerThanCurrent(candidate, current string) bool { | |
| normalizedCandidate, validCandidate := normalizeSemver(candidate) | |
| normalizedCurrent, validCurrent := normalizeSemver(current) | |
| if validCandidate && validCurrent { | |
| return semver.Compare(normalizedCandidate, normalizedCurrent) > 0 | |
| } | |
| return true | |
| } | |
| func isVersionNewerThanCurrent(candidate, current string) bool { | |
| normalizedCandidate, validCandidate := normalizeSemver(candidate) | |
| normalizedCurrent, validCurrent := normalizeSemver(current) | |
| if validCandidate && validCurrent { | |
| return semver.Compare(normalizedCandidate, normalizedCurrent) > 0 | |
| } | |
| // If candidate is not parseable, don't treat it as newer | |
| // If current is not parseable, any valid candidate is acceptable | |
| return validCandidate | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/stack/version_sort.go` around lines 41 - 48, The function
isVersionNewerThanCurrent currently treats any non-parseable candidate as newer;
change it so that if normalizeSemver reports the candidate is invalid you return
false (do not treat invalid candidates as upgrades). Keep the existing behavior
of comparing normalizedCandidate and normalizedCurrent when both valid, and if
candidate is valid but current is not you may still return true; update the
logic in isVersionNewerThanCurrent (which calls normalizeSemver and
semver.Compare) so invalid candidate -> false, validCandidate && validCurrent ->
use semver.Compare, otherwise (validCandidate && !validCurrent) -> true.
1 participant
Summary
Tests