Initial code#1
Open
peteraba wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe PR introduces Changeslitegocli Application
🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (2)
.goreleaser.yaml (1)
19-19:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix the package path to match the repository.
The ldflags reference
github.com/peter/litegocli/internal/app.Version, but the repository appears to bepeteraba/litegoclibased on the PR URL. This is the same issue as in theMakefile(line 2) and will prevent version injection from working.🔧 Proposed fix
- - -X github.com/peter/litegocli/internal/app.Version={{.Version}} + - -X github.com/peteraba/litegocli/internal/app.Version={{.Version}}🤖 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 @.goreleaser.yaml at line 19, Update the ldflags entry that sets the Version to use the correct repository import path: replace the incorrect package path `github.com/peter/litegocli/internal/app.Version` with the actual repository path `github.com/peteraba/litegocli/internal/app.Version` so the `-X ...Version={{.Version}}` flag injects the version correctly; ensure the same change is mirrored wherever the old path is referenced (e.g., Makefile) and keep the `Version` symbol name unchanged.README.md (1)
29-29:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpdate installation URLs to match the actual repository.
The installation command (line 29) and clone URL (line 35) reference
github.com/peter/litegocli, but the PR URL indicates the repository ispeteraba/litegocli. This is the same package path issue present in theMakefile,.goreleaser.yaml, and will causego installto fail.📝 Proposed fix
-go install github.com/peter/litegocli@latest +go install github.com/peteraba/litegocli@latest-git clone https://github.com/peter/litegocli +git clone https://github.com/peteraba/litegocliAlso applies to: 35-35
🤖 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 `@README.md` at line 29, Update all package paths and URLs that reference the wrong GitHub owner from "github.com/peter/litegocli" to "github.com/peteraba/litegocli": change the README installation command string "go install github.com/peter/litegocli@latest" and the clone URL occurrences, and also update matching entries in Makefile and .goreleaser.yaml so go install and clone commands point to the correct repository path.
🟡 Minor comments (5)
internal/specials/registry.go-82-90 (1)
82-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake suggestions case-insensitive to match command dispatch.
Handlelowercases command names (Line [73]), butSuggestcompares case-sensitively (Line [85]). This causes missed completions for equivalent casing (for example, lowercase input for uppercase command names).Suggested fix
func (r *Registry) Suggest(prefix string) []Command { var out []Command + p := strings.ToLower(prefix) for _, c := range r.commands { - if strings.HasPrefix(c.Name, prefix) { + if strings.HasPrefix(strings.ToLower(c.Name), p) { out = append(out, *c) } } sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) return out }🤖 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 `@internal/specials/registry.go` around lines 82 - 90, Suggest currently matches prefixes case-sensitively while Handle lowercases command names; update Registry.Suggest to perform case-insensitive matching by comparing lowercased values (e.g., use strings.ToLower on both prefix and each Command.Name from r.commands) and also sort the results case-insensitively (compare strings.ToLower(out[i].Name) < strings.ToLower(out[j].Name)) so suggestions align with the dispatch behavior.internal/specials/backslash.go-185-189 (1)
185-189:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
pagerstatus query should not enable pager.At Line [188], running
pagerwith no args currently turns pager on. That branch is described as “show current pager command,” so it should be read-only.Suggested fix
func handlePager(s *session.State, args []string) error { if len(args) == 0 { fmt.Fprintln(s.Out, "pager:", currentPager(s)) - s.Renderer.SetPagerEnabled(true) return nil }🤖 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 `@internal/specials/backslash.go` around lines 185 - 189, The no-arg branch of handlePager currently flips the pager on; change it to be read-only by removing the call to s.Renderer.SetPagerEnabled(true) (or avoid mutating Renderer state) in the len(args) == 0 branch so it only prints currentPager(s) and returns; locate handlePager, the len(args) == 0 branch, currentPager(s) usage and Renderer.SetPagerEnabled to make this change.internal/db/db_test.go-13-13 (1)
13-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCheck
Close()errors in test cleanup.Line 13 and Line 46 ignore
conn.Close()errors, which can mask teardown failures.Suggested fix
- defer conn.Close() + t.Cleanup(func() { + if err := conn.Close(); err != nil { + t.Errorf("Close: %v", err) + } + })Also applies to: 46-46
🤖 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 `@internal/db/db_test.go` at line 13, Test cleanup currently ignores errors from conn.Close() in db_test.go; update both deferred conn.Close() calls to check and fail the test on error—e.g., replace defer conn.Close() with a deferred closure that captures the return value and calls t.Fatalf/t.Errorf or require.NoError(t, err) using the existing *testing.T variable to surface teardown failures (target the conn.Close() calls in the tests).internal/db/libsql.go-40-52 (1)
40-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
authTokenbefore appending to DSN.Line 50 treats whitespace-padded tokens as valid values, which can break authentication with copied/env tokens.
Suggested fix
func buildLibsqlDSN(rawURL, authToken string) (string, error) { rawURL = strings.TrimSpace(rawURL) + authToken = strings.TrimSpace(authToken) if rawURL == "" { return "", fmt.Errorf("empty libsql URL") } @@ q := u.Query() if q.Get("authToken") == "" && q.Get("auth_token") == "" && authToken != "" { q.Set("authToken", authToken) }🤖 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 `@internal/db/libsql.go` around lines 40 - 52, In buildLibsqlDSN, normalize the incoming authToken by calling strings.TrimSpace(authToken) into a local variable and use that trimmed value when checking whether to append and when calling q.Set("authToken", ...); this ensures whitespace-only/padded tokens from env/copy-paste are treated as empty and prevents writing raw padded tokens into the DSN. Also use the trimmed value when comparing existing query params if you want to avoid treating " auth " as present.internal/db/integration_test.go-24-24 (1)
24-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle connection close errors during test cleanup.
Line 24 defers
conn.Close()without checking its returned error, so teardown failures are silently ignored.Proposed fix
- defer conn.Close() + t.Cleanup(func() { + if err := conn.Close(); err != nil { + t.Errorf("Close: %v", err) + } + })🤖 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 `@internal/db/integration_test.go` at line 24, Replace the bare defer conn.Close() with a deferred closure that checks and reports the error from conn.Close(); e.g. defer func() { if err := conn.Close(); err != nil { t.Fatalf("failed to close conn: %v", err) } }() so teardown failures are surfaced (reference the conn.Close() call in integration_test.go and use the test's t to report the error).
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
18-19: ⚖️ Poor tradeoffConsider pinning GitHub Actions to commit SHAs for supply-chain security.
The workflow uses semantic version tags (
@v4,@v5) for actions. While convenient and widely used, pinning to specific commit SHAs provides stronger supply-chain guarantees against tag manipulation.Example:
- uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 # v4.0.0 - uses: actions/setup-go@0a12ed9d6a96ab950c8f026ed9f722fe0da7ef32 # v5.0.0This is a project-specific security posture decision; many projects accept semantic tags as sufficient.
Also applies to: 45-46
🤖 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 @.github/workflows/ci.yml around lines 18 - 19, The workflow uses semantic tags for GitHub Actions (actions/checkout@v4 and actions/setup-go@v5) which can be subject to tag manipulation; update those usages to pinned commit SHAs instead (replace the `@v4` and `@v5` references with the corresponding commit SHA for the exact release) for stronger supply-chain security, and do the same for the other occurrences referenced in the comment (the other actions at lines shown).internal/repl/completer.go (1)
115-117: ⚡ Quick winPrecompile regexes used in completion hot paths.
regexp.MustCompileis currently executed on each completion pass. Moving patterns to package-level vars avoids repeated compilation during typing.Suggested refactor
+var ( + tokenRE = regexp.MustCompile(`[A-Za-z_][A-Za-z0-9_]*`) + extractTblRE = regexp.MustCompile(`\b(?:from|join|update|into)\s+([a-zA-Z_][a-zA-Z0-9_]*)(?:\s+(?:as\s+)?([a-zA-Z_][a-zA-Z0-9_]*))?`) +) + func tokenize(text string) []string { - re := regexp.MustCompile(`[A-Za-z_][A-Za-z0-9_]*`) - return re.FindAllString(text, -1) + return tokenRE.FindAllString(text, -1) } @@ func extractTables(text string) []string { var out []string - re := regexp.MustCompile(`\b(?:from|join|update|into)\s+([a-zA-Z_][a-zA-Z0-9_]*)(?:\s+(?:as\s+)?([a-zA-Z_][a-zA-Z0-9_]*))?`) - for _, m := range re.FindAllStringSubmatch(text, -1) { + for _, m := range extractTblRE.FindAllStringSubmatch(text, -1) { out = append(out, m[1]) } return out }Also applies to: 134-135
🤖 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 `@internal/repl/completer.go` around lines 115 - 117, The regex in tokenize (regexp.MustCompile(`[A-Za-z_][A-Za-z0-9_]*`)) is compiled on every completion pass; move this pattern to a package-level variable (e.g., var identRE = regexp.MustCompile(...)) and update tokenize to call identRE.FindAllString(text, -1). Do the same for the other regex used around the 134-135 region (precompile it as a package-level var and reuse it instead of calling regexp.MustCompile inside the hot path).
🤖 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 @.github/workflows/ci.yml:
- Line 55: The CI go build step currently omits the -X linker flag so CI-built
binaries lack version info; update the run command (`run: go build -trimpath
-ldflags ... -o litegocli-${{ matrix.goos }}-${{ matrix.goarch }} .`) to include
an -X assignment that injects the build version (use `${{ github.sha }}` or
`dev-${{ github.sha }}`) into the same package-level variable your
Makefile/.goreleaser set (e.g., main.Version or the project's version symbol) so
the binary contains the version string.
In `@go.mod`:
- Line 1: The module path in go.mod is incorrect: change the module declaration
from "github.com/peter/litegocli" to the canonical
"github.com/peteraba/litegocli" and update any related references (search for
occurrences of "github.com/peter/litegocli" in the repo, including build flags
like -ldflags, import statements, and README install instructions) so all
module/import paths and documentation consistently use
"github.com/peteraba/litegocli" (or alternatively document a vanity/redirect if
you intend to keep the original string).
In `@internal/app/app.go`:
- Around line 214-224: The quote-scanning branch in app.go (the loop that
handles case '\'' and '"' using variables c, input, i, and b) incorrectly treats
the first matching quote as the end even when SQL uses doubled quotes as
escapes; update the inner loop to detect doubled-quote escapes by checking if
input[i] == c and the next character (input[i+1]) is also c, in which case write
both quote characters to b and advance i past both without breaking, and only
treat a single c not followed by the same quote as the string terminator; ensure
bounds checks when peeking input[i+1].
- Around line 182-201: runScript currently swallows errors (printing them)
during script/--execute runs; update runScript so that when running
non-interactively (script/--execute mode) any failure from Specials.Handle,
s.DB.Run (except db.ErrEmpty) or s.Renderer.Render is returned as a non-nil
error instead of only logging to s.Err. Concretely, detect the
non-interactive/execute flag used by the caller (the runScript context), and on
handled-but-error from Specials.Handle, on err from s.DB.Run (unless
errors.Is(err, db.ErrEmpty)), and on err from s.Renderer.Render, return that
error (or aggregate and return a combined error) so the process exits non-zero;
retain printing for interactive mode using the same s.Err logging code.
In `@internal/browser/browser.go`:
- Around line 30-31: The selected-table lookup uses mm.tables with mm.cursor
while the cursor is maintained against the filtered set; update the
Enter/selection path that builds SQL (the branch testing final.(model) and
mm.chosen) to index into the filtered slice (call the filtered() method or
equivalent on model) instead of mm.tables and ensure you guard against
out-of-range cursor (check length > cursor) before accessing Name so the SQL
always refers to the highlighted filtered table.
In `@internal/config/config.go`:
- Around line 128-131: The Save() implementation currently ignores errors from
f.NewSection("main") and subsequent m.NewKey(...) calls (and similarly
fav.NewKey(...) in the favorites loop), so fix Save() to capture and return any
errors from f.NewSection and each NewKey call instead of discarding them;
specifically, check the error returned by f.NewSection("main") (m, err :=
f.NewSection("main")) and return it on failure, and for each m.NewKey(...) and
fav.NewKey(...) call capture the returned (key, err) and propagate the err
(e.g., return or accumulate and return the first/non-nil) so file/section/key
creation failures are not silently ignored.
In `@internal/db/db.go`:
- Around line 145-151: isQueryStatement currently treats any statement starting
with "WITH" as row-producing and ignores DMLs with RETURNING; update it to (1)
detect DML verbs ("INSERT","UPDATE","DELETE") as row-producing only when the
statement contains a top-level "RETURNING" clause and (2) when the leading
keyword is "WITH" use leadingKeyword on the remainder after the CTE header(s)
(i.e., skip the CTE block(s)) to determine the actual outer statement instead of
assuming "WITH" is enough; use the existing leadingKeyword helper to extract the
outer verb and check for "SELECT","VALUES","SHOW","PRAGMA","EXPLAIN" or
DML+RETURNING to decide true/false for isQueryStatement.
- Around line 55-56: The DB methods currently call QueryContext/ExecContext with
context.Background(), preventing caller cancellation; update the Run and
QueryRowsAsStrings functions to accept a context.Context parameter (or thread an
existing ctx through their callers) and replace context.Background() with that
ctx when calling c.DB.QueryContext and c.DB.ExecContext (references: Run,
QueryRowsAsStrings, and any callers of those functions) so queries/execs respect
deadlines and cancellations.
In `@internal/logging/logging.go`:
- Around line 54-59: When opening the audit log (cfg.Main.AuditLog -> ap ->
os.OpenFile) fails, the previously opened main log handle is leaked; modify the
error path in the block that calls os.OpenFile for the audit log to close the
already-opened main log file handle (call its Close() safely or defer closure
earlier) before returning the error so the main log descriptor is not leaked.
In `@internal/output/renderer.go`:
- Around line 76-79: The Render function currently ignores errors from
fmt.Fprintf/Fprintln when writing result footers and exec output (e.g., the
fmt.Fprintf call that prints "%d row%s in set" using res.Rows and plural(), and
the block that prints Time when r.showTimer is true, plus the exec output write
calls); modify these writes to capture their error returns and propagate them up
by returning the error from Render instead of discarding it. Locate the write
calls in Render (including uses of res.Duration, res.Rows, plural(), and any
exec output writers) and change each fmt.Fprintf/Fprintln to check the returned
error (err) and return err immediately if non-nil, ensuring all footer and
exec-output writes surface write failures to the caller.
In `@internal/output/vertical.go`:
- Around line 21-26: The fmt.Fprintf calls that write to w are ignoring returned
errors (seen around the loop writing the row header and each cell), so update
the writer logic in this function to capture and return write errors: check the
error returned by fmt.Fprintf for the header line and for each cell (the calls
using w, fmt.Fprintf(..., "*************************** %d. row ...", i+1) and
fmt.Fprintf(w, "%s%s: %s\n", pad, name, formatCell(val))), and if err != nil
return that error (or wrap it with context) instead of always returning nil;
ensure the function signature still returns error and propagate the first write
error encountered.
In `@internal/repl/history.go`:
- Line 39: The file is opened with world-readable permissions using
os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644); change the mode
to user-only (0o600) so the REPL history is readable/writable only by the owner.
Locate the os.OpenFile call in internal/repl/history.go (the line that sets f,
err := os.OpenFile(...)) and replace the file mode 0o644 with 0o600; if the code
elsewhere creates the file earlier, ensure any creation path also uses 0o600 (or
explicitly call os.Chmod(path, 0o600) after creation) to enforce user-only
access.
In `@internal/repl/repl.go`:
- Around line 29-36: Add an explicit nil check for the session DB in Run(ctx
context.Context, s *session.State): after validating s.Renderer and s.Specials,
verify s.DB != nil and return a clear error (e.g., errors.New("repl: state.DB
must be set")) so the REPL fails fast instead of panicking when s.DB is used for
title/prompt and execution; apply the same nil-check pattern wherever s.DB is
dereferenced in this file (same guard used around title/prompt setup and
execution code paths).
In `@internal/specials/backslash.go`:
- Around line 170-173: The code currently does unchecked concrete casts like
s.Specials.(*Registry) in handlers such as handleHelp (and the other handlers
around the same area), which can panic if session.State.Specials is a different
implementation; change these to a safe type assertion (r, ok :=
s.Specials.(*Registry>) and handle the !ok case) or preferably call the
session.SpecialsHandler interface methods directly if available; update
handleHelp to guard the cast before calling r.AllCommands() and return a clear
non-panicking error or fallback behavior when the assertion fails.
In `@internal/specials/favorites.go`:
- Around line 51-56: The Save method currently unlocks f.mu before calling
f.cfg.Save(), allowing interleaving; hold the mutex for the entire
"update+persist" critical section by acquiring f.mu.Lock() and deferring
f.mu.Unlock() so the unlock happens after f.cfg.Save() completes (i.e., move the
unlock to after persistence). Apply the same change to the other affected
method(s) in this file (the method around lines 59-64, e.g., Delete/Remove) so
they also perform mutation and cfg.Save() while the lock is held.
In `@internal/specials/turso.go`:
- Around line 28-33: The .push and .pull handlers currently call
s.DB.Push(context.Background()) and s.DB.Pull(context.Background()) which can
hang indefinitely; replace those calls in handlePush and handlePull with a
context created via context.WithTimeout (e.g., ctx, cancel :=
context.WithTimeout(context.Background(), <reasonableDuration>)) and defer
cancel(), then pass ctx to s.DB.Push and s.DB.Pull so the network syncs are
bounded by a timeout; ensure you import context if not present and choose an
appropriate timeout constant.
---
Minor comments:
In `@internal/db/db_test.go`:
- Line 13: Test cleanup currently ignores errors from conn.Close() in
db_test.go; update both deferred conn.Close() calls to check and fail the test
on error—e.g., replace defer conn.Close() with a deferred closure that captures
the return value and calls t.Fatalf/t.Errorf or require.NoError(t, err) using
the existing *testing.T variable to surface teardown failures (target the
conn.Close() calls in the tests).
In `@internal/db/integration_test.go`:
- Line 24: Replace the bare defer conn.Close() with a deferred closure that
checks and reports the error from conn.Close(); e.g. defer func() { if err :=
conn.Close(); err != nil { t.Fatalf("failed to close conn: %v", err) } }() so
teardown failures are surfaced (reference the conn.Close() call in
integration_test.go and use the test's t to report the error).
In `@internal/db/libsql.go`:
- Around line 40-52: In buildLibsqlDSN, normalize the incoming authToken by
calling strings.TrimSpace(authToken) into a local variable and use that trimmed
value when checking whether to append and when calling q.Set("authToken", ...);
this ensures whitespace-only/padded tokens from env/copy-paste are treated as
empty and prevents writing raw padded tokens into the DSN. Also use the trimmed
value when comparing existing query params if you want to avoid treating " auth
" as present.
In `@internal/specials/backslash.go`:
- Around line 185-189: The no-arg branch of handlePager currently flips the
pager on; change it to be read-only by removing the call to
s.Renderer.SetPagerEnabled(true) (or avoid mutating Renderer state) in the
len(args) == 0 branch so it only prints currentPager(s) and returns; locate
handlePager, the len(args) == 0 branch, currentPager(s) usage and
Renderer.SetPagerEnabled to make this change.
In `@internal/specials/registry.go`:
- Around line 82-90: Suggest currently matches prefixes case-sensitively while
Handle lowercases command names; update Registry.Suggest to perform
case-insensitive matching by comparing lowercased values (e.g., use
strings.ToLower on both prefix and each Command.Name from r.commands) and also
sort the results case-insensitively (compare strings.ToLower(out[i].Name) <
strings.ToLower(out[j].Name)) so suggestions align with the dispatch behavior.
---
Duplicate comments:
In @.goreleaser.yaml:
- Line 19: Update the ldflags entry that sets the Version to use the correct
repository import path: replace the incorrect package path
`github.com/peter/litegocli/internal/app.Version` with the actual repository
path `github.com/peteraba/litegocli/internal/app.Version` so the `-X
...Version={{.Version}}` flag injects the version correctly; ensure the same
change is mirrored wherever the old path is referenced (e.g., Makefile) and keep
the `Version` symbol name unchanged.
In `@README.md`:
- Line 29: Update all package paths and URLs that reference the wrong GitHub
owner from "github.com/peter/litegocli" to "github.com/peteraba/litegocli":
change the README installation command string "go install
github.com/peter/litegocli@latest" and the clone URL occurrences, and also
update matching entries in Makefile and .goreleaser.yaml so go install and clone
commands point to the correct repository path.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 18-19: The workflow uses semantic tags for GitHub Actions
(actions/checkout@v4 and actions/setup-go@v5) which can be subject to tag
manipulation; update those usages to pinned commit SHAs instead (replace the `@v4`
and `@v5` references with the corresponding commit SHA for the exact release) for
stronger supply-chain security, and do the same for the other occurrences
referenced in the comment (the other actions at lines shown).
In `@internal/repl/completer.go`:
- Around line 115-117: The regex in tokenize
(regexp.MustCompile(`[A-Za-z_][A-Za-z0-9_]*`)) is compiled on every completion
pass; move this pattern to a package-level variable (e.g., var identRE =
regexp.MustCompile(...)) and update tokenize to call identRE.FindAllString(text,
-1). Do the same for the other regex used around the 134-135 region (precompile
it as a package-level var and reuse it instead of calling regexp.MustCompile
inside the hot path).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ba7d539-0cd4-47c8-92ba-fb838f085e5f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (51)
.github/workflows/ci.yml.gitignore.goreleaser.yamlMakefileREADME.mdgo.modgoliteinternal/app/app.gointernal/app/app_test.gointernal/app/integration_test.gointernal/browser/browser.gointernal/config/config.gointernal/config/config_test.gointernal/config/paths.gointernal/db/backend.gointernal/db/config.gointernal/db/db.gointernal/db/db_test.gointernal/db/display.gointernal/db/integration_test.gointernal/db/libsql.gointernal/db/open.gointernal/db/open_test.gointernal/db/schema.gointernal/db/sqlite.gointernal/db/turso_sync.gointernal/highlight/highlight.gointernal/logging/logging.gointernal/output/csv.gointernal/output/json.gointernal/output/pager.gointernal/output/renderer.gointernal/output/renderer_test.gointernal/output/table.gointernal/output/tsv.gointernal/output/vertical.gointernal/repl/completer.gointernal/repl/completer_test.gointernal/repl/history.gointernal/repl/keywords.gointernal/repl/prompt.gointernal/repl/repl.gointernal/session/session.gointernal/specials/backslash.gointernal/specials/dot.gointernal/specials/favorites.gointernal/specials/favorites_test.gointernal/specials/registry.gointernal/specials/turso.gomain.gotestdata/sample.sql
| GOOS: ${{ matrix.goos }} | ||
| GOARCH: ${{ matrix.goarch }} | ||
| CGO_ENABLED: "0" | ||
| run: go build -trimpath -ldflags "-s -w" -o litegocli-${{ matrix.goos }}-${{ matrix.goarch }} . |
There was a problem hiding this comment.
Add version injection to match Makefile and goreleaser builds.
The CI build step omits the -X linker flag for version injection that both the Makefile (line 3) and .goreleaser.yaml (line 19) include. This causes CI-built binaries to lack version information.
📦 Proposed fix to add version injection
- run: go build -trimpath -ldflags "-s -w" -o litegocli-${{ matrix.goos }}-${{ matrix.goarch }} .
+ run: go build -trimpath -ldflags "-s -w -X github.com/peteraba/litegocli/internal/app.Version=${{ github.sha }}" -o litegocli-${{ matrix.goos }}-${{ matrix.goarch }} .Note: Using ${{ github.sha }} for CI builds. Alternatively, use a version like dev-${{ github.sha }} or derive from tags if building a release.
📝 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.
Suggested change
| run: go build -trimpath -ldflags "-s -w" -o litegocli-${{ matrix.goos }}-${{ matrix.goarch }} . | |
| run: go build -trimpath -ldflags "-s -w -X github.com/peteraba/litegocli/internal/app.Version=${{ github.sha }}" -o litegocli-${{ matrix.goos }}-${{ matrix.goarch }} . |
🤖 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 @.github/workflows/ci.yml at line 55, The CI go build step currently omits
the -X linker flag so CI-built binaries lack version info; update the run
command (`run: go build -trimpath -ldflags ... -o litegocli-${{ matrix.goos
}}-${{ matrix.goarch }} .`) to include an -X assignment that injects the build
version (use `${{ github.sha }}` or `dev-${{ github.sha }}`) into the same
package-level variable your Makefile/.goreleaser set (e.g., main.Version or the
project's version symbol) so the binary contains the version string.
| @@ -0,0 +1,57 @@ | |||
| module github.com/peter/litegocli | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify declared module path and current import usage.
sed -n '1,6p' go.mod
rg -n --type go 'github.com/peter/litegocli|github.com/peteraba/litegocli'Repository: peteraba/litegocli
Length of output: 3762
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check repository's declared origin (if present)
git remote -v 2>/dev/null || true
git config --get remote.origin.url 2>/dev/null || true
# See what the docs mention as the canonical module path
rg -n --hidden --no-ignore -S --glob 'README*' --glob '*.md' \
'github\.com/peter/litegocli|github\.com/peteraba/litegocli' || true
# Also search all non-vendored text files for the alternate path mentions
rg -n --hidden --no-ignore --glob '!*vendor/*' -S \
'github\.com/peteraba/litegocli' || trueRepository: peteraba/litegocli
Length of output: 1175
🌐 Web query:
github.com/peter/litegocli repository redirect mirror fork relationship with github.com/peteraba/litegocli
💡 Result:
There is no evidence of a repository named litegocli under either the github.com/peter or github.com/peteraba namespaces. Extensive searches of GitHub and general technical resources confirm that no such repository exists, nor is there any documented redirect, mirror, or fork relationship between those specific paths. It is likely that the query refers to one of several similarly named projects in the Go or CLI ecosystem, such as: - dbcli/litecli: A popular command-line client for SQLite databases [1]. - liteclaw/liteclaw: A Go-based personal AI assistant [2]. - Various other projects with "lite" or "cli" in their names, such as git-lite-cli [3] or lite-cc [4]. The user Peter Aba (@peteraba) is a developer with a presence on GitHub and DEV Community, but his public repositories do not include a project named litegocli [5][6][7].
Citations:
- 1: https://github.com/dbcli/litecli/
- 2: https://github.com/liteclaw/liteclaw
- 3: https://github.com/NikhilKatkuri/git-lite-cli
- 4: https://github.com/key4ng/lite-cc
- 5: https://github.com/peteraba/dotfiles
- 6: https://github.com/peteraba
- 7: https://dev.to/peteraba
Fix the go.mod module path to match the canonical GitHub location (or document the vanity/redirect).
go.mod declares module github.com/peter/litegocli, but this repo is hosted at github.com/peteraba/litegocli (and the README also instructs go install github.com/peter/litegocli@latest). Public web search finds no repository/redirect at github.com/peter/litegocli, so module consumers may fail to install or end up pulling different code. Update go.mod (and any -ldflags/import paths) to github.com/peteraba/litegocli, or explicitly document an existing vanity mapping for github.com/peter/litegocli.
🤖 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 `@go.mod` at line 1, The module path in go.mod is incorrect: change the module
declaration from "github.com/peter/litegocli" to the canonical
"github.com/peteraba/litegocli" and update any related references (search for
occurrences of "github.com/peter/litegocli" in the repo, including build flags
like -ldflags, import statements, and README install instructions) so all
module/import paths and documentation consistently use
"github.com/peteraba/litegocli" (or alternatively document a vanity/redirect if
you intend to keep the original string).
Comment on lines
+182
to
+201
| if handled, herr := s.Specials.Handle(stmt); handled { | ||
| if herr != nil { | ||
| fmt.Fprintln(s.Err, "error:", herr) | ||
| } | ||
| continue | ||
| } | ||
| s.Logger.Audit(stmt) | ||
| result, err := s.DB.Run(stmt) | ||
| if err != nil { | ||
| if errors.Is(err, db.ErrEmpty) { | ||
| continue | ||
| } | ||
| fmt.Fprintln(s.Err, "error:", err) | ||
| continue | ||
| } | ||
| if err := s.Renderer.Render(result); err != nil { | ||
| fmt.Fprintln(s.Err, "render error:", err) | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Return a non-nil error when non-interactive execution has failures.
In --execute/script mode, SQL/special/render failures are only printed (Lines 184/194/198) and runScript still returns nil, so the process can exit with status 0 on failure.
Proposed fix
func runScript(s *session.State, r io.Reader) error {
data, err := io.ReadAll(r)
if err != nil {
return err
}
+ var firstErr error
statements := splitStatements(string(data))
for _, stmt := range statements {
stmt = strings.TrimSpace(stmt)
if stmt == "" {
continue
}
if handled, herr := s.Specials.Handle(stmt); handled {
if herr != nil {
fmt.Fprintln(s.Err, "error:", herr)
+ if firstErr == nil {
+ firstErr = fmt.Errorf("special command failed: %w", herr)
+ }
}
continue
}
s.Logger.Audit(stmt)
result, err := s.DB.Run(stmt)
if err != nil {
if errors.Is(err, db.ErrEmpty) {
continue
}
fmt.Fprintln(s.Err, "error:", err)
+ if firstErr == nil {
+ firstErr = err
+ }
continue
}
if err := s.Renderer.Render(result); err != nil {
fmt.Fprintln(s.Err, "render error:", err)
+ if firstErr == nil {
+ firstErr = fmt.Errorf("render failed: %w", err)
+ }
}
}
- return nil
+ return firstErr
}🤖 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 `@internal/app/app.go` around lines 182 - 201, runScript currently swallows
errors (printing them) during script/--execute runs; update runScript so that
when running non-interactively (script/--execute mode) any failure from
Specials.Handle, s.DB.Run (except db.ErrEmpty) or s.Renderer.Render is returned
as a non-nil error instead of only logging to s.Err. Concretely, detect the
non-interactive/execute flag used by the caller (the runScript context), and on
handled-but-error from Specials.Handle, on err from s.DB.Run (unless
errors.Is(err, db.ErrEmpty)), and on err from s.Renderer.Render, return that
error (or aggregate and return a combined error) so the process exits non-zero;
retain printing for interactive mode using the same s.Err logging code.
Comment on lines
+214
to
+224
| case '\'', '"': | ||
| b.WriteByte(c) | ||
| i++ | ||
| for i < len(input) { | ||
| b.WriteByte(input[i]) | ||
| if input[i] == c { | ||
| i++ | ||
| break | ||
| } | ||
| i++ | ||
| } |
There was a problem hiding this comment.
Handle doubled-quote escapes in SQL string parsing.
The quote scanner breaks on the first matching quote, but SQL allows escaped quotes as doubled chars ('' / ""). This can incorrectly split statements containing semicolons inside such strings.
Proposed fix
case '\'', '"':
- b.WriteByte(c)
+ quote := c
+ b.WriteByte(quote)
i++
for i < len(input) {
b.WriteByte(input[i])
- if input[i] == c {
+ if input[i] == quote {
+ if i+1 < len(input) && input[i+1] == quote {
+ b.WriteByte(input[i+1])
+ i += 2
+ continue
+ }
i++
break
}
i++
}🤖 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 `@internal/app/app.go` around lines 214 - 224, The quote-scanning branch in
app.go (the loop that handles case '\'' and '"' using variables c, input, i, and
b) incorrectly treats the first matching quote as the end even when SQL uses
doubled quotes as escapes; update the inner loop to detect doubled-quote escapes
by checking if input[i] == c and the next character (input[i+1]) is also c, in
which case write both quote characters to b and advance i past both without
breaking, and only treat a single c not followed by the same quote as the string
terminator; ensure bounds checks when peeking input[i+1].
Comment on lines
+30
to
+31
| if mm, ok := final.(model); ok && mm.chosen { | ||
| return fmt.Sprintf("SELECT * FROM %s LIMIT 100;", mm.tables[mm.cursor].Name), nil |
There was a problem hiding this comment.
Fix selected-table lookup to use the filtered slice (current code can return the wrong table).
Line 31 indexes mm.tables using a cursor that is maintained against filtered() results. After filtering, Enter can generate SQL for a different table than the highlighted one.
Proposed fix
- if mm, ok := final.(model); ok && mm.chosen {
- return fmt.Sprintf("SELECT * FROM %s LIMIT 100;", mm.tables[mm.cursor].Name), nil
- }
+ if mm, ok := final.(model); ok && mm.chosen {
+ tables := mm.filtered()
+ if mm.cursor >= 0 && mm.cursor < len(tables) {
+ name := strings.ReplaceAll(tables[mm.cursor].Name, `"`, `""`)
+ return fmt.Sprintf(`SELECT * FROM "%s" LIMIT 100;`, name), nil
+ }
+ }📝 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.
Suggested change
| if mm, ok := final.(model); ok && mm.chosen { | |
| return fmt.Sprintf("SELECT * FROM %s LIMIT 100;", mm.tables[mm.cursor].Name), nil | |
| if mm, ok := final.(model); ok && mm.chosen { | |
| tables := mm.filtered() | |
| if mm.cursor >= 0 && mm.cursor < len(tables) { | |
| name := strings.ReplaceAll(tables[mm.cursor].Name, `"`, `""`) | |
| return fmt.Sprintf(`SELECT * FROM "%s" LIMIT 100;`, name), nil | |
| } | |
| } |
🤖 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 `@internal/browser/browser.go` around lines 30 - 31, The selected-table lookup
uses mm.tables with mm.cursor while the cursor is maintained against the
filtered set; update the Enter/selection path that builds SQL (the branch
testing final.(model) and mm.chosen) to index into the filtered slice (call the
filtered() method or equivalent on model) instead of mm.tables and ensure you
guard against out-of-range cursor (check length > cursor) before accessing Name
so the SQL always refers to the highlighted filtered table.
| if line == "" { | ||
| return | ||
| } | ||
| f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) |
There was a problem hiding this comment.
Restrict history file permissions to user-only access.
REPL history may contain secrets (tokens, literals, PII) from executed SQL. Creating it with 0o644 makes it readable by other local users.
Suggested fix
- f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644)
+ f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600)📝 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.
Suggested change
| f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) | |
| f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) |
🤖 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 `@internal/repl/history.go` at line 39, The file is opened with world-readable
permissions using os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644);
change the mode to user-only (0o600) so the REPL history is readable/writable
only by the owner. Locate the os.OpenFile call in internal/repl/history.go (the
line that sets f, err := os.OpenFile(...)) and replace the file mode 0o644 with
0o600; if the code elsewhere creates the file earlier, ensure any creation path
also uses 0o600 (or explicitly call os.Chmod(path, 0o600) after creation) to
enforce user-only access.
Comment on lines
+29
to
+36
| func Run(ctx context.Context, s *session.State) error { | ||
| if s.Renderer == nil { | ||
| return errors.New("repl: state.Renderer must be set") | ||
| } | ||
| reg, ok := s.Specials.(*specials.Registry) | ||
| if !ok { | ||
| return errors.New("repl: state.Specials must be a *specials.Registry") | ||
| } |
There was a problem hiding this comment.
Add an explicit nil check for state.DB before REPL setup.
The REPL uses s.DB immediately for title/prompt and later execution. A nil DB currently panics instead of failing fast with a clear error.
Suggested fix
func Run(ctx context.Context, s *session.State) error {
+ if s == nil {
+ return errors.New("repl: state must be set")
+ }
+ if s.DB == nil {
+ return errors.New("repl: state.DB must be set")
+ }
if s.Renderer == nil {
return errors.New("repl: state.Renderer must be set")
}Also applies to: 47-47, 72-72, 151-151
🤖 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 `@internal/repl/repl.go` around lines 29 - 36, Add an explicit nil check for
the session DB in Run(ctx context.Context, s *session.State): after validating
s.Renderer and s.Specials, verify s.DB != nil and return a clear error (e.g.,
errors.New("repl: state.DB must be set")) so the REPL fails fast instead of
panicking when s.DB is used for title/prompt and execution; apply the same
nil-check pattern wherever s.DB is dereferenced in this file (same guard used
around title/prompt setup and execution code paths).
Comment on lines
+170
to
+173
| func handleHelp(s *session.State, _ []string) error { | ||
| fmt.Fprintln(s.Out, "Special commands:") | ||
| for _, c := range s.Specials.(*Registry).AllCommands() { | ||
| fmt.Fprintf(s.Out, " %-22s %s\n", c.Usage, c.Description) |
There was a problem hiding this comment.
Guard s.Specials.(*Registry) assertions to avoid panic paths.
Line [172], Line [213], Line [246], and Line [260] use unchecked concrete casts. If session.State.Specials is replaced/mocked with another session.SpecialsHandler, these handlers panic and kill the REPL.
Suggested fix
+func registryFromState(s *session.State) (*Registry, error) {
+ reg, ok := s.Specials.(*Registry)
+ if !ok || reg == nil {
+ return nil, errors.New("specials registry unavailable")
+ }
+ return reg, nil
+}
+
func handleHelp(s *session.State, _ []string) error {
+ reg, err := registryFromState(s)
+ if err != nil {
+ return err
+ }
fmt.Fprintln(s.Out, "Special commands:")
- for _, c := range s.Specials.(*Registry).AllCommands() {
+ for _, c := range reg.AllCommands() {
fmt.Fprintf(s.Out, " %-22s %s\n", c.Usage, c.Description)
}
@@
func handleFavoriteRun(s *session.State, args []string) error {
- reg := s.Specials.(*Registry)
+ reg, err := registryFromState(s)
+ if err != nil {
+ return err
+ }
@@
func handleFavoriteSave(s *session.State, args []string) error {
@@
- reg := s.Specials.(*Registry)
+ reg, err := registryFromState(s)
+ if err != nil {
+ return err
+ }
@@
func handleFavoriteDelete(s *session.State, args []string) error {
@@
- reg := s.Specials.(*Registry)
+ reg, err := registryFromState(s)
+ if err != nil {
+ return err
+ }Also applies to: 212-214, 246-247, 260-260
🤖 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 `@internal/specials/backslash.go` around lines 170 - 173, The code currently
does unchecked concrete casts like s.Specials.(*Registry) in handlers such as
handleHelp (and the other handlers around the same area), which can panic if
session.State.Specials is a different implementation; change these to a safe
type assertion (r, ok := s.Specials.(*Registry>) and handle the !ok case) or
preferably call the session.SpecialsHandler interface methods directly if
available; update handleHelp to guard the cast before calling r.AllCommands()
and return a clear non-panicking error or fallback behavior when the assertion
fails.
Comment on lines
+51
to
+56
| func (f *FavoriteStore) Save(name, query string) error { | ||
| f.mu.Lock() | ||
| f.cfg.Favorites[name] = query | ||
| f.mu.Unlock() | ||
| return f.cfg.Save() | ||
| } |
There was a problem hiding this comment.
Keep mutation and persistence in one critical section.
Line [54] and Line [62] release FavoriteStore.mu before config.Config.Save(). That permits interleaving updates during persistence and breaks atomic “update+persist” behavior under concurrency.
Suggested fix
func (f *FavoriteStore) Save(name, query string) error {
- f.mu.Lock()
- f.cfg.Favorites[name] = query
- f.mu.Unlock()
- return f.cfg.Save()
+ f.mu.Lock()
+ defer f.mu.Unlock()
+ f.cfg.Favorites[name] = query
+ return f.cfg.Save()
}
@@
func (f *FavoriteStore) Delete(name string) error {
- f.mu.Lock()
- delete(f.cfg.Favorites, name)
- f.mu.Unlock()
- return f.cfg.Save()
+ f.mu.Lock()
+ defer f.mu.Unlock()
+ delete(f.cfg.Favorites, name)
+ return f.cfg.Save()
}Also applies to: 59-64
🤖 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 `@internal/specials/favorites.go` around lines 51 - 56, The Save method
currently unlocks f.mu before calling f.cfg.Save(), allowing interleaving; hold
the mutex for the entire "update+persist" critical section by acquiring
f.mu.Lock() and deferring f.mu.Unlock() so the unlock happens after f.cfg.Save()
completes (i.e., move the unlock to after persistence). Apply the same change to
the other affected method(s) in this file (the method around lines 59-64, e.g.,
Delete/Remove) so they also perform mutation and cfg.Save() while the lock is
held.
Comment on lines
+28
to
+33
| func handlePush(s *session.State, _ []string) error { | ||
| if !s.DB.Backend.SupportsSync() { | ||
| return fmt.Errorf(".push requires --sync mode") | ||
| } | ||
| if err := s.DB.Push(context.Background()); err != nil { | ||
| return err |
There was a problem hiding this comment.
Add a bounded timeout for .push/.pull network sync calls.
Line [32] and Line [43] use context.Background(), so a hung remote call can block the REPL indefinitely.
Suggested fix
import (
"context"
"fmt"
+ "time"
@@
func handlePush(s *session.State, _ []string) error {
@@
- if err := s.DB.Push(context.Background()); err != nil {
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
+ if err := s.DB.Push(ctx); err != nil {
return err
}
@@
func handlePull(s *session.State, _ []string) error {
@@
- changed, err := s.DB.Pull(context.Background())
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
+ changed, err := s.DB.Pull(ctx)
if err != nil {
return err
}Also applies to: 39-44
🤖 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 `@internal/specials/turso.go` around lines 28 - 33, The .push and .pull
handlers currently call s.DB.Push(context.Background()) and
s.DB.Pull(context.Background()) which can hang indefinitely; replace those calls
in handlePush and handlePull with a context created via context.WithTimeout
(e.g., ctx, cancel := context.WithTimeout(context.Background(),
<reasonableDuration>)) and defer cancel(), then pass ctx to s.DB.Push and
s.DB.Pull so the network syncs are bounded by a timeout; ensure you import
context if not present and choose an appropriate timeout constant.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation