AGENT-420 added an option to open login OTP with enter #393
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe login command's user interaction was updated for improved clarity and automation. A new function, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Browser
User->>CLI: Run login command
CLI->>User: Display authentication URL and prompt
User->>CLI: Press ENTER
CLI->>CLI: Detect environment (DISPLAY variable)
alt GUI available
CLI->>Browser: Open authentication URL
Browser-->>CLI: Browser opened (success/failure)
CLI->>User: Show success or error message
else No GUI
CLI->>User: Instruct manual URL visit
end
Poem
✨ Finishing Touches🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
yoav0gal
changed the title
yoav0gal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/util/browser.go (1)
196-200: Parameter name shadows the importedurlpackage.Inside this function
url stringmasks thenet/urlimport, which is harmless here but hurts readability.
ConsidertargetURL stringto avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/auth.go(2 hunks)internal/util/browser.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: Build and Test (blacksmith-4vcpu-ubuntu-2204)
cmd/auth.go
[failure] 81-81:
non-constant format string in call to github.com/agentuity/go-common/tui.Link
internal/util/browser.go
[failure] 228-228:
non-constant format string in call to github.com/agentuity/go-common/tui.Link
🪛 golangci-lint (1.64.8)
cmd/auth.go
81-81: printf: non-constant format string in call to github.com/agentuity/go-common/tui.Link
(govet)
internal/util/browser.go
228-228: printf: non-constant format string in call to github.com/agentuity/go-common/tui.Link
(govet)
🪛 GitHub Actions: Go Build and Test
cmd/auth.go
[error] 81-81: non-constant format string in call to github.com/agentuity/go-common/tui.Link
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
internal/util/browser.go (1)
167-170: Good catch – switched to%wfor proper error wrapping.
The updated formatting propagates the original error correctly for upstream inspection.
| // Clear the prompt and show the URL for manual opening (and the loading spinner) | ||
| fmt.Print("\r\033[K") | ||
| fmt.Println(tui.Muted("Please visit the URL manually:")) | ||
| fmt.Println(tui.Link(url)) | ||
| fmt.Println() |
There was a problem hiding this comment.
tui.Link called with a dynamic format string triggers vet failure.
go vet (and your pipeline) flags fmt.Println(tui.Link(url)) because url is treated as a format string and may contain % tokens. Pass a constant format instead:
- fmt.Println(tui.Link(url))
+ fmt.Println(tui.Link("%s", url))📝 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.
| // Clear the prompt and show the URL for manual opening (and the loading spinner) | |
| fmt.Print("\r\033[K") | |
| fmt.Println(tui.Muted("Please visit the URL manually:")) | |
| fmt.Println(tui.Link(url)) | |
| fmt.Println() | |
| // Clear the prompt and show the URL for manual opening (and the loading spinner) | |
| fmt.Print("\r\033[K") | |
| fmt.Println(tui.Muted("Please visit the URL manually:")) | |
| - fmt.Println(tui.Link(url)) | |
| + fmt.Println(tui.Link("%s", url)) | |
| fmt.Println() |
🧰 Tools
🪛 GitHub Check: Build and Test (blacksmith-4vcpu-ubuntu-2204)
[failure] 228-228:
non-constant format string in call to github.com/agentuity/go-common/tui.Link
🪛 golangci-lint (1.64.8)
228-228: printf: non-constant format string in call to github.com/agentuity/go-common/tui.Link
(govet)
🤖 Prompt for AI Agents
In internal/util/browser.go around lines 225 to 229, the call to tui.Link with
url as a dynamic string causes go vet to fail because url may contain percent
signs interpreted as format verbs. To fix this, change the call to pass a
constant format string and url as an argument, ensuring tui.Link treats url as
data, not a format string.
| if skipOpen { | ||
| fmt.Print(tui.Secondary("Press Enter to continue, or Ctrl+C to skip: ")) | ||
| } else { | ||
| fmt.Print(tui.Secondary("Press Enter to open browser, or Ctrl+C to skip: ")) | ||
| } | ||
|
|
||
| reader := bufio.NewReader(os.Stdin) | ||
| reader.ReadLine() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ignored error from ReadLine may hide I/O issues.
reader.ReadLine() returns (line, isPrefix, err) – you’re discarding err.
At minimum, capture and log/ignore io.EOF explicitly to avoid silent failures in unusual terminals (e.g., piped input).
Example quick fix:
- reader := bufio.NewReader(os.Stdin)
- reader.ReadLine()
+ reader := bufio.NewReader(os.Stdin)
+ if _, _, err := reader.ReadLine(); err != nil && !errors.Is(err, io.EOF) {
+ logger.Error("Error reading stdin: %v", err)
+ return
+ }📝 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.
| if skipOpen { | |
| fmt.Print(tui.Secondary("Press Enter to continue, or Ctrl+C to skip: ")) | |
| } else { | |
| fmt.Print(tui.Secondary("Press Enter to open browser, or Ctrl+C to skip: ")) | |
| } | |
| reader := bufio.NewReader(os.Stdin) | |
| reader.ReadLine() | |
| if skipOpen { | |
| fmt.Print(tui.Secondary("Press Enter to continue, or Ctrl+C to skip: ")) | |
| } else { | |
| fmt.Print(tui.Secondary("Press Enter to open browser, or Ctrl+C to skip: ")) | |
| } | |
| reader := bufio.NewReader(os.Stdin) | |
| if _, _, err := reader.ReadLine(); err != nil && !errors.Is(err, io.EOF) { | |
| logger.Error("Error reading stdin: %v", err) | |
| return | |
| } |
🤖 Prompt for AI Agents
In internal/util/browser.go around lines 206 to 214, the error returned by
reader.ReadLine() is ignored, which can hide I/O issues. Modify the code to
capture the error returned by ReadLine(), then explicitly check if the error is
io.EOF and handle it gracefully; for other errors, log or handle them
appropriately to avoid silent failures.
cmd/auth.go
Outdated
|
|
||
| authURL := fmt.Sprintf("%s/auth/cli", appUrl) | ||
|
|
||
| body := tui.Paragraph( | ||
| "Copy the following code:", | ||
| tui.Bold(otp), | ||
| "Then open the url in your browser and paste the code:", | ||
| tui.Link("%s/auth/cli", appUrl), | ||
| "Then open the url in your browser (Or just press ENTER) and paste the code:", | ||
| tui.Link(authURL), | ||
| tui.Muted("This code will expire in 60 seconds"), | ||
| ) |
There was a problem hiding this comment.
Same vet error – non-constant format string in tui.Link.
The authentication banner fails the build for the same reason:
- "Then open the url in your browser (Or just press ENTER) and paste the code:",
- tui.Link(authURL),
+ "Then open the url in your browser (or just press ENTER) and paste the code:",
+ tui.Link("%s", authURL),Apply the change to unblock CI.
📝 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.
| authURL := fmt.Sprintf("%s/auth/cli", appUrl) | |
| body := tui.Paragraph( | |
| "Copy the following code:", | |
| tui.Bold(otp), | |
| "Then open the url in your browser and paste the code:", | |
| tui.Link("%s/auth/cli", appUrl), | |
| "Then open the url in your browser (Or just press ENTER) and paste the code:", | |
| tui.Link(authURL), | |
| tui.Muted("This code will expire in 60 seconds"), | |
| ) | |
| authURL := fmt.Sprintf("%s/auth/cli", appUrl) | |
| body := tui.Paragraph( | |
| "Copy the following code:", | |
| tui.Bold(otp), | |
| "Then open the url in your browser (or just press ENTER) and paste the code:", | |
| tui.Link("%s", authURL), | |
| tui.Muted("This code will expire in 60 seconds"), | |
| ) |
🧰 Tools
🪛 GitHub Check: Build and Test (blacksmith-4vcpu-ubuntu-2204)
[failure] 81-81:
non-constant format string in call to github.com/agentuity/go-common/tui.Link
🪛 golangci-lint (1.64.8)
81-81: printf: non-constant format string in call to github.com/agentuity/go-common/tui.Link
(govet)
🪛 GitHub Actions: Go Build and Test
[error] 81-81: non-constant format string in call to github.com/agentuity/go-common/tui.Link
🤖 Prompt for AI Agents
In cmd/auth.go around lines 74 to 83, the call to tui.Link uses a non-constant
format string causing a vet error and build failure. To fix this, replace the
dynamic string passed to tui.Link with a constant string or refactor the code to
avoid passing a formatted string directly. This change will unblock the CI
build.
|
@jhaynie Implementing this would require changes to our web app, but I think it offers a better developer experience (which we want to maximize). It's the little things that count. Also, what do you think about implementing something similar for dev mode? I think that would be the most used. |
yeah the problem with pass in the URL is its probably a potential secure vector.. i think we would likely need to encrypt the code, put it in the URL and then on the other side we could decrypt it. The problem there is we don't have an easy way to do this safely since this is a login flow and we can't assume any sort of trust. would need to give it some thought on how to do this safely. |
|
I've looked into this a bit, but I think I'm a bit confused and out of my depth. If I understand correctly, the main concern is that somebody could get the one-time password (OTP) from the URL and poll the credentials before the CLI does (assuming the OTP was already validated but not yet claimed). Anyone could then just: curl -X POST https://api.agentuity.com/cli/auth/check \
-H "Content-Type: application/json" \
-d '{"otp":"ABC123"}'...and get back our API key and I've looked into some potential solutions:
I feel like I'm out of my depth here and need some guidance. I can definitely wrestle with it more and create some prototypes, but I think the smart move would be for us to talk before I invest more time into this without a green light from someone who knows better than me. |
yoav0gal
changed the title
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Style
yoav/agent-421-open-browser-with-enter-when-agentuity-login