Address various minor issues

LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)

Author: shrsv

internal/reviewquery/aliases.go

@@ -2,6 +2,7 @@ package reviewquery
 
 import (
 	"fmt"
+	"maps"
 	"os"
 	"path/filepath"
 	"sort"
@@ -19,7 +20,7 @@ import (
 // the installer writes ~/.lrc/queries.toml. User-defined aliases override these.
 func builtinAliases() map[string]string {
 	return map[string]string{
-		"stats":     "SELECT action AS Action, COUNT(*) AS Commits, ROUND(AVG(iterations),1) AS AvgIter, ROUND(AVG(coverage)) AS AvgCoveragePct FROM review_log GROUP BY action ORDER BY Commits DESC",
+		"stats":     "SELECT action AS Action, COUNT(*) AS Commits, ROUND(AVG(iterations),1) AS AvgIter, ROUND(AVG(coverage),1) AS AvgCoveragePct FROM review_log GROUP BY action ORDER BY Commits DESC",
 		"by-author": "SELECT author AS Author, COUNT(*) AS Commits, SUM(action = 'reviewed') AS Reviewed FROM review_log GROUP BY author ORDER BY Commits DESC",
 		"recent":    "SELECT short_hash AS Hash, date AS Date, action AS Action, subject AS Subject FROM review_log ORDER BY date DESC LIMIT 20",
 	}
@@ -52,17 +53,20 @@ func loadUserAliases() (map[string]string, error) {
 		if os.IsNotExist(err) {
 			return map[string]string{}, nil
 		}
-		return nil, fmt.Errorf("failed to access %s: %w", path, err)
+		return nil, fmt.Errorf("failed to access user aliases file %s: %w", path, err)
 	}
 
 	k := koanf.New(".")
 	if err := k.Load(file.Provider(path), toml.Parser()); err != nil {
-		return nil, fmt.Errorf("failed to parse %s: %w", path, err)
+		return nil, fmt.Errorf("failed to parse user aliases file %s: %w", path, err)
 	}
-	out := map[string]string{}
-	for name, val := range k.StringMap("queries") {
-		out[name] = val
+	// A non-empty file that lacks the [queries] table entirely is malformed —
+	// surface that instead of silently loading zero aliases.
+	if len(k.Keys()) > 0 && !k.Exists("queries") {
+		return nil, fmt.Errorf("user aliases file %s has no [queries] table", path)
 	}
+	out := map[string]string{}
+	maps.Copy(out, k.StringMap("queries"))
 	return out, nil
 }
 
@@ -119,6 +123,9 @@ func AddAlias(name, sql string) error {
 	if strings.TrimSpace(sql) == "" {
 		return fmt.Errorf("alias SQL cannot be empty")
 	}
+	if err := validateReadOnlySQL(sql); err != nil {
+		return fmt.Errorf("alias SQL rejected: %w", err)
+	}
 	user, err := loadUserAliases()
 	if err != nil {
 		return err

internal/reviewquery/command.go

@@ -10,11 +10,15 @@ import (
 // RunQuery is the default action for `lrc query`. It either saves an alias
 // (--add/--name) or runs a saved alias / raw SQL and prints a table or JSON.
 func RunQuery(c *cli.Context) error {
-	if add := strings.TrimSpace(c.String("add")); add != "" {
+	if c.IsSet("add") {
+		add := strings.TrimSpace(c.String("add"))
 		name := strings.TrimSpace(c.String("name"))
-		if name == "" {
+		if !c.IsSet("name") || name == "" {
 			return fmt.Errorf("--add requires --name")
 		}
+		if add == "" {
+			return fmt.Errorf("--add requires non-empty SQL")
+		}
 		if err := AddAlias(name, add); err != nil {
 			return err
 		}
@@ -28,7 +32,10 @@ func RunQuery(c *cli.Context) error {
 
 	// urfave/cli stops parsing flags at the first positional arg, so also scan
 	// the remaining args for trailing flags (e.g. `lrc query stats --from 2024-01-01`).
-	positionals := parseTrailingFlags(c.Args().Slice(), &jsonOut, &filter)
+	positionals, err := parseTrailingFlags(c.Args().Slice(), &jsonOut, &filter)
+	if err != nil {
+		return err
+	}
 
 	arg := "stats" // default alias
 	if len(positionals) > 0 && strings.TrimSpace(positionals[0]) != "" {
@@ -64,45 +71,50 @@ func RunQuery(c *cli.Context) error {
 // parseTrailingFlags pulls flags out of args that cli left unparsed (anything
 // after the first positional). Supports `--flag value` and `--flag=value`.
 // Returns the remaining positional args; sets jsonOut/filter via pointers.
-func parseTrailingFlags(args []string, jsonOut *bool, filter *Filter) []string {
+// Returns an error if a bound flag (--from/--to/--range) is the last arg with
+// no value following it, rather than silently swallowing the flag name into
+// the positionals (where it would end up mangling the SQL/alias lookup).
+func parseTrailingFlags(args []string, jsonOut *bool, filter *Filter) ([]string, error) {
+	boundFlags := []struct {
+		name string
+		dest *string
+	}{
+		{"--from", &filter.From},
+		{"--to", &filter.To},
+		{"--range", &filter.Range},
+	}
+
 	positionals := make([]string, 0, len(args))
 	for i := 0; i < len(args); i++ {
 		a := args[i]
-		// flag=value form
-		switch {
-		case a == "--json" || a == "-j":
+		if a == "--json" || a == "-j" {
 			*jsonOut = true
 			continue
-		case strings.HasPrefix(a, "--from="):
-			filter.From = strings.TrimPrefix(a, "--from=")
-			continue
-		case strings.HasPrefix(a, "--to="):
-			filter.To = strings.TrimPrefix(a, "--to=")
-			continue
-		case strings.HasPrefix(a, "--range="):
-			filter.Range = strings.TrimPrefix(a, "--range=")
-			continue
 		}
-		// flag value form (consume next arg)
-		if i+1 < len(args) {
-			switch a {
-			case "--from":
-				filter.From = args[i+1]
-				i++
-				continue
-			case "--to":
-				filter.To = args[i+1]
-				i++
-				continue
-			case "--range":
-				filter.Range = args[i+1]
+
+		consumed := false
+		for _, bf := range boundFlags {
+			if val, ok := strings.CutPrefix(a, bf.name+"="); ok {
+				*bf.dest = val
+				consumed = true
+				break
+			}
+			if a == bf.name {
+				if i+1 >= len(args) {
+					return nil, fmt.Errorf("%s requires a value", bf.name)
+				}
+				*bf.dest = args[i+1]
 				i++
-				continue
+				consumed = true
+				break
 			}
 		}
+		if consumed {
+			continue
+		}
 		positionals = append(positionals, a)
 	}
-	return positionals
+	return positionals, nil
 }
 
 // RunQueryList prints every alias and its source.

internal/reviewquery/command_test.go

@@ -23,7 +23,10 @@ func TestParseTrailingFlags(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			jsonOut := false
 			f := Filter{}
-			pos := parseTrailingFlags(tc.args, &jsonOut, &f)
+			pos, err := parseTrailingFlags(tc.args, &jsonOut, &f)
+			if err != nil {
+				t.Fatalf("parseTrailingFlags error: %v", err)
+			}
 			if len(pos) != len(tc.wantPos) || (len(pos) > 0 && pos[0] != tc.wantPos[0]) {
 				t.Errorf("positionals = %v; want %v", pos, tc.wantPos)
 			}
@@ -34,3 +37,16 @@ func TestParseTrailingFlags(t *testing.T) {
 		})
 	}
 }
+
+func TestParseTrailingFlagsMissingValue(t *testing.T) {
+	for _, flag := range []string{"--from", "--to", "--range"} {
+		t.Run(flag, func(t *testing.T) {
+			jsonOut := false
+			f := Filter{}
+			_, err := parseTrailingFlags([]string{"stats", flag}, &jsonOut, &f)
+			if err == nil {
+				t.Fatalf("expected error when %s has no value, got nil", flag)
+			}
+		})
+	}
+}

internal/reviewquery/engine.go

@@ -2,6 +2,8 @@ package reviewquery
 
 import (
 	"fmt"
+	"log"
+	"strings"
 
 	"github.com/HexmosTech/git-lrc/storage"
 )
@@ -33,7 +35,9 @@ VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?);`
 
 // Run extracts the review history (scoped by filter), loads it into an in-memory
 // SQLite table named review_log, and runs sqlText against it. sqlText must
-// already be resolved (alias -> SQL) by the caller.
+// already be resolved (alias -> SQL) by the caller, but may otherwise be raw
+// text typed by the user (an unresolved alias name is treated as ad-hoc SQL) —
+// RunOnRecords enforces that it's a single read-only statement before running it.
 func Run(f Filter, sqlText string) (QueryResult, error) {
 	records, err := Extract(f)
 	if err != nil {
@@ -42,14 +46,43 @@ func Run(f Filter, sqlText string) (QueryResult, error) {
 	return RunOnRecords(records, sqlText)
 }
 
+// validateReadOnlySQL guards the in-memory review_log database against
+// anything but a single read-only SELECT/WITH statement. sqlText can come
+// straight from a user's shell (unresolved alias -> raw positional args) or
+// from a saved alias, so this rejects stacked statements (which could smuggle
+// a DROP/ATTACH/PRAGMA in after a semicolon) and any non-SELECT statement type.
+func validateReadOnlySQL(sqlText string) error {
+	stmt := strings.TrimSpace(sqlText)
+	if stmt == "" {
+		return fmt.Errorf("query is empty")
+	}
+	stmt = strings.TrimSpace(strings.TrimSuffix(stmt, ";"))
+	if strings.Contains(stmt, ";") {
+		return fmt.Errorf("only a single SQL statement is allowed")
+	}
+	upper := strings.ToUpper(stmt)
+	if !strings.HasPrefix(upper, "SELECT") && !strings.HasPrefix(upper, "WITH") {
+		return fmt.Errorf("only read-only SELECT queries are allowed")
+	}
+	return nil
+}
+
 // RunOnRecords loads records into an in-memory review_log table and runs sqlText
 // against it. Split out from Run so it can be tested/benchmarked without git.
 func RunOnRecords(records []ReviewRecord, sqlText string) (QueryResult, error) {
+	if err := validateReadOnlySQL(sqlText); err != nil {
+		return QueryResult{}, err
+	}
+
 	db, err := storage.OpenInMemorySQLite()
 	if err != nil {
 		return QueryResult{}, err
 	}
-	defer func() { _ = db.Close() }()
+	defer func() {
+		if cerr := db.Close(); cerr != nil {
+			log.Printf("reviewquery: failed to close in-memory sqlite db: %v", cerr)
+		}
+	}()
 
 	if _, err := storage.ExecSQL(db, createTableSQL); err != nil {
 		return QueryResult{}, fmt.Errorf("failed to create review_log table: %w", err)

internal/reviewquery/extract.go

@@ -2,6 +2,7 @@ package reviewquery
 
 import (
 	"fmt"
+	"log"
 	"os/exec"
 	"regexp"
 	"strconv"
@@ -67,9 +68,11 @@ func parseRecord(raw, branch string) (ReviewRecord, bool) {
 	}
 	if t, err := time.Parse(time.RFC3339, strings.TrimSpace(parts[4])); err == nil {
 		rec.Date = t
+	} else {
+		log.Printf("reviewquery: failed to parse commit date %q for %s: %v", parts[4], rec.ShortHash, err)
 	}
 	body := parts[6]
-	for _, line := range strings.Split(body, "\n") {
+	for line := range strings.SplitSeq(body, "\n") {
 		if action, iter, cov, ok := parseTrailer(line); ok {
 			rec.Action = action
 			rec.Iterations = iter
@@ -84,6 +87,7 @@ func parseRecord(raw, branch string) (ReviewRecord, bool) {
 func currentBranch() string {
 	out, err := exec.Command("git", "rev-parse", "--abbrev-ref", "HEAD").Output()
 	if err != nil {
+		log.Printf("reviewquery: failed to determine current branch: %v", err)
 		return ""
 	}
 	return strings.TrimSpace(string(out))
@@ -104,7 +108,10 @@ func Extract(f Filter) ([]ReviewRecord, error) {
 		args = append(args, "--until="+f.To)
 	}
 	if f.Range != "" {
-		args = append(args, f.Range)
+		// --end-of-options stops git from treating f.Range as an option if it
+		// happens to start with '-' (e.g. a crafted --range value); the rest of
+		// the args up to "--" are already our own well-formed --flag=value pairs.
+		args = append(args, "--end-of-options", f.Range)
 	}
 	if f.PathPrefix != "" {
 		args = append(args, "--", f.PathPrefix)

internal/reviewquery/format.go

@@ -52,6 +52,12 @@ func FormatTable(r QueryResult) string {
 
 // FormatJSON renders a QueryResult as a JSON array of row objects, preserving
 // column order. All values are strings (the engine stringifies cells).
+//
+// This builds the object syntax by hand rather than json.Marshal-ing a
+// map[string]string per row: encoding/json has no way to preserve key order
+// for a Go map (it always sorts map keys alphabetically), and column order is
+// part of this format's contract. Each key/value is still run through
+// json.Marshal so escaping stays correct.
 func FormatJSON(r QueryResult) (string, error) {
 	var b strings.Builder
 	b.WriteString("[")

scripts/lrc-install.ps1

@@ -656,7 +656,7 @@ if (-not (Test-Path $LRC_QUERIES_FILE)) {
 # Add your own with:                lrc query --add "<sql>" --name "<name>"
 # Table columns: hash, short_hash, author, email, date, branch, subject, action, iterations, coverage
 [queries]
-stats = "SELECT action AS Action, COUNT(*) AS Commits, ROUND(AVG(iterations),1) AS AvgIter, ROUND(AVG(coverage)) AS AvgCoveragePct FROM review_log GROUP BY action ORDER BY Commits DESC"
+stats = "SELECT action AS Action, COUNT(*) AS Commits, ROUND(AVG(iterations),1) AS AvgIter, ROUND(AVG(coverage),1) AS AvgCoveragePct FROM review_log GROUP BY action ORDER BY Commits DESC"
 by-author = "SELECT author AS Author, COUNT(*) AS Commits, SUM(action = 'reviewed') AS Reviewed FROM review_log GROUP BY author ORDER BY Commits DESC"
 recent = "SELECT short_hash AS Hash, date AS Date, action AS Action, subject AS Subject FROM review_log ORDER BY date DESC LIMIT 20"
 '@ | Set-Content -Path $LRC_QUERIES_FILE -Encoding UTF8

scripts/lrc-install.sh

@@ -537,7 +537,7 @@ if [ ! -f "$LRC_QUERIES_FILE" ]; then
 # Add your own with:                lrc query --add "<sql>" --name "<name>"
 # Table columns: hash, short_hash, author, email, date, branch, subject, action, iterations, coverage
 [queries]
-stats = "SELECT action AS Action, COUNT(*) AS Commits, ROUND(AVG(iterations),1) AS AvgIter, ROUND(AVG(coverage)) AS AvgCoveragePct FROM review_log GROUP BY action ORDER BY Commits DESC"
+stats = "SELECT action AS Action, COUNT(*) AS Commits, ROUND(AVG(iterations),1) AS AvgIter, ROUND(AVG(coverage),1) AS AvgCoveragePct FROM review_log GROUP BY action ORDER BY Commits DESC"
 by-author = "SELECT author AS Author, COUNT(*) AS Commits, SUM(action = 'reviewed') AS Reviewed FROM review_log GROUP BY author ORDER BY Commits DESC"
 recent = "SELECT short_hash AS Hash, date AS Date, action AS Action, subject AS Subject FROM review_log ORDER BY date DESC LIMIT 20"
 QUERIESEOF

storage/sqlite_query_io.go

@@ -24,6 +24,9 @@ func OpenInMemorySQLite() (*sql.DB, error) {
 
 // BulkInsert inserts many rows under a single transaction with a prepared
 // statement — far faster than autocommitting each row (matters for large repos).
+// query is executed as-is with no restriction on statement type, so callers
+// must only pass trusted, internally-constructed SQL (e.g. a fixed INSERT
+// template) — never untrusted/user-supplied text.
 func BulkInsert(db *sql.DB, query string, rows [][]any) error {
 	if db == nil {
 		return fmt.Errorf("failed bulk insert: nil database handle")
@@ -51,9 +54,12 @@ func BulkInsert(db *sql.DB, query string, rows [][]any) error {
 	return nil
 }
 
-// QueryRows runs a read-only query and returns the column names plus each row
+// QueryRows runs a query and returns the column names plus each row
 // stringified (NULL -> ""). Keeping database/sql access inside the storage
 // boundary lets callers render results without importing database/sql.
+// This package does not itself enforce that query is read-only or a single
+// statement — when query may originate from user input (as it does for the
+// reviewquery engine), the caller is responsible for validating it first.
 func QueryRows(db *sql.DB, query string, args ...any) (columns []string, rows [][]string, err error) {
 	if db == nil {
 		return nil, nil, fmt.Errorf("failed SQL query: nil database handle")