fix: avoid startup SSO login#223
Conversation
- skip startup caller identity lookup when SSO login would be required - keep context switching SSO login behavior unchanged - document passive SSO startup behavior Closes #221
|
Warning Review limit reached
More reviews will be available in 54 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughModel.Init() now conditionally skips caller-identity loading at startup when SSO session checks indicate login is required, preventing interactive AWS SSO authentication. Identity loads only when the SSO cache is valid or for non-SSO auth types. Tests and README document the new passive startup behavior. ChangesConditional SSO Startup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR successfully implements passive SSO startup behavior to prevent interactive login prompts at TUI startup. The implementation correctly checks SSO session validity before attempting to load caller identity, falling back to an empty identity message when login would be required. The changes are well-tested with comprehensive unit tests covering all three scenarios: missing SSO cache, valid SSO cache, and non-SSO auth types. No defects found that would block merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![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
🤖 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 `@internal/app/app_test.go`:
- Around line 154-227: Add a test that asserts loadStartupCallerIdentity()
treats a failing SSO session check like the LoginRequired case: set
contextCheckSSOSessionFn to return an error (and flip a local checkCalled flag),
stub appLoadCallerIdentityFn to set loadCalled if invoked, call New(...) and
invoke m.loadStartupCallerIdentity()(), then assert the returned msg is a
callerIdentityMsg, checkCalled is true and loadCalled is false; remember to save
and restore the original contextCheckSSOSessionFn and appLoadCallerIdentityFn in
a defer like the other tests.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7a14abd9-b9b7-4b26-8633-9a452e6aca58
📒 Files selected for processing (3)
README.mdinternal/app/app.gointernal/app/app_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
README.md
📄 CodeRabbit inference engine (CLAUDE.md)
README.md: When adding, modifying, or deleting features, always updateREADME.mdin parallel with code changes
UpdateCurrently Implemented Featurestable in README.md: add new services/features, update status changes (🚧→✅), remove deleted items
UpdateTUI Key Bindingstable in README.md when key bindings are added, changed, or deleted
UpdateUsagesection in README.md when new CLI commands or flags are added
UpdateConfigurationsection in README.md when configuration format changes
Files:
README.md
⚙️ CodeRabbit configuration file
README.md: Verify that README changes match actual CLI/TUI behavior and that
Currently Implemented Features, TUI Key Bindings, Usage, and
Configuration content stay aligned with code changes.
Files:
README.md
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Use lipgloss for styled TUI output — column-aligned tables with dimmed labels in Go implementation files
Implement scroll windowing with formula:visibleLines := max(m.height-N, 5)in Go TUI implementation
Files:
internal/app/app.gointernal/app/app_test.go
⚙️ CodeRabbit configuration file
**/*.go: For Go reviews, look beyond compilation and prioritize nil pointer risks,
context propagation, AWS SDK pagination, error wrapping, deterministic
sorting, and stable table/detail rendering. For new AWS service work,
verify that repository interfaces, model mapping, app integration, and
tests are updated together.
Files:
internal/app/app.gointernal/app/app_test.go
internal/app/**
⚙️ CodeRabbit configuration file
internal/app/**: For Bubble Tea screen changes, verify message routing, key handling,
filter target resets, height-based windowing, help text, and back/home
navigation against the existing screen patterns.
Files:
internal/app/app.gointernal/app/app_test.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Tests use mock client interfaces (see
rds_test.gopattern) in Go test files
Files:
internal/app/app_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Check that tests cover API errors, mapping edge cases, and navigation
state transitions, not only happy paths. Prefer mock-based tests that do
not depend on external AWS calls.
Files:
internal/app/app_test.go
🔇 Additional comments (3)
README.md (1)
193-194: LGTM!internal/app/app.go (1)
274-302: LGTM!internal/app/app_test.go (1)
229-256: LGTM!
- assert failed SSO session checks skip startup identity loading - cover the same passive behavior as missing SSO cache
|
/q review |
There was a problem hiding this comment.
This PR implements passive SSO startup behavior for the TUI, preventing automatic aws sso login prompts during initialization. The implementation is clean with comprehensive test coverage.
Key Changes:
- Adds
loadStartupCallerIdentity()that checks SSO cache validity before loading caller identity - Introduces
appLoadCallerIdentityFnvariable for improved testability - Provides thorough test coverage for SSO cache scenarios
- Documents the passive SSO behavior in README
The code correctly handles the different SSO session states (cache missing, check failure, valid cache) and preserves existing behavior for non-SSO authentication types. All changes follow existing patterns in the codebase.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
1 participant
Summary
Related Issues
Closes #221
Validation
Checklist
Summary by CodeRabbit
New Features
Documentation