Conversation
WalkthroughThe PR adds a PostgreSQL wire-protocol client, CLI startup and configuration handling, responsive terminal rendering, and a Bubbletea v2 interactive query interface. ChangesPenguinDB CLI and interactive terminal
Sequence Diagram(s)sequenceDiagram
participant User
participant Bubbletea CLI
participant PGClient
participant PenguinDB Gateway
User->>Bubbletea CLI: Enter SQL statement
Bubbletea CLI->>PGClient: ExecuteQuery(sql)
PGClient->>PenguinDB Gateway: Send PostgreSQL query frame
PenguinDB Gateway-->>PGClient: Return result frames
PGClient-->>Bubbletea CLI: Return query result
Bubbletea CLI-->>User: Render formatted result or confirmation
Priority: ➖ Normal Merge Risk: 🟠 High · up to The CLI can mis-handle concurrent queries, hide connection failures, and allow destructive statements to bypass confirmation; the server also uses a gRPC version affected by a denial-of-service vulnerability. These issues should be fixed before merge. 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
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: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 0400c31b-8084-4da2-9c4e-fdf9fc21d700
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
cmd/cli/client.gocmd/cli/main.gocmd/cli/styles.gocmd/cli/ui.gogo.mod
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| c.conn.Write(lenBuf[:]) | ||
| c.conn.Write(payload.Bytes()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Propagate startup packet write failures.
Connect sets a 30-second deadline before the two unchecked startup writes. net.Conn.Write can return a partial count and an error. If either write fails, Connect ignores the failure and may wait for the handshake read until the deadline before Close runs. The CLI then exits with a misleading connection error.
Write the complete startup packet with a checked helper. Close the connection immediately on failure.
| c.Close() | ||
| return err | ||
| } | ||
| pData := make([]byte, pLen-4) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Validate server-controlled frame lengths before allocation.
Connect and ExecuteQuery pass pLen-4 directly to make; values such as pLen == 0 cause a negative-size panic, and no application maximum limits excessive allocations. Negative row and column counts also reach make. Any value length other than -1 can be negative, and a positive value length larger than r.Len() causes an oversized allocation before bytes.Reader.Read returns a short read.
Reject invalid outer lengths and negative counts. Enforce a maximum packet size. Accept -1 only for NULL values, and require other value lengths to be nonnegative and no greater than the remaining payload. The current code does not perform an out-of-bounds read because bytes.Reader.Read truncates the read, but it ignores that short-read result.
| if err := c.Connect(); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return a non-nil result when reconnection fails.
This path returns nil, err, but cmd/cli/ui.go discards the error and forwards only the result. FormatResultTable then converts the nil result to empty output.
Return a QueryResult with Error set, as the later network failure paths do. Alternatively, update execQueryCmd to preserve the separate error.
| } | ||
|
|
||
| if res.Error != nil { | ||
| return FormatErrorBox(res.Error.Error()) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Sanitize database text before terminal rendering.
The renderer passes errors, column names, and cell values directly to the terminal. A stored value or server response can contain ANSI, OSC, or other control sequences. These sequences can spoof output or modify terminal state.
Remove terminal control sequences before Lipgloss renders untrusted text. Preserve only explicitly supported characters such as newline and tab where required.
Also applies to: 193-193, 204-204
| case tea.KeyMsg: | ||
| switch msg.String() { | ||
| case "ctrl+c": | ||
| if len(m.multiBuffer) > 0 { | ||
| m.multiBuffer = nil | ||
| m.textInput.SetValue("") | ||
| m.textInput.Prompt = FormatPrompt(m.activeDb) | ||
| m.appendOutput("^C") | ||
| return m, nil | ||
| } | ||
| m.client.Close() | ||
| return m, tea.Quit |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Serialize PGClient.Close with in-flight ExecuteQuery.
execQueryCmd runs ExecuteQuery asynchronously. If Ctrl-C arrives during execution, Update calls m.client.Close concurrently. Although net.Conn supports concurrent method calls, PGClient does not synchronize its conn field. Close sets c.conn = nil while ExecuteQuery reads it for deadlines, writes, reads, and deferred cleanup. This can cause a data race or a nil-connection panic. Closing the socket can also interrupt the query.
Protect the connection lifecycle with synchronization, or cancel and wait for ExecuteQuery before Close clears c.conn. Ensure error cleanup inside ExecuteQuery does not re-enter a lock held for the execution. This is separate from preventing multiple query submissions.
| if strings.HasSuffix(fullSql, ";") { | ||
| m.history = append(m.history, fullSql) | ||
| m.multiBuffer = nil | ||
| m.textInput.Prompt = FormatPrompt(m.activeDb) | ||
| m.loading = true | ||
| return m, execQueryCmd(m.client, fullSql) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Prevent concurrent queries on the shared PostgreSQL connection.
The Enter handler does not check m.loading, so it can return another execQueryCmd before the first QueryResultMsg arrives. Bubble Tea v1.3.10 runs commands asynchronously. Each command calls PGClient.ExecuteQuery, which directly reads from and writes to c.conn. PGClient has no mutex or other serialization. Serialize or queue query submissions until the current query completes.
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: f9e2435f-64bd-4b7a-adc4-1739bf16489d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/cli/main.gocmd/cli/styles.gocmd/cli/ui.gogo.mod
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| if contentWidth < 20 { | ||
| contentWidth = bannerArtWidth | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep narrow terminal layouts within availWidth.
When availWidth is positive and narrow, RenderBanner can set contentWidth to 78. FormatHistory can leave long entries unwrapped when availWidth <= 14. FormatHelpPanel can reserve 40 columns for commands and leave descriptions unconstrained. These paths can overflow narrow terminals.
Apply one consistent width policy across the three formatters. Account for borders and padding, and reduce the help command column when necessary. The impact is localized visual overflow in these panels, not a major workflow failure.
| if total <= budget || budget <= numCols*minColWidth { | ||
| return natural, false | ||
| } | ||
|
|
||
| scaled := make([]int, numCols) | ||
| for c, w := range natural { | ||
| nw := int(float64(w) / float64(total) * float64(budget)) | ||
| if nw < minColWidth { | ||
| nw = minColWidth | ||
| } | ||
| scaled[c] = nw | ||
| } | ||
| return scaled, true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep result-table widths within the available terminal width.
computeColumnWidths can return widths whose sum exceeds budget. The minimum-width clamp can increase scaled columns after proportional allocation. The budget <= numCols*minColWidth branch can also return natural widths above the budget. FormatResultTable uses these widths when rendering the table, so narrow terminals can overflow. Apply a narrow-budget allocation policy that keeps the returned widths within the budget, including when the budget is below the minimum-width total.
| r := []rune(s) | ||
| if len(r) <= width-1 { | ||
| return s | ||
| } | ||
| return string(r[:width-1]) + "…" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Truncate by terminal display width.
This code slices by rune count after measuring with lipgloss.Width. Wide characters can still exceed width. For example, three double-width characters remain unchanged when width is four.
Build the prefix using display width, then append the ellipsis.
| reDropOrTruncate = regexp.MustCompile(`(?i)^\s*(DROP\s+(TABLE|DATABASE)|TRUNCATE)\b`) | ||
| reDeleteOrUpdate = regexp.MustCompile(`(?i)^\s*(DELETE\s+FROM|UPDATE)\b`) | ||
| reHasWhere = regexp.MustCompile(`(?i)\bWHERE\b`) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not classify destructive SQL with raw regular expressions.
reHasWhere matches text inside literals and comments. For example, UPDATE users SET note = 'WHERE'; bypasses confirmation but updates every row. Leading comments also prevent the anchored DROP and TRUNCATE expressions from matching.
Tokenize or parse the SQL before checking statement type and top-level WHERE clauses.
| if err != nil { | ||
| return FormatErrorBox(fmt.Sprintf("could not create file: %v", err), m.termWidth()) | ||
| } | ||
| defer f.Close() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Report file-close failures before confirming export success.
The deferred f.Close() error is discarded. A filesystem can accept writes but fail during final close, while this function still reports a successful export.
Close the file explicitly after w.Flush(). Return an error if Close fails.
| charm.land/lipgloss/v2 v2.0.6 | ||
| github.com/google/uuid v1.6.0 | ||
| google.golang.org/grpc v1.82.0 | ||
| google.golang.org/grpc v1.82.1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Upgrade gRPC beyond the affected version range.
google.golang.org/grpc v1.82.1 is affected by GHSA-vp52-pcj8-j9qc. An unauthenticated HTTP/2 attacker can exhaust heap memory through fragmented DATA frames and cause a denial of service. The advisory lists v1.83.1 as patched. Upgrade to v1.83.1 or a later patched release, then refresh the module sums. (github.com)
🧰 Tools
🪛 OSV Scanner (2.5.1)
[HIGH] 10-10: google.golang.org/grpc 1.82.1: gRPC-Go: Heap Memory Exhaustion (OOM) via HTTP/2 DATA Frame Fragmentation
Source: Linters/SAST tools
1 participant
Issue Reference
Summary by CodeRabbit