Conversation
WalkthroughAdds QR-code rendering to error console output by generating a terminal QR for the Discord support URL. Introduces an internal helper to produce the QR, updates ShowErrorAndExit to include it, adds tests for QR generation and a manual verification test, and declares indirect Go module dependencies for QR libraries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Caller as Caller
participant ErrSys as ErrorSystem
participant QR as generateQRCode
participant Lib as qrterminal
participant TTY as Console
Caller->>ErrSys: ShowErrorAndExit(error)
ErrSys->>QR: generateQRCode(discordURL)
QR->>Lib: render QR to buffer
Lib-->>QR: QR ASCII string
QR-->>ErrSys: QR string
ErrSys->>TTY: Print error details + help link + QR
TTY-->>User: Terminal output
note over ErrSys,TTY: New/changed step: QR generation and inclusion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
49-63: Update go.mod: promote qrterminal to a direct dependency and remove rsc.io/qrThe project’s code imports github.com/mdp/qrterminal/v3 directly (in internal/errsystem/console.go) and does not import rsc.io/qr itself—letting qrterminal pull in rsc.io/qr transitively is sufficient. Maintaining a manual, indirect pin for rsc.io/qr adds unnecessary churn and risk of version drift.
Please apply the following changes in go.mod:
• In the indirect block, remove the qrterminal entry:
- github.com/mdp/qrterminal/v3 v3.2.1 // indirect• Also remove the manual indirect pin for rsc.io/qr:
- rsc.io/qr v0.2.0 // indirect• In the main require block, add qrterminal as a direct dependency:
require ( github.com/Masterminds/semver v1.5.0 github.com/agentuity/go-common v1.0.71 + github.com/mdp/qrterminal/v3 v3.2.1 ... )
🧹 Nitpick comments (3)
internal/errsystem/console.go (1)
99-110: Tighten QR config and avoid shadowing the stdlib url package
- Variable name url shadows the imported net/url package in this file’s scope; prefer targetURL for clarity.
- QuietZone of 1 can make scanning unreliable on some terminals; 2–4 modules is safer without bloating too much.
Apply:
-// generateQRCode creates a QR code for the given URL and returns it as a string -func generateQRCode(url string) string { +// generateQRCode creates a QR code for the given URL and returns it as a string. +func generateQRCode(targetURL string) string { var buf bytes.Buffer config := qrterminal.Config{ Level: qrterminal.M, Writer: &buf, HalfBlocks: true, // Use half blocks to make QR code more square and compact - QuietZone: 1, + QuietZone: 2, // Improve scanner tolerance without too much padding } - qrterminal.GenerateWithConfig(url, config) + qrterminal.GenerateWithConfig(targetURL, config) return buf.String() }internal/errsystem/console_test.go (2)
34-36: Avoid calling ShowErrorAndExit in unit testsThis invokes os.Exit(1). Prefer extracting a pure renderer (e.g., renderErrorBody) to assert on output content, or use a subprocess helper to validate exit semantics.
I can help extract a render function from ShowErrorAndExit and add targeted tests if you want.
3-7: Use strings.ContainsAny; drop bespoke contains helpersReplace the custom contains/containsInner with the stdlib strings helpers and import strings.
Apply:
import ( "errors" "os" "testing" + "strings" )And remove the custom helpers below (lines 55–70).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(2 hunks)internal/errsystem/console.go(5 hunks)internal/errsystem/console_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-22T18:27:10.386Z
Learnt from: jhaynie
PR: agentuity/cli#313
File: cmd/cloud.go:852-853
Timestamp: 2025-05-22T18:27:10.386Z
Learning: In the agentuity CLI codebase, there's a specific error handling pattern: errsystem is used exclusively for unexpected system errors, while expected error conditions use tui.ShowWarning() and direct os.Exit(1) calls.
Applied to files:
internal/errsystem/console_test.go
🧬 Code graph analysis (1)
internal/errsystem/console_test.go (2)
internal/errsystem/errorcodes.go (1)
ErrInvalidConfiguration(25-28)internal/errsystem/errsystem.go (2)
WithContextMessage(100-104)WithUserMessage(70-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Build and Test (macos-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/errsystem/console.go (2)
4-4: Import of bytes is appropriateNeeded for capturing QR output via io.Writer in the buffer. No issues.
23-23: Dependency choice is finegithub.com/mdp/qrterminal/v3 is a solid choice for terminal QR rendering. Keep it as a direct dependency in go.mod (not indirect).
Ensure go.mod lists this as a direct require after running go mod tidy.
| // This test demonstrates the QR code functionality | ||
| // It will show the error banner with QR code, but we need to prevent actual exit | ||
| // We'll use a deferred recover to catch the os.Exit call | ||
|
|
There was a problem hiding this comment.
Recover cannot intercept os.Exit; the comment is incorrect and potentially misleading
os.Exit terminates the process immediately; defer/recover won’t run. This test as written will kill the test process when ShowErrorAndExit executes.
Remove the misleading comment and avoid calling ShowErrorAndExit directly from a normal unit test. See next comment for a safe pattern.
🤖 Prompt for AI Agents
In internal/errsystem/console_test.go around lines 10 to 13, the comment
claiming a deferred recover will catch os.Exit is incorrect and must be removed;
os.Exit terminates the process and defer/recover won’t run. Replace the direct
call to ShowErrorAndExit in the unit test with a safe pattern: either run the
code that calls ShowErrorAndExit in a separate subprocess (using os/exec to run
a helper binary/test that asserts exit code and output) or refactor the package
to inject an exit function (e.g., variable or interface) so tests can replace it
with a no-op or capture call; remove the misleading comment and implement one of
these safe approaches to prevent killing the test process.
| // Skip in CI or non-interactive environments | ||
| if os.Getenv("CI") != "" { | ||
| t.Skip("Skipping interactive test in CI environment") | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Skip this “manual” test by default; don’t rely solely on CI detection
Relying only on CI env risks accidental local exits during go test. Gate behind an explicit opt-in env var.
Apply:
- // Skip in CI or non-interactive environments
- if os.Getenv("CI") != "" {
- t.Skip("Skipping interactive test in CI environment")
- }
+ // Manual/interactive test: require explicit opt-in
+ if os.Getenv("ERRSYSTEM_QR_MANUAL") == "" {
+ t.Skip("Skipping manual ShowErrorAndExit test (set ERRSYSTEM_QR_MANUAL=1 to run)")
+ }Optionally, replace this test with a subprocess pattern that asserts exit code without killing the parent test runner. I can provide that snippet if you’d like.
🤖 Prompt for AI Agents
In internal/errsystem/console_test.go around lines 14 to 18, the test currently
skips only when CI env is set which can still cause accidental local exits;
change the guard so the test runs only when an explicit opt-in environment
variable is present (e.g., require TEST_INTERACTIVE=true or MANUAL_TEST=1) and
otherwise call t.Skip with a clear message; alternatively (recommended) convert
the test to a subprocess pattern that spawns a child process to exercise the
exit behavior and asserts its exit code without terminating the parent test
runner.
| func TestGenerateQRCode(t *testing.T) { | ||
| // Test that QR code generation works | ||
| qrCode := generateQRCode("https://discord.gg/agentuity") | ||
|
|
||
| // Basic validation that something was generated | ||
| if len(qrCode) == 0 { | ||
| t.Error("QR code generation returned empty string") | ||
| } | ||
|
|
||
| // Check that it contains expected QR code characters | ||
| if !contains(qrCode, "█") { | ||
| t.Error("QR code should contain block characters") | ||
| } | ||
|
|
||
| t.Logf("Generated QR code length: %d characters", len(qrCode)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Relax the assertion to support both full- and half-block render modes
generateQRCode currently uses HalfBlocks=true, which typically uses ▀/▄, not █. The test will flake/fail depending on config or library changes.
Apply:
@@
- // Check that it contains expected QR code characters
- if !contains(qrCode, "█") {
- t.Error("QR code should contain block characters")
- }
+ // Check that it contains expected QR code characters (full or half blocks)
+ if !strings.ContainsAny(qrCode, "█▀▄") {
+ t.Error("QR code should contain block-drawing characters")
+ }Also update imports to use the stdlib helper (see next comment).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
internal/errsystem/console_test.go lines 38-53: the test assumes full-block
characters (█) but generateQRCode uses HalfBlocks=true so it may produce ▀/▄;
relax the assertion to accept any block char by checking that the output is
non-empty and contains at least one of "█", "▀", or "▄"; replace the custom
contains helper with the standard library strings.Contains and update imports to
include "strings".
| func contains(s, substr string) bool { | ||
| return len(s) >= len(substr) && | ||
| (s == substr || | ||
| (len(s) > len(substr) && (s[:len(substr)] == substr || | ||
| s[len(s)-len(substr):] == substr || | ||
| containsInner(s, substr)))) | ||
| } | ||
|
|
||
| func containsInner(s, substr string) bool { | ||
| for i := 0; i <= len(s)-len(substr); i++ { | ||
| if s[i:i+len(substr)] == substr { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove custom contains helpers; they reimplement strings.Contains
These helpers add complexity and potential bugs. Use strings.Contains/ContainsAny instead.
Apply:
-func contains(s, substr string) bool {
- return len(s) >= len(substr) &&
- (s == substr ||
- (len(s) > len(substr) && (s[:len(substr)] == substr ||
- s[len(s)-len(substr):] == substr ||
- containsInner(s, substr))))
-}
-
-func containsInner(s, substr string) bool {
- for i := 0; i <= len(s)-len(substr); i++ {
- if s[i:i+len(substr)] == substr {
- return true
- }
- }
- return false
-}
+// Custom contains helpers removed in favor of strings.Contains/ContainsAny.Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/errsystem/console_test.go around lines 55 to 70, the custom contains
and containsInner helpers are reimplementing functionality provided by the
standard library; replace their usage with strings.Contains (or
strings.ContainsAny if matching any rune) and remove these helper functions, and
update imports to include "strings" if not already present; ensure tests call
strings.Contains(s, substr) where appropriate and delete the
contains/containsInner definitions.
| // Add community help message and QR code when running in terminal | ||
| qrCode := generateQRCode(discordURL) | ||
| body.WriteString(qrCode) | ||
| body.WriteString("\n" + tui.Bold("Get help from the Agentuity community at ")) | ||
| body.WriteString(tui.Link(discordURL) + " ") | ||
| body.WriteString(tui.Muted("(or scan the QR code)") + "\n\n") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gate QR rendering on TTY; avoid dumping block characters into logs/non-interactive output
The comment says “when running in terminal,” but this block runs unconditionally. In non-TTY contexts (CI logs, file redirection), the QR’s block characters reduce readability and bloat logs.
Apply:
- // Add community help message and QR code when running in terminal
- qrCode := generateQRCode(discordURL)
- body.WriteString(qrCode)
- body.WriteString("\n" + tui.Bold("Get help from the Agentuity community at "))
- body.WriteString(tui.Link(discordURL) + " ")
- body.WriteString(tui.Muted("(or scan the QR code)") + "\n\n")
+ // Add community help message and only render QR when running in a TTY
+ if tui.HasTTY && isatty.IsTerminal(os.Stdout.Fd()) {
+ qrCode := generateQRCode(discordURL)
+ body.WriteString(qrCode)
+ body.WriteString("\n" + tui.Bold("Get help from the Agentuity community at "))
+ body.WriteString(tui.Link(discordURL) + " ")
+ body.WriteString(tui.Muted("(or scan the QR code)") + "\n\n")
+ } else {
+ body.WriteString(tui.Bold("Get help from the Agentuity community at "))
+ body.WriteString(tui.Link(discordURL) + "\n\n")
+ }
|
this feels like a minority use-case, maybe we can we have a command like |
2 participants
SLACK CONVO HERE
Run to test:
go test -v ./internal/errsystem -run TestShowErrorAndExitWithQRCodeSummary by CodeRabbit
New Features
Chores
Tests