From 029ea2f9ca92a65af4f8dc063e64bbec3a3e062b Mon Sep 17 00:00:00 2001 From: Vignesh Goud Date: Thu, 18 Jun 2026 19:49:13 +0530 Subject: [PATCH 1/6] feat: add review-history query engine (lrc query) (#60) Adds 'lrc query', a read-only engine that builds an in-memory SQLite table of the repo's review history (parsed from commit trailers) and runs SQL or saved aliases against it. - lrc query [stats]: default summary (reviewed/vouched/skipped) - lrc query "": arbitrary SQL over the review_log table - lrc query [--json]: run a saved alias; table or JSON output - lrc query --add "" --name : save an alias to ~/.lrc/queries.toml - lrc query list | view | delete Pure-Go SQLite (modernc.org/sqlite), no CGO/DuckDB dependency. All file and SQL I/O goes through storage/ so the architecture boundary test passes. Default aliases shipped via the installer scripts. Unit tests cover trailer parsing, record extraction, and output formatting. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%) --- cmd/app.go | 90 ++++++++++---- internal/reviewquery/aliases.go | 169 +++++++++++++++++++++++++++ internal/reviewquery/command.go | 113 ++++++++++++++++++ internal/reviewquery/engine.go | 71 +++++++++++ internal/reviewquery/extract.go | 135 +++++++++++++++++++++ internal/reviewquery/extract_test.go | 84 +++++++++++++ internal/reviewquery/format.go | 87 ++++++++++++++ internal/reviewquery/format_test.go | 46 ++++++++ internal/reviewquery/model.go | 39 +++++++ main.go | 5 + scripts/lrc-install.ps1 | 17 +++ scripts/lrc-install.sh | 15 +++ storage/sqlite_query_io.go | 65 +++++++++++ 13 files changed, 912 insertions(+), 24 deletions(-) create mode 100644 internal/reviewquery/aliases.go create mode 100644 internal/reviewquery/command.go create mode 100644 internal/reviewquery/engine.go create mode 100644 internal/reviewquery/extract.go create mode 100644 internal/reviewquery/extract_test.go create mode 100644 internal/reviewquery/format.go create mode 100644 internal/reviewquery/format_test.go create mode 100644 internal/reviewquery/model.go create mode 100644 storage/sqlite_query_io.go diff --git a/cmd/app.go b/cmd/app.go index c0e5d5a..a5c8a6b 100644 --- a/cmd/app.go +++ b/cmd/app.go @@ -36,31 +36,35 @@ do not write a commit attestation or offer to commit/push.` // Handlers contains injected command actions so CLI wiring can live outside main. type Handlers struct { - RunReviewSimple cli.ActionFunc - RunReviewDebug cli.ActionFunc - RunEnsure cli.ActionFunc - RunUninstall cli.ActionFunc - RunHooksInstall cli.ActionFunc - RunHooksUninstall cli.ActionFunc - RunHooksEnable cli.ActionFunc - RunHooksDisable cli.ActionFunc - RunHooksStatus cli.ActionFunc - RunSelfUpdate cli.ActionFunc - RunReviewCleanup cli.ActionFunc - RunAttestationTrailer cli.ActionFunc - RunSetup cli.ActionFunc - RunUI cli.ActionFunc - RunUsageInspect cli.ActionFunc - RunInternalClaudePreToolUse cli.ActionFunc - RunInternalClaudeRunCommit cli.ActionFunc - RunInternalClaudeSetupStart cli.ActionFunc - RunInternalClaudeSetupWorker cli.ActionFunc + RunReviewSimple cli.ActionFunc + RunReviewDebug cli.ActionFunc + RunEnsure cli.ActionFunc + RunUninstall cli.ActionFunc + RunHooksInstall cli.ActionFunc + RunHooksUninstall cli.ActionFunc + RunHooksEnable cli.ActionFunc + RunHooksDisable cli.ActionFunc + RunHooksStatus cli.ActionFunc + RunSelfUpdate cli.ActionFunc + RunReviewCleanup cli.ActionFunc + RunAttestationTrailer cli.ActionFunc + RunSetup cli.ActionFunc + RunUI cli.ActionFunc + RunUsageInspect cli.ActionFunc + RunInternalClaudePreToolUse cli.ActionFunc + RunInternalClaudeRunCommit cli.ActionFunc + RunInternalClaudeSetupStart cli.ActionFunc + RunInternalClaudeSetupWorker cli.ActionFunc RunInternalClaudeSetupSubmitKey cli.ActionFunc - RunInternalClaudeSetupStatus cli.ActionFunc - RunRemoveAttestation cli.ActionFunc - RunConfigInit cli.ActionFunc - RunConfigCheck cli.ActionFunc - RunConfigPreview cli.ActionFunc + RunInternalClaudeSetupStatus cli.ActionFunc + RunRemoveAttestation cli.ActionFunc + RunConfigInit cli.ActionFunc + RunConfigCheck cli.ActionFunc + RunConfigPreview cli.ActionFunc + RunQuery cli.ActionFunc + RunQueryList cli.ActionFunc + RunQueryView cli.ActionFunc + RunQueryDelete cli.ActionFunc } // BuildApp constructs the full CLI app with all command wiring. @@ -347,6 +351,44 @@ func BuildApp(version, buildTime, gitCommit, reviewMode string, baseFlags, debug }, }, }, + { + Name: "query", + Usage: "Query LiveReview history with SQL or a saved alias (e.g. 'lrc query stats')", + Description: `Runs a SQL query (or a saved alias) against an in-memory table of this +repo's review history, built from commit trailers. + + lrc query # default 'stats' alias + lrc query stats --json # same data as JSON + lrc query "SELECT author, COUNT(*) FROM review_log GROUP BY author" + lrc query --add "SELECT ..." --name myreport # save an alias + lrc query myreport # run the saved alias + +Columns in review_log: hash, short_hash, author, email, date, branch, +subject, action, iterations, coverage.`, + Flags: []cli.Flag{ + &cli.BoolFlag{Name: "json", Usage: "output machine-readable JSON"}, + &cli.StringFlag{Name: "add", Usage: "save the given SQL as an alias (requires --name)"}, + &cli.StringFlag{Name: "name", Usage: "alias name to save with --add"}, + }, + Action: h.RunQuery, + Subcommands: []*cli.Command{ + { + Name: "list", + Usage: "List saved and built-in query aliases", + Action: h.RunQueryList, + }, + { + Name: "view", + Usage: "Print the SQL behind an alias", + Action: h.RunQueryView, + }, + { + Name: "delete", + Usage: "Delete a saved alias", + Action: h.RunQueryDelete, + }, + }, + }, { Name: "internal", Usage: "Internal back-office commands (not for direct use)", diff --git a/internal/reviewquery/aliases.go b/internal/reviewquery/aliases.go new file mode 100644 index 0000000..b4ad5f7 --- /dev/null +++ b/internal/reviewquery/aliases.go @@ -0,0 +1,169 @@ +package reviewquery + +import ( + "fmt" + "os" + "path/filepath" + "sort" + "strconv" + "strings" + + "github.com/HexmosTech/git-lrc/configpath" + "github.com/HexmosTech/git-lrc/storage" + "github.com/knadh/koanf/parsers/toml" + "github.com/knadh/koanf/providers/file" + "github.com/knadh/koanf/v2" +) + +// builtinAliases ship with the binary so `lrc query ` works even before +// 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", + "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", + } +} + +// AliasInfo describes one alias and where it came from. +type AliasInfo struct { + Name string + SQL string + Source string // "built-in" or "user" +} + +// queriesPath returns ~/.lrc/queries.toml. +func queriesPath() (string, error) { + dir, err := configpath.ResolveLRCDataDir() + if err != nil { + return "", err + } + return filepath.Join(dir, "queries.toml"), nil +} + +// loadUserAliases reads ~/.lrc/queries.toml ([queries] table). Missing file is +// not an error — it returns an empty map. +func loadUserAliases() (map[string]string, error) { + path, err := queriesPath() + if err != nil { + return nil, err + } + if _, err := os.Stat(path); err != nil { + if os.IsNotExist(err) { + return map[string]string{}, nil + } + return nil, fmt.Errorf("failed to access %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) + } + out := map[string]string{} + for name, val := range k.StringMap("queries") { + out[name] = val + } + return out, nil +} + +// ResolveAlias returns the SQL for an alias name (user file wins over built-in). +func ResolveAlias(name string) (string, bool, error) { + user, err := loadUserAliases() + if err != nil { + return "", false, err + } + if sql, ok := user[name]; ok { + return sql, true, nil + } + if sql, ok := builtinAliases()[name]; ok { + return sql, true, nil + } + return "", false, nil +} + +// ListAliases returns every alias (built-in + user) sorted by name; a user +// alias shadows a built-in of the same name. +func ListAliases() ([]AliasInfo, error) { + user, err := loadUserAliases() + if err != nil { + return nil, err + } + merged := map[string]AliasInfo{} + for name, sql := range builtinAliases() { + merged[name] = AliasInfo{Name: name, SQL: sql, Source: "built-in"} + } + for name, sql := range user { + merged[name] = AliasInfo{Name: name, SQL: sql, Source: "user"} + } + names := make([]string, 0, len(merged)) + for n := range merged { + names = append(names, n) + } + sort.Strings(names) + out := make([]AliasInfo, 0, len(names)) + for _, n := range names { + out = append(out, merged[n]) + } + return out, nil +} + +// AddAlias saves (or overwrites) a user alias in ~/.lrc/queries.toml. +func AddAlias(name, sql string) error { + name = strings.TrimSpace(name) + if name == "" { + return fmt.Errorf("alias name cannot be empty") + } + if strings.ContainsAny(name, ". \t") { + return fmt.Errorf("alias name %q may not contain spaces or dots", name) + } + if strings.TrimSpace(sql) == "" { + return fmt.Errorf("alias SQL cannot be empty") + } + user, err := loadUserAliases() + if err != nil { + return err + } + user[name] = sql + return writeUserAliases(user) +} + +// DeleteAlias removes a user alias. Built-in aliases cannot be deleted. +func DeleteAlias(name string) error { + user, err := loadUserAliases() + if err != nil { + return err + } + if _, ok := user[name]; !ok { + if _, isBuiltin := builtinAliases()[name]; isBuiltin { + return fmt.Errorf("%q is a built-in alias and cannot be deleted", name) + } + return fmt.Errorf("no user alias named %q", name) + } + delete(user, name) + return writeUserAliases(user) +} + +// writeUserAliases serializes the alias map to ~/.lrc/queries.toml atomically. +func writeUserAliases(aliases map[string]string) error { + path, err := queriesPath() + if err != nil { + return err + } + + names := make([]string, 0, len(aliases)) + for n := range aliases { + names = append(names, n) + } + sort.Strings(names) + + var b strings.Builder + b.WriteString("# git-lrc saved queries. Managed by `lrc query --add/--delete`.\n") + b.WriteString("[queries]\n") + for _, n := range names { + b.WriteString(n) + b.WriteString(" = ") + b.WriteString(strconv.Quote(aliases[n])) + b.WriteString("\n") + } + return storage.WriteFileAtomically(path, []byte(b.String()), 0o644) +} diff --git a/internal/reviewquery/command.go b/internal/reviewquery/command.go new file mode 100644 index 0000000..6558087 --- /dev/null +++ b/internal/reviewquery/command.go @@ -0,0 +1,113 @@ +package reviewquery + +import ( + "fmt" + "strings" + + "github.com/urfave/cli/v2" +) + +// 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 != "" { + name := strings.TrimSpace(c.String("name")) + if name == "" { + return fmt.Errorf("--add requires --name") + } + if err := AddAlias(name, add); err != nil { + return err + } + fmt.Printf("Saved alias %q.\n", name) + return nil + } + + // urfave/cli stops parsing flags at the first positional arg, so support a + // trailing --json too (e.g. `lrc query stats --json`). + jsonOut := c.Bool("json") + positionals := make([]string, 0, c.NArg()) + for _, a := range c.Args().Slice() { + switch a { + case "--json", "-json", "-j": + jsonOut = true + default: + positionals = append(positionals, a) + } + } + + arg := "stats" // default alias + if len(positionals) > 0 && strings.TrimSpace(positionals[0]) != "" { + arg = strings.TrimSpace(positionals[0]) + } + + sqlText, found, err := ResolveAlias(arg) + if err != nil { + return err + } + if !found { + // Not a known alias — treat the positional args as raw SQL. + sqlText = strings.Join(positionals, " ") + } + + res, err := Run(Filter{}, sqlText) + if err != nil { + return err + } + + if jsonOut { + out, err := FormatJSON(res) + if err != nil { + return err + } + fmt.Println(out) + return nil + } + fmt.Print(FormatTable(res)) + return nil +} + +// RunQueryList prints every alias and its source. +func RunQueryList(c *cli.Context) error { + aliases, err := ListAliases() + if err != nil { + return err + } + if len(aliases) == 0 { + fmt.Println("(no aliases)") + return nil + } + for _, a := range aliases { + fmt.Printf("%-18s [%s]\n", a.Name, a.Source) + } + return nil +} + +// RunQueryView prints the SQL behind a named alias. +func RunQueryView(c *cli.Context) error { + name := strings.TrimSpace(c.Args().First()) + if name == "" { + return fmt.Errorf("usage: lrc query view ") + } + sqlText, found, err := ResolveAlias(name) + if err != nil { + return err + } + if !found { + return fmt.Errorf("no alias named %q", name) + } + fmt.Println(sqlText) + return nil +} + +// RunQueryDelete removes a user-defined alias. +func RunQueryDelete(c *cli.Context) error { + name := strings.TrimSpace(c.Args().First()) + if name == "" { + return fmt.Errorf("usage: lrc query delete ") + } + if err := DeleteAlias(name); err != nil { + return err + } + fmt.Printf("Deleted alias %q.\n", name) + return nil +} diff --git a/internal/reviewquery/engine.go b/internal/reviewquery/engine.go new file mode 100644 index 0000000..fa27dfa --- /dev/null +++ b/internal/reviewquery/engine.go @@ -0,0 +1,71 @@ +package reviewquery + +import ( + "fmt" + + "github.com/HexmosTech/git-lrc/storage" +) + +// QueryResult is a generic tabular result: column headers + stringified rows. +type QueryResult struct { + Columns []string + Rows [][]string +} + +const createTableSQL = ` +CREATE TABLE review_log ( + hash TEXT, + short_hash TEXT, + author TEXT, + email TEXT, + date TEXT, + branch TEXT, + subject TEXT, + action TEXT, + iterations INTEGER, + coverage INTEGER +);` + +const insertSQL = ` +INSERT INTO review_log + (hash, short_hash, author, email, date, branch, subject, action, iterations, coverage) +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. +func Run(f Filter, sqlText string) (QueryResult, error) { + records, err := Extract(f) + if err != nil { + return QueryResult{}, err + } + + db, err := storage.OpenInMemorySQLite() + if err != nil { + return QueryResult{}, err + } + defer func() { _ = db.Close() }() + + if _, err := storage.ExecSQL(db, createTableSQL); err != nil { + return QueryResult{}, fmt.Errorf("failed to create review_log table: %w", err) + } + + for _, r := range records { + date := "" + if !r.Date.IsZero() { + date = r.Date.UTC().Format("2006-01-02T15:04:05Z") + } + if _, err := storage.ExecSQL(db, insertSQL, + r.Hash, r.ShortHash, r.Author, r.Email, date, + r.Branch, r.Subject, r.Action, r.Iterations, r.CoveragePct, + ); err != nil { + return QueryResult{}, fmt.Errorf("failed to insert review record: %w", err) + } + } + + columns, rows, err := storage.QueryRows(db, sqlText) + if err != nil { + return QueryResult{}, err + } + return QueryResult{Columns: columns, Rows: rows}, nil +} diff --git a/internal/reviewquery/extract.go b/internal/reviewquery/extract.go new file mode 100644 index 0000000..44c563d --- /dev/null +++ b/internal/reviewquery/extract.go @@ -0,0 +1,135 @@ +package reviewquery + +import ( + "fmt" + "os/exec" + "regexp" + "strconv" + "strings" + "time" +) + +// trailerPrefix is the marker the commit-msg hook writes into each commit. +// See hooks/commit-msg.sh and internal/appcore/attestation_flow.go. +const trailerPrefix = "LiveReview Pre-Commit Check:" + +// trailerDetailRe pulls the optional "(iter:N, coverage:M%)" suffix. +var trailerDetailRe = regexp.MustCompile(`iter:(\d+),\s*coverage:(\d+)%`) + +// field/record separators chosen so they never appear in commit text. +const ( + fieldSep = "\x1f" + recordSep = "\x1e" +) + +// parseTrailer extracts the outcome and optional metrics from a single +// commit-message line. Pure function — unit-testable without git. +func parseTrailer(line string) (action string, iter int, covPct int, ok bool) { + line = strings.TrimSpace(line) + if !strings.HasPrefix(line, trailerPrefix) { + return "", 0, 0, false + } + rest := strings.TrimSpace(strings.TrimPrefix(line, trailerPrefix)) + + switch { + case strings.HasPrefix(rest, "ran"): + action = "reviewed" + case strings.HasPrefix(rest, "vouched"): + action = "vouched" + case strings.HasPrefix(rest, "skipped"): + action = "skipped" + default: + return "", 0, 0, false + } + + if m := trailerDetailRe.FindStringSubmatch(rest); m != nil { + iter, _ = strconv.Atoi(m[1]) + covPct, _ = strconv.Atoi(m[2]) + } + return action, iter, covPct, true +} + +// parseRecord turns one git-log record (fields joined by fieldSep) into a +// ReviewRecord. Pure function. Returns ok=false if the record is malformed. +func parseRecord(raw, branch string) (ReviewRecord, bool) { + parts := strings.Split(raw, fieldSep) + if len(parts) < 7 { + return ReviewRecord{}, false + } + rec := ReviewRecord{ + Hash: strings.TrimSpace(parts[0]), + ShortHash: strings.TrimSpace(parts[1]), + Author: parts[2], + Email: parts[3], + Subject: parts[5], + Branch: branch, + Action: "none", + } + if t, err := time.Parse(time.RFC3339, strings.TrimSpace(parts[4])); err == nil { + rec.Date = t + } + body := parts[6] + for _, line := range strings.Split(body, "\n") { + if action, iter, cov, ok := parseTrailer(line); ok { + rec.Action = action + rec.Iterations = iter + rec.CoveragePct = cov + break + } + } + return rec, true +} + +// currentBranch returns the branch git log will run against (best-effort). +func currentBranch() string { + out, err := exec.Command("git", "rev-parse", "--abbrev-ref", "HEAD").Output() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} + +// Extract runs `git log` (scoped by filter) and returns one record per commit. +func Extract(f Filter) ([]ReviewRecord, error) { + format := strings.Join([]string{"%H", "%h", "%an", "%ae", "%aI", "%s", "%B"}, fieldSep) + recordSep + args := []string{"log", "--pretty=format:" + format} + + if f.Author != "" { + args = append(args, "--author="+f.Author) + } + if !f.Since.IsZero() { + args = append(args, "--since="+f.Since.Format(time.RFC3339)) + } + if !f.Until.IsZero() { + args = append(args, "--until="+f.Until.Format(time.RFC3339)) + } + if f.Range != "" { + args = append(args, f.Range) + } + if f.PathPrefix != "" { + args = append(args, "--", f.PathPrefix) + } + + out, err := exec.Command("git", args...).Output() + if err != nil { + return nil, fmt.Errorf("failed to read git log (are you inside a git repo?): %w", err) + } + + branch := currentBranch() + rawRecords := strings.Split(string(out), recordSep) + records := make([]ReviewRecord, 0, len(rawRecords)) + for _, raw := range rawRecords { + if strings.TrimSpace(raw) == "" { + continue + } + rec, ok := parseRecord(raw, branch) + if !ok { + continue + } + if f.Action != "" && rec.Action != f.Action { + continue + } + records = append(records, rec) + } + return records, nil +} diff --git a/internal/reviewquery/extract_test.go b/internal/reviewquery/extract_test.go new file mode 100644 index 0000000..dd730c9 --- /dev/null +++ b/internal/reviewquery/extract_test.go @@ -0,0 +1,84 @@ +package reviewquery + +import ( + "strings" + "testing" +) + +func TestParseTrailer(t *testing.T) { + cases := []struct { + name string + in string + action string + iter int + cov int + ok bool + }{ + {"ran plain", "LiveReview Pre-Commit Check: ran", "reviewed", 0, 0, true}, + {"ran with metrics", "LiveReview Pre-Commit Check: ran (iter:3, coverage:82%)", "reviewed", 3, 82, true}, + {"vouched", "LiveReview Pre-Commit Check: vouched (iter:1, coverage:100%)", "vouched", 1, 100, true}, + {"skipped", "LiveReview Pre-Commit Check: skipped", "skipped", 0, 0, true}, + {"skipped manually", "LiveReview Pre-Commit Check: skipped manually", "skipped", 0, 0, true}, + {"indented", " LiveReview Pre-Commit Check: ran", "reviewed", 0, 0, true}, + {"unrelated", "Fix the login bug", "", 0, 0, false}, + {"empty", "", "", 0, 0, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + action, iter, cov, ok := parseTrailer(tc.in) + if ok != tc.ok || action != tc.action || iter != tc.iter || cov != tc.cov { + t.Errorf("parseTrailer(%q) = (%q,%d,%d,%v); want (%q,%d,%d,%v)", + tc.in, action, iter, cov, ok, tc.action, tc.iter, tc.cov, tc.ok) + } + }) + } +} + +func TestParseRecord(t *testing.T) { + // fields: hash, short, author, email, dateISO, subject, body + raw := strings.Join([]string{ + "abc123def456", + "abc123d", + "Jane Dev", + "jane@example.com", + "2026-06-17T10:30:00Z", + "Add the thing", + "Add the thing\n\nLiveReview Pre-Commit Check: ran (iter:2, coverage:75%)", + }, fieldSep) + + rec, ok := parseRecord(raw, "main") + if !ok { + t.Fatal("parseRecord returned ok=false for a valid record") + } + if rec.Hash != "abc123def456" || rec.ShortHash != "abc123d" { + t.Errorf("hash fields wrong: %+v", rec) + } + if rec.Author != "Jane Dev" || rec.Branch != "main" { + t.Errorf("author/branch wrong: %+v", rec) + } + if rec.Action != "reviewed" || rec.Iterations != 2 || rec.CoveragePct != 75 { + t.Errorf("trailer parse wrong: action=%q iter=%d cov=%d", rec.Action, rec.Iterations, rec.CoveragePct) + } + if rec.Date.Year() != 2026 || rec.Date.Month() != 6 { + t.Errorf("date parse wrong: %v", rec.Date) + } +} + +func TestParseRecordNoTrailer(t *testing.T) { + raw := strings.Join([]string{ + "h", "h", "A", "a@b.c", "2026-06-17T10:30:00Z", "subject", "body with no trailer", + }, fieldSep) + rec, ok := parseRecord(raw, "main") + if !ok { + t.Fatal("expected ok=true") + } + if rec.Action != "none" { + t.Errorf("expected action=none, got %q", rec.Action) + } +} + +func TestParseRecordMalformed(t *testing.T) { + if _, ok := parseRecord("too\x1ffew\x1ffields", "main"); ok { + t.Error("expected ok=false for malformed record") + } +} diff --git a/internal/reviewquery/format.go b/internal/reviewquery/format.go new file mode 100644 index 0000000..51f026b --- /dev/null +++ b/internal/reviewquery/format.go @@ -0,0 +1,87 @@ +package reviewquery + +import ( + "encoding/json" + "strings" +) + +// FormatTable renders a QueryResult as an aligned, human-readable table. +func FormatTable(r QueryResult) string { + if len(r.Columns) == 0 { + return "(no columns)\n" + } + + widths := make([]int, len(r.Columns)) + for i, c := range r.Columns { + widths[i] = len(c) + } + for _, row := range r.Rows { + for i, cell := range row { + if i < len(widths) && len(cell) > widths[i] { + widths[i] = len(cell) + } + } + } + + var b strings.Builder + writeRow := func(cells []string) { + for i, c := range cells { + b.WriteString(c) + if i < len(cells)-1 { + b.WriteString(strings.Repeat(" ", widths[i]-len(c)+2)) + } + } + b.WriteString("\n") + } + + writeRow(r.Columns) + sep := make([]string, len(r.Columns)) + for i := range sep { + sep[i] = strings.Repeat("-", widths[i]) + } + writeRow(sep) + for _, row := range r.Rows { + writeRow(row) + } + + if len(r.Rows) == 0 { + b.WriteString("(no rows)\n") + } + return b.String() +} + +// FormatJSON renders a QueryResult as a JSON array of row objects, preserving +// column order. All values are strings (the engine stringifies cells). +func FormatJSON(r QueryResult) (string, error) { + var b strings.Builder + b.WriteString("[") + for ri, row := range r.Rows { + if ri > 0 { + b.WriteString(",") + } + b.WriteString("{") + for ci, col := range r.Columns { + if ci > 0 { + b.WriteString(",") + } + key, err := json.Marshal(col) + if err != nil { + return "", err + } + val := "" + if ci < len(row) { + val = row[ci] + } + valJSON, err := json.Marshal(val) + if err != nil { + return "", err + } + b.Write(key) + b.WriteString(":") + b.Write(valJSON) + } + b.WriteString("}") + } + b.WriteString("]") + return b.String(), nil +} diff --git a/internal/reviewquery/format_test.go b/internal/reviewquery/format_test.go new file mode 100644 index 0000000..aec814b --- /dev/null +++ b/internal/reviewquery/format_test.go @@ -0,0 +1,46 @@ +package reviewquery + +import ( + "strings" + "testing" +) + +func sampleResult() QueryResult { + return QueryResult{ + Columns: []string{"Action", "Commits"}, + Rows: [][]string{ + {"reviewed", "89"}, + {"skipped", "218"}, + }, + } +} + +func TestFormatTable(t *testing.T) { + out := FormatTable(sampleResult()) + for _, want := range []string{"Action", "Commits", "reviewed", "89", "skipped", "218", "----"} { + if !strings.Contains(out, want) { + t.Errorf("table output missing %q\n---\n%s", want, out) + } + } +} + +func TestFormatJSON(t *testing.T) { + out, err := FormatJSON(sampleResult()) + if err != nil { + t.Fatalf("FormatJSON error: %v", err) + } + want := `[{"Action":"reviewed","Commits":"89"},{"Action":"skipped","Commits":"218"}]` + if out != want { + t.Errorf("FormatJSON =\n%s\nwant\n%s", out, want) + } +} + +func TestFormatJSONEmpty(t *testing.T) { + out, err := FormatJSON(QueryResult{Columns: []string{"a"}, Rows: nil}) + if err != nil { + t.Fatalf("FormatJSON error: %v", err) + } + if out != "[]" { + t.Errorf("FormatJSON empty = %q; want []", out) + } +} diff --git a/internal/reviewquery/model.go b/internal/reviewquery/model.go new file mode 100644 index 0000000..4cd22f8 --- /dev/null +++ b/internal/reviewquery/model.go @@ -0,0 +1,39 @@ +// Package reviewquery builds a queryable view of a repo's LiveReview history. +// +// It extracts review metadata from git commit trailers into structured records, +// loads them into an in-memory SQLite table, and runs SQL queries (or named +// aliases) against that table — the "filter -> group -> aggregate" engine. +package reviewquery + +import "time" + +// ReviewRecord is one commit's review metadata, one row in the review_log table. +type ReviewRecord struct { + Hash string // full commit hash + ShortHash string // abbreviated hash + Author string // author name + Email string // author email + Date time.Time // author date + Branch string // branch the query was run from (Phase 1: current branch) + Subject string // commit subject (first line) + Action string // reviewed | vouched | skipped | none + Iterations int // review iterations (0 if absent) + CoveragePct int // coverage percent (0 if absent) +} + +// Filter narrows which commits are extracted. Phase 1 uses Range/Since; the +// rest are wired in Phase 2. +type Filter struct { + Range string // e.g. "main...feature" (PR diff); empty = full history + Since time.Time // zero = no lower bound + Until time.Time // zero = no upper bound + Author string // substring match on author/email + PathPrefix string // limit to commits touching this path + Action string // limit to one action +} + +// Alias is a saved, named SQL query stored in ~/.lrc/queries.toml. +type Alias struct { + Name string `toml:"name"` + SQL string `toml:"sql"` +} diff --git a/main.go b/main.go index 2c43c08..5e6c411 100644 --- a/main.go +++ b/main.go @@ -10,6 +10,7 @@ import ( "github.com/HexmosTech/git-lrc/internal/appui" "github.com/HexmosTech/git-lrc/internal/reviewdb" "github.com/HexmosTech/git-lrc/internal/reviewopts" + "github.com/HexmosTech/git-lrc/internal/reviewquery" "github.com/HexmosTech/git-lrc/internal/selfupdate" "github.com/urfave/cli/v2" ) @@ -84,6 +85,10 @@ func main() { RunConfigInit: appcore.RunConfigInit, RunConfigCheck: appcore.RunConfigCheck, RunConfigPreview: appcore.RunConfigPreview, + RunQuery: reviewquery.RunQuery, + RunQueryList: reviewquery.RunQueryList, + RunQueryView: reviewquery.RunQueryView, + RunQueryDelete: reviewquery.RunQueryDelete, }) if err := app.Run(os.Args); err != nil { diff --git a/scripts/lrc-install.ps1 b/scripts/lrc-install.ps1 index 35143a0..0a371dc 100644 --- a/scripts/lrc-install.ps1 +++ b/scripts/lrc-install.ps1 @@ -646,6 +646,23 @@ if (-not $env:HOME -and $env:USERPROFILE) { $env:HOME = $env:USERPROFILE } +# Ship default review-history query aliases (idempotent — never clobbers edits) +$LRC_DATA_DIR = Join-Path $env:USERPROFILE ".lrc" +$LRC_QUERIES_FILE = Join-Path $LRC_DATA_DIR "queries.toml" +if (-not (Test-Path $LRC_QUERIES_FILE)) { + New-Item -ItemType Directory -Path $LRC_DATA_DIR -Force | Out-Null + @' +# git-lrc saved queries. Run with: lrc query +# Add your own with: lrc query --add "" --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" +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 + Write-Host " OK Wrote default query aliases to $LRC_QUERIES_FILE" -ForegroundColor Green +} + # Install global hooks via lrc unless explicitly suppressed by the caller. if ($LRC_INSTALL_SKIP_HOOKS -eq "1") { Write-Host "Skipping automatic hook installation because LRC_INSTALL_SKIP_HOOKS=1" -ForegroundColor Yellow diff --git a/scripts/lrc-install.sh b/scripts/lrc-install.sh index 2bbf251..e78332f 100755 --- a/scripts/lrc-install.sh +++ b/scripts/lrc-install.sh @@ -529,6 +529,21 @@ esac ENVEOF chmod +x "$LRC_ENV_FILE" +# Ship default review-history query aliases (idempotent — never clobbers edits) +LRC_QUERIES_FILE="$LRC_ENV_DIR/queries.toml" +if [ ! -f "$LRC_QUERIES_FILE" ]; then + cat > "$LRC_QUERIES_FILE" << 'QUERIESEOF' +# git-lrc saved queries. Run with: lrc query +# Add your own with: lrc query --add "" --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" +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 + echo -e " ${GREEN}OK${NC} Wrote default query aliases to $LRC_QUERIES_FILE" +fi + # Helper: append source line to a shell rc file if not already present add_source_line() { local rcfile="$1" diff --git a/storage/sqlite_query_io.go b/storage/sqlite_query_io.go new file mode 100644 index 0000000..179b9ef --- /dev/null +++ b/storage/sqlite_query_io.go @@ -0,0 +1,65 @@ +package storage + +import ( + "database/sql" + "fmt" + + _ "modernc.org/sqlite" +) + +// OpenInMemorySQLite opens a fresh in-memory sqlite database via the storage +// boundary. Used by the review-query engine to build an ephemeral table that is +// discarded when the handle is closed. +func OpenInMemorySQLite() (*sql.DB, error) { + db, err := sql.Open("sqlite", ":memory:") + if err != nil { + return nil, fmt.Errorf("failed to open in-memory sqlite database: %w", err) + } + if err := db.Ping(); err != nil { + _ = db.Close() + return nil, fmt.Errorf("failed to connect in-memory sqlite database: %w", err) + } + return db, nil +} + +// QueryRows runs a read-only 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. +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") + } + + result, err := db.Query(query, args...) + if err != nil { + return nil, nil, fmt.Errorf("failed SQL query: %w", err) + } + defer func() { _ = result.Close() }() + + columns, err = result.Columns() + if err != nil { + return nil, nil, fmt.Errorf("failed reading query columns: %w", err) + } + + for result.Next() { + raw := make([]sql.NullString, len(columns)) + scanTargets := make([]any, len(columns)) + for i := range raw { + scanTargets[i] = &raw[i] + } + if err := result.Scan(scanTargets...); err != nil { + return nil, nil, fmt.Errorf("failed scanning query row: %w", err) + } + row := make([]string, len(columns)) + for i, ns := range raw { + if ns.Valid { + row[i] = ns.String + } + } + rows = append(rows, row) + } + if err := result.Err(); err != nil { + return nil, nil, fmt.Errorf("failed iterating query rows: %w", err) + } + return columns, rows, nil +} From a92494dbc250e58b3efcba2cd6fa1a6d72e919b8 Mon Sep 17 00:00:00 2001 From: Vignesh Goud Date: Thu, 18 Jun 2026 20:29:40 +0530 Subject: [PATCH 2/6] address review: richer 'query --help', batched inserts, perf benchmark - query --help now documents the review_log schema (columns + types) and includes runnable example queries (incident forensics, per-author, coverage) - storage.BulkInsert: load rows in one transaction + prepared statement (far faster than per-row autocommit on large repos) - split RunOnRecords out of Run so the engine is testable/benchmarkable without git; add correctness tests + a scaling benchmark - installer query-file write is already idempotent (guarded by file-exists) LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%) --- cmd/app.go | 44 +++++++++++++---- internal/reviewquery/engine.go | 19 +++++--- internal/reviewquery/engine_test.go | 73 +++++++++++++++++++++++++++++ storage/sqlite_query_io.go | 29 ++++++++++++ 4 files changed, 150 insertions(+), 15 deletions(-) create mode 100644 internal/reviewquery/engine_test.go diff --git a/cmd/app.go b/cmd/app.go index a5c8a6b..2db1479 100644 --- a/cmd/app.go +++ b/cmd/app.go @@ -354,17 +354,43 @@ func BuildApp(version, buildTime, gitCommit, reviewMode string, baseFlags, debug { Name: "query", Usage: "Query LiveReview history with SQL or a saved alias (e.g. 'lrc query stats')", - Description: `Runs a SQL query (or a saved alias) against an in-memory table of this -repo's review history, built from commit trailers. + Description: `Builds an in-memory SQLite table of this repo's review history (parsed +from the 'LiveReview Pre-Commit Check' commit trailers) and runs SQL — or a +saved alias — against it. Output as a table or, with --json, machine-readable. - lrc query # default 'stats' alias - lrc query stats --json # same data as JSON - lrc query "SELECT author, COUNT(*) FROM review_log GROUP BY author" - lrc query --add "SELECT ..." --name myreport # save an alias - lrc query myreport # run the saved alias +TABLE: review_log (one row per commit) + hash TEXT full commit hash + short_hash TEXT abbreviated hash + author TEXT commit author name + email TEXT commit author email + date TEXT author date, ISO-8601 (sortable, e.g. 2026-06-17T10:30:00Z) + branch TEXT branch the query ran from + subject TEXT commit subject (first line) + action TEXT 'reviewed' | 'vouched' | 'skipped' | 'none' + iterations INTEGER review iterations (0 if none) + coverage INTEGER review coverage percent 0-100 (0 if none) -Columns in review_log: hash, short_hash, author, email, date, branch, -subject, action, iterations, coverage.`, +ALIASES: built-ins (stats, by-author, recent) plus your own, saved in +~/.lrc/queries.toml. 'lrc query' with no args runs the 'stats' alias. + +EXAMPLES + lrc query # default summary (the 'stats' alias) + lrc query stats --json # same data, as JSON + lrc query list # show all aliases + lrc query view stats # show an alias's SQL + + # Was a specific commit reviewed? (incident forensics) + lrc query "SELECT short_hash, action, iterations, coverage FROM review_log WHERE hash LIKE 'a1b2c3%'" + + # Per-author review effort + lrc query "SELECT author, COUNT(*) AS commits, SUM(action='reviewed') AS reviewed FROM review_log GROUP BY author ORDER BY commits DESC" + + # Coverage only on reviewed commits + lrc query "SELECT ROUND(AVG(coverage),1) AS avg_cov FROM review_log WHERE action='reviewed'" + + # Save and reuse your own query + lrc query --add "SELECT date, subject FROM review_log WHERE action='skipped'" --name skipped + lrc query skipped --json`, Flags: []cli.Flag{ &cli.BoolFlag{Name: "json", Usage: "output machine-readable JSON"}, &cli.StringFlag{Name: "add", Usage: "save the given SQL as an alias (requires --name)"}, diff --git a/internal/reviewquery/engine.go b/internal/reviewquery/engine.go index fa27dfa..75e82ea 100644 --- a/internal/reviewquery/engine.go +++ b/internal/reviewquery/engine.go @@ -39,7 +39,12 @@ func Run(f Filter, sqlText string) (QueryResult, error) { if err != nil { return QueryResult{}, err } + return RunOnRecords(records, sqlText) +} +// 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) { db, err := storage.OpenInMemorySQLite() if err != nil { return QueryResult{}, err @@ -50,22 +55,24 @@ func Run(f Filter, sqlText string) (QueryResult, error) { return QueryResult{}, fmt.Errorf("failed to create review_log table: %w", err) } + rows := make([][]any, 0, len(records)) for _, r := range records { date := "" if !r.Date.IsZero() { date = r.Date.UTC().Format("2006-01-02T15:04:05Z") } - if _, err := storage.ExecSQL(db, insertSQL, + rows = append(rows, []any{ r.Hash, r.ShortHash, r.Author, r.Email, date, r.Branch, r.Subject, r.Action, r.Iterations, r.CoveragePct, - ); err != nil { - return QueryResult{}, fmt.Errorf("failed to insert review record: %w", err) - } + }) + } + if err := storage.BulkInsert(db, insertSQL, rows); err != nil { + return QueryResult{}, err } - columns, rows, err := storage.QueryRows(db, sqlText) + columns, outRows, err := storage.QueryRows(db, sqlText) if err != nil { return QueryResult{}, err } - return QueryResult{Columns: columns, Rows: rows}, nil + return QueryResult{Columns: columns, Rows: outRows}, nil } diff --git a/internal/reviewquery/engine_test.go b/internal/reviewquery/engine_test.go new file mode 100644 index 0000000..a245009 --- /dev/null +++ b/internal/reviewquery/engine_test.go @@ -0,0 +1,73 @@ +package reviewquery + +import ( + "fmt" + "testing" + "time" +) + +func syntheticRecords(n int) []ReviewRecord { + actions := []string{"reviewed", "vouched", "skipped", "none"} + recs := make([]ReviewRecord, n) + base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + for i := 0; i < n; i++ { + recs[i] = ReviewRecord{ + Hash: fmt.Sprintf("%040x", i), + ShortHash: fmt.Sprintf("%07x", i), + Author: fmt.Sprintf("dev%d", i%10), + Email: fmt.Sprintf("dev%d@example.com", i%10), + Date: base.Add(time.Duration(i) * time.Hour), + Branch: "main", + Subject: "commit subject", + Action: actions[i%len(actions)], + Iterations: i % 5, + CoveragePct: i % 101, + } + } + return recs +} + +func TestRunOnRecords(t *testing.T) { + recs := syntheticRecords(8) // 2 of each action + res, err := RunOnRecords(recs, "SELECT action, COUNT(*) AS n FROM review_log GROUP BY action ORDER BY action") + if err != nil { + t.Fatalf("RunOnRecords error: %v", err) + } + if len(res.Columns) != 2 || res.Columns[0] != "action" { + t.Errorf("unexpected columns: %v", res.Columns) + } + if len(res.Rows) != 4 { + t.Fatalf("expected 4 action groups, got %d: %v", len(res.Rows), res.Rows) + } + for _, row := range res.Rows { + if row[1] != "2" { + t.Errorf("expected 2 per action, got %v", row) + } + } +} + +func TestRunOnRecordsEmpty(t *testing.T) { + res, err := RunOnRecords(nil, "SELECT COUNT(*) AS n FROM review_log") + if err != nil { + t.Fatalf("RunOnRecords(nil) error: %v", err) + } + if len(res.Rows) != 1 || res.Rows[0][0] != "0" { + t.Errorf("expected count 0 on empty input, got %v", res.Rows) + } +} + +// BenchmarkRunOnRecords measures load+query cost at various repo sizes. +// Run: go test -run=^$ -bench=RunOnRecords -benchmem ./internal/reviewquery/ +func BenchmarkRunOnRecords(b *testing.B) { + for _, n := range []int{1000, 10000, 100000} { + recs := syntheticRecords(n) + b.Run(fmt.Sprintf("commits=%d", n), func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + if _, err := RunOnRecords(recs, "SELECT action, COUNT(*) FROM review_log GROUP BY action"); err != nil { + b.Fatal(err) + } + } + }) + } +} diff --git a/storage/sqlite_query_io.go b/storage/sqlite_query_io.go index 179b9ef..8d4c353 100644 --- a/storage/sqlite_query_io.go +++ b/storage/sqlite_query_io.go @@ -22,6 +22,35 @@ func OpenInMemorySQLite() (*sql.DB, error) { return db, nil } +// BulkInsert inserts many rows under a single transaction with a prepared +// statement — far faster than autocommitting each row (matters for large repos). +func BulkInsert(db *sql.DB, query string, rows [][]any) error { + if db == nil { + return fmt.Errorf("failed bulk insert: nil database handle") + } + tx, err := db.Begin() + if err != nil { + return fmt.Errorf("failed to begin transaction: %w", err) + } + stmt, err := tx.Prepare(query) + if err != nil { + _ = tx.Rollback() + return fmt.Errorf("failed to prepare insert: %w", err) + } + defer func() { _ = stmt.Close() }() + + for _, args := range rows { + if _, err := stmt.Exec(args...); err != nil { + _ = tx.Rollback() + return fmt.Errorf("failed bulk insert exec: %w", err) + } + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("failed to commit insert transaction: %w", err) + } + return nil +} + // QueryRows runs a read-only 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. From d5b9bf3661ea187777f8155487d60efb2ce097d5 Mon Sep 17 00:00:00 2001 From: Vignesh Goud Date: Thu, 18 Jun 2026 21:33:35 +0530 Subject: [PATCH 3/6] feat(query): add --from/--to/--range scan-bounding for large repos Bounds the git log scan so huge histories (e.g. Linux kernel, ~1.5M commits) aren't walked in full. --from/--to accept any git date; --range takes a ref range (e.g. main...feature) which also serves per-PR stats. Flags work before or after the positional arg. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%) --- cmd/app.go | 9 +++- internal/reviewquery/command.go | 63 ++++++++++++++++++++++------ internal/reviewquery/command_test.go | 36 ++++++++++++++++ internal/reviewquery/extract.go | 8 ++-- internal/reviewquery/model.go | 18 ++++---- 5 files changed, 109 insertions(+), 25 deletions(-) create mode 100644 internal/reviewquery/command_test.go diff --git a/cmd/app.go b/cmd/app.go index 2db1479..de3e73e 100644 --- a/cmd/app.go +++ b/cmd/app.go @@ -390,11 +390,18 @@ EXAMPLES # Save and reuse your own query lrc query --add "SELECT date, subject FROM review_log WHERE action='skipped'" --name skipped - lrc query skipped --json`, + lrc query skipped --json + + # Bound the scan on huge repos (Linux kernel = ~1.5M commits) + lrc query stats --from "2024-01-01" --to "2024-12-31" + lrc query stats --range main...feature # just this PR's commits`, Flags: []cli.Flag{ &cli.BoolFlag{Name: "json", Usage: "output machine-readable JSON"}, &cli.StringFlag{Name: "add", Usage: "save the given SQL as an alias (requires --name)"}, &cli.StringFlag{Name: "name", Usage: "alias name to save with --add"}, + &cli.StringFlag{Name: "from", Usage: "only scan commits since this git date (e.g. 2024-01-01, '2 weeks ago') — bounds large repos"}, + &cli.StringFlag{Name: "to", Usage: "only scan commits until this git date"}, + &cli.StringFlag{Name: "range", Usage: "only scan a ref range, e.g. main...feature (per-PR stats)"}, }, Action: h.RunQuery, Subcommands: []*cli.Command{ diff --git a/internal/reviewquery/command.go b/internal/reviewquery/command.go index 6558087..2f245a0 100644 --- a/internal/reviewquery/command.go +++ b/internal/reviewquery/command.go @@ -22,18 +22,13 @@ func RunQuery(c *cli.Context) error { return nil } - // urfave/cli stops parsing flags at the first positional arg, so support a - // trailing --json too (e.g. `lrc query stats --json`). + // Seed from flags placed BEFORE the positional arg (cli parses those). jsonOut := c.Bool("json") - positionals := make([]string, 0, c.NArg()) - for _, a := range c.Args().Slice() { - switch a { - case "--json", "-json", "-j": - jsonOut = true - default: - positionals = append(positionals, a) - } - } + filter := Filter{From: c.String("from"), To: c.String("to"), Range: c.String("range")} + + // 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) arg := "stats" // default alias if len(positionals) > 0 && strings.TrimSpace(positionals[0]) != "" { @@ -49,7 +44,7 @@ func RunQuery(c *cli.Context) error { sqlText = strings.Join(positionals, " ") } - res, err := Run(Filter{}, sqlText) + res, err := Run(filter, sqlText) if err != nil { return err } @@ -66,6 +61,50 @@ func RunQuery(c *cli.Context) error { return nil } +// 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 { + 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": + *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] + i++ + continue + } + } + positionals = append(positionals, a) + } + return positionals +} + // RunQueryList prints every alias and its source. func RunQueryList(c *cli.Context) error { aliases, err := ListAliases() diff --git a/internal/reviewquery/command_test.go b/internal/reviewquery/command_test.go new file mode 100644 index 0000000..728e715 --- /dev/null +++ b/internal/reviewquery/command_test.go @@ -0,0 +1,36 @@ +package reviewquery + +import "testing" + +func TestParseTrailingFlags(t *testing.T) { + cases := []struct { + name string + args []string + wantPos []string + from string + to string + rng string + json bool + }{ + {"none", []string{"stats"}, []string{"stats"}, "", "", "", false}, + {"trailing json", []string{"stats", "--json"}, []string{"stats"}, "", "", "", true}, + {"from value", []string{"stats", "--from", "2024-01-01"}, []string{"stats"}, "2024-01-01", "", "", false}, + {"from equals", []string{"stats", "--from=2024-01-01"}, []string{"stats"}, "2024-01-01", "", "", false}, + {"range+json", []string{"q", "--range", "main...dev", "--json"}, []string{"q"}, "", "", "main...dev", true}, + {"to", []string{"stats", "--to=2025-12-31"}, []string{"stats"}, "", "2025-12-31", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + jsonOut := false + f := Filter{} + pos := parseTrailingFlags(tc.args, &jsonOut, &f) + if len(pos) != len(tc.wantPos) || (len(pos) > 0 && pos[0] != tc.wantPos[0]) { + t.Errorf("positionals = %v; want %v", pos, tc.wantPos) + } + if f.From != tc.from || f.To != tc.to || f.Range != tc.rng || jsonOut != tc.json { + t.Errorf("got from=%q to=%q range=%q json=%v; want from=%q to=%q range=%q json=%v", + f.From, f.To, f.Range, jsonOut, tc.from, tc.to, tc.rng, tc.json) + } + }) + } +} diff --git a/internal/reviewquery/extract.go b/internal/reviewquery/extract.go index 44c563d..a6c1877 100644 --- a/internal/reviewquery/extract.go +++ b/internal/reviewquery/extract.go @@ -97,11 +97,11 @@ func Extract(f Filter) ([]ReviewRecord, error) { if f.Author != "" { args = append(args, "--author="+f.Author) } - if !f.Since.IsZero() { - args = append(args, "--since="+f.Since.Format(time.RFC3339)) + if f.From != "" { + args = append(args, "--since="+f.From) } - if !f.Until.IsZero() { - args = append(args, "--until="+f.Until.Format(time.RFC3339)) + if f.To != "" { + args = append(args, "--until="+f.To) } if f.Range != "" { args = append(args, f.Range) diff --git a/internal/reviewquery/model.go b/internal/reviewquery/model.go index 4cd22f8..3a3eb7d 100644 --- a/internal/reviewquery/model.go +++ b/internal/reviewquery/model.go @@ -21,15 +21,17 @@ type ReviewRecord struct { CoveragePct int // coverage percent (0 if absent) } -// Filter narrows which commits are extracted. Phase 1 uses Range/Since; the -// rest are wired in Phase 2. +// Filter narrows (and bounds) which commits are scanned. From/To/Range bound the +// git log so huge repos (e.g. the Linux kernel, ~1.5M commits) don't get walked +// in full. From/To are passed straight to git, so they accept any git date +// (e.g. "2024-01-01", "2 weeks ago"). type Filter struct { - Range string // e.g. "main...feature" (PR diff); empty = full history - Since time.Time // zero = no lower bound - Until time.Time // zero = no upper bound - Author string // substring match on author/email - PathPrefix string // limit to commits touching this path - Action string // limit to one action + Range string // e.g. "main...feature" (PR diff); empty = full history + From string // git --since bound (lower); empty = no lower bound + To string // git --until bound (upper); empty = no upper bound + Author string // substring match on author/email + PathPrefix string // limit to commits touching this path + Action string // limit to one action } // Alias is a saved, named SQL query stored in ~/.lrc/queries.toml. From f659150ecdf82443938d835d899264503eb092f0 Mon Sep 17 00:00:00 2001 From: Shrijith Venkatramana Date: Fri, 19 Jun 2026 14:01:39 +0000 Subject: [PATCH 4/6] Address various minor issues LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%) --- internal/reviewquery/aliases.go | 19 ++++--- internal/reviewquery/command.go | 74 ++++++++++++++++------------ internal/reviewquery/command_test.go | 18 ++++++- internal/reviewquery/engine.go | 37 +++++++++++++- internal/reviewquery/extract.go | 11 ++++- internal/reviewquery/format.go | 6 +++ scripts/lrc-install.ps1 | 2 +- scripts/lrc-install.sh | 2 +- storage/sqlite_query_io.go | 8 ++- 9 files changed, 132 insertions(+), 45 deletions(-) diff --git a/internal/reviewquery/aliases.go b/internal/reviewquery/aliases.go index b4ad5f7..cc21bdf 100644 --- a/internal/reviewquery/aliases.go +++ b/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 diff --git a/internal/reviewquery/command.go b/internal/reviewquery/command.go index 2f245a0..281a746 100644 --- a/internal/reviewquery/command.go +++ b/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. diff --git a/internal/reviewquery/command_test.go b/internal/reviewquery/command_test.go index 728e715..ab9877c 100644 --- a/internal/reviewquery/command_test.go +++ b/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) + } + }) + } +} diff --git a/internal/reviewquery/engine.go b/internal/reviewquery/engine.go index 75e82ea..2876497 100644 --- a/internal/reviewquery/engine.go +++ b/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) diff --git a/internal/reviewquery/extract.go b/internal/reviewquery/extract.go index a6c1877..007c32b 100644 --- a/internal/reviewquery/extract.go +++ b/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) diff --git a/internal/reviewquery/format.go b/internal/reviewquery/format.go index 51f026b..c4605c1 100644 --- a/internal/reviewquery/format.go +++ b/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("[") diff --git a/scripts/lrc-install.ps1 b/scripts/lrc-install.ps1 index 0a371dc..054ccbf 100644 --- a/scripts/lrc-install.ps1 +++ b/scripts/lrc-install.ps1 @@ -656,7 +656,7 @@ if (-not (Test-Path $LRC_QUERIES_FILE)) { # Add your own with: lrc query --add "" --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 diff --git a/scripts/lrc-install.sh b/scripts/lrc-install.sh index e78332f..f6a297c 100755 --- a/scripts/lrc-install.sh +++ b/scripts/lrc-install.sh @@ -537,7 +537,7 @@ if [ ! -f "$LRC_QUERIES_FILE" ]; then # Add your own with: lrc query --add "" --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 diff --git a/storage/sqlite_query_io.go b/storage/sqlite_query_io.go index 8d4c353..736d9b3 100644 --- a/storage/sqlite_query_io.go +++ b/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") From ce2f395664dd6dc78f1e9ea4dca77156fadf2e7c Mon Sep 17 00:00:00 2001 From: Vignesh Goud Date: Fri, 19 Jun 2026 19:54:47 +0530 Subject: [PATCH 5/6] address review: bare 'query' shows help, 'add' subcommand, list previews, SQL hints - lrc query with no args now prints help (not the stats alias) - promote add to a subcommand: lrc query add "" (consistent with list/view/delete); removed --add/--name options - lrc query list now shows a truncated SQL preview per alias - on a failed query, append correct-usage examples (alias + raw SQL) - query --help documents the ~/.lrc/queries.toml structure - remove installer queries.toml writing: default aliases are built into the binary, so the installer change was unnecessary LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%) --- cmd/app.go | 28 ++++++++++------- internal/reviewquery/command.go | 55 +++++++++++++++++++-------------- main.go | 1 + scripts/lrc-install.ps1 | 17 ---------- scripts/lrc-install.sh | 15 --------- 5 files changed, 50 insertions(+), 66 deletions(-) diff --git a/cmd/app.go b/cmd/app.go index de3e73e..dfc7581 100644 --- a/cmd/app.go +++ b/cmd/app.go @@ -62,6 +62,7 @@ type Handlers struct { RunConfigCheck cli.ActionFunc RunConfigPreview cli.ActionFunc RunQuery cli.ActionFunc + RunQueryAdd cli.ActionFunc RunQueryList cli.ActionFunc RunQueryView cli.ActionFunc RunQueryDelete cli.ActionFunc @@ -370,14 +371,18 @@ TABLE: review_log (one row per commit) iterations INTEGER review iterations (0 if none) coverage INTEGER review coverage percent 0-100 (0 if none) -ALIASES: built-ins (stats, by-author, recent) plus your own, saved in -~/.lrc/queries.toml. 'lrc query' with no args runs the 'stats' alias. +ALIASES: built-in (stats, by-author, recent) plus your own. Manage them with +'lrc query add|list|view|delete'. User aliases are saved in ~/.lrc/queries.toml: + + [queries] + skipped = "SELECT date, subject FROM review_log WHERE action='skipped'" + my-cov = "SELECT ROUND(AVG(coverage),1) FROM review_log WHERE action='reviewed'" EXAMPLES - lrc query # default summary (the 'stats' alias) + lrc query stats # run a built-in alias lrc query stats --json # same data, as JSON - lrc query list # show all aliases - lrc query view stats # show an alias's SQL + lrc query list # show all aliases + a preview + lrc query view stats # show an alias's full SQL # Was a specific commit reviewed? (incident forensics) lrc query "SELECT short_hash, action, iterations, coverage FROM review_log WHERE hash LIKE 'a1b2c3%'" @@ -385,11 +390,8 @@ EXAMPLES # Per-author review effort lrc query "SELECT author, COUNT(*) AS commits, SUM(action='reviewed') AS reviewed FROM review_log GROUP BY author ORDER BY commits DESC" - # Coverage only on reviewed commits - lrc query "SELECT ROUND(AVG(coverage),1) AS avg_cov FROM review_log WHERE action='reviewed'" - # Save and reuse your own query - lrc query --add "SELECT date, subject FROM review_log WHERE action='skipped'" --name skipped + lrc query add skipped "SELECT date, subject FROM review_log WHERE action='skipped'" lrc query skipped --json # Bound the scan on huge repos (Linux kernel = ~1.5M commits) @@ -397,14 +399,18 @@ EXAMPLES lrc query stats --range main...feature # just this PR's commits`, Flags: []cli.Flag{ &cli.BoolFlag{Name: "json", Usage: "output machine-readable JSON"}, - &cli.StringFlag{Name: "add", Usage: "save the given SQL as an alias (requires --name)"}, - &cli.StringFlag{Name: "name", Usage: "alias name to save with --add"}, &cli.StringFlag{Name: "from", Usage: "only scan commits since this git date (e.g. 2024-01-01, '2 weeks ago') — bounds large repos"}, &cli.StringFlag{Name: "to", Usage: "only scan commits until this git date"}, &cli.StringFlag{Name: "range", Usage: "only scan a ref range, e.g. main...feature (per-PR stats)"}, }, Action: h.RunQuery, Subcommands: []*cli.Command{ + { + Name: "add", + Usage: "Save a query alias: lrc query add \"\"", + ArgsUsage: " \"\"", + Action: h.RunQueryAdd, + }, { Name: "list", Usage: "List saved and built-in query aliases", diff --git a/internal/reviewquery/command.go b/internal/reviewquery/command.go index 281a746..a469452 100644 --- a/internal/reviewquery/command.go +++ b/internal/reviewquery/command.go @@ -7,25 +7,10 @@ import ( "github.com/urfave/cli/v2" ) -// 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. +// RunQuery is the default action for `lrc query`: runs a saved alias or raw SQL +// and prints a table or JSON. With no argument it shows help (so users discover +// the schema and examples rather than silently running a default query). func RunQuery(c *cli.Context) error { - if c.IsSet("add") { - add := strings.TrimSpace(c.String("add")) - name := strings.TrimSpace(c.String("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 - } - fmt.Printf("Saved alias %q.\n", name) - return nil - } - // Seed from flags placed BEFORE the positional arg (cli parses those). jsonOut := c.Bool("json") filter := Filter{From: c.String("from"), To: c.String("to"), Range: c.String("range")} @@ -37,11 +22,12 @@ func RunQuery(c *cli.Context) error { return err } - arg := "stats" // default alias - if len(positionals) > 0 && strings.TrimSpace(positionals[0]) != "" { - arg = strings.TrimSpace(positionals[0]) + // No alias/SQL given -> show help instead of defaulting to a query. + if len(positionals) == 0 || strings.TrimSpace(positionals[0]) == "" { + return cli.ShowSubcommandHelp(c) } + arg := strings.TrimSpace(positionals[0]) sqlText, found, err := ResolveAlias(arg) if err != nil { return err @@ -53,7 +39,7 @@ func RunQuery(c *cli.Context) error { res, err := Run(filter, sqlText) if err != nil { - return err + return fmt.Errorf("%w\n\nRun a saved alias or valid SQL, e.g.:\n lrc query stats\n lrc query \"SELECT * FROM review_log LIMIT 5\"\nSee 'lrc query --help' for the table schema and 'lrc query list' for aliases", err) } if jsonOut { @@ -68,6 +54,29 @@ func RunQuery(c *cli.Context) error { return nil } +// RunQueryAdd saves a user alias: `lrc query add ""`. +func RunQueryAdd(c *cli.Context) error { + name := strings.TrimSpace(c.Args().Get(0)) + sqlText := strings.TrimSpace(c.Args().Get(1)) + if name == "" || sqlText == "" { + return fmt.Errorf("usage: lrc query add \"\"") + } + if err := AddAlias(name, sqlText); err != nil { + return err + } + fmt.Printf("Saved alias %q.\n", name) + return nil +} + +// truncateSQL shortens a query for compact listing. +func truncateSQL(s string, max int) string { + s = strings.Join(strings.Fields(s), " ") // collapse whitespace/newlines + if len(s) > max { + return s[:max-1] + "…" + } + return s +} + // 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. @@ -128,7 +137,7 @@ func RunQueryList(c *cli.Context) error { return nil } for _, a := range aliases { - fmt.Printf("%-18s [%s]\n", a.Name, a.Source) + fmt.Printf("%-16s %-10s %s\n", a.Name, "["+a.Source+"]", truncateSQL(a.SQL, 60)) } return nil } diff --git a/main.go b/main.go index 5e6c411..c26b120 100644 --- a/main.go +++ b/main.go @@ -86,6 +86,7 @@ func main() { RunConfigCheck: appcore.RunConfigCheck, RunConfigPreview: appcore.RunConfigPreview, RunQuery: reviewquery.RunQuery, + RunQueryAdd: reviewquery.RunQueryAdd, RunQueryList: reviewquery.RunQueryList, RunQueryView: reviewquery.RunQueryView, RunQueryDelete: reviewquery.RunQueryDelete, diff --git a/scripts/lrc-install.ps1 b/scripts/lrc-install.ps1 index 054ccbf..35143a0 100644 --- a/scripts/lrc-install.ps1 +++ b/scripts/lrc-install.ps1 @@ -646,23 +646,6 @@ if (-not $env:HOME -and $env:USERPROFILE) { $env:HOME = $env:USERPROFILE } -# Ship default review-history query aliases (idempotent — never clobbers edits) -$LRC_DATA_DIR = Join-Path $env:USERPROFILE ".lrc" -$LRC_QUERIES_FILE = Join-Path $LRC_DATA_DIR "queries.toml" -if (-not (Test-Path $LRC_QUERIES_FILE)) { - New-Item -ItemType Directory -Path $LRC_DATA_DIR -Force | Out-Null - @' -# git-lrc saved queries. Run with: lrc query -# Add your own with: lrc query --add "" --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),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 - Write-Host " OK Wrote default query aliases to $LRC_QUERIES_FILE" -ForegroundColor Green -} - # Install global hooks via lrc unless explicitly suppressed by the caller. if ($LRC_INSTALL_SKIP_HOOKS -eq "1") { Write-Host "Skipping automatic hook installation because LRC_INSTALL_SKIP_HOOKS=1" -ForegroundColor Yellow diff --git a/scripts/lrc-install.sh b/scripts/lrc-install.sh index f6a297c..2bbf251 100755 --- a/scripts/lrc-install.sh +++ b/scripts/lrc-install.sh @@ -529,21 +529,6 @@ esac ENVEOF chmod +x "$LRC_ENV_FILE" -# Ship default review-history query aliases (idempotent — never clobbers edits) -LRC_QUERIES_FILE="$LRC_ENV_DIR/queries.toml" -if [ ! -f "$LRC_QUERIES_FILE" ]; then - cat > "$LRC_QUERIES_FILE" << 'QUERIESEOF' -# git-lrc saved queries. Run with: lrc query -# Add your own with: lrc query --add "" --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),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 - echo -e " ${GREEN}OK${NC} Wrote default query aliases to $LRC_QUERIES_FILE" -fi - # Helper: append source line to a shell rc file if not already present add_source_line() { local rcfile="$1" From 94fbab12d79fdaf2fc158fab704c2bc7ea0bed35 Mon Sep 17 00:00:00 2001 From: Shrijith Venkatramana Date: Fri, 19 Jun 2026 14:53:18 +0000 Subject: [PATCH 6/6] fix minor issues LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%) --- internal/reviewquery/engine.go | 3 ++ internal/reviewquery/engine_test.go | 43 +++++++++++++++++++++++++++++ storage/sqlite_query_io.go | 4 ++- 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/internal/reviewquery/engine.go b/internal/reviewquery/engine.go index 2876497..453d16c 100644 --- a/internal/reviewquery/engine.go +++ b/internal/reviewquery/engine.go @@ -9,6 +9,9 @@ import ( ) // QueryResult is a generic tabular result: column headers + stringified rows. +// Invariant: every row in Rows has exactly len(Columns) cells. storage.QueryRows +// guarantees this when building a QueryResult; preserve it in any other +// construction path (formatters rely on it). type QueryResult struct { Columns []string Rows [][]string diff --git a/internal/reviewquery/engine_test.go b/internal/reviewquery/engine_test.go index a245009..716dff7 100644 --- a/internal/reviewquery/engine_test.go +++ b/internal/reviewquery/engine_test.go @@ -56,6 +56,49 @@ func TestRunOnRecordsEmpty(t *testing.T) { } } +func TestValidateReadOnlySQL(t *testing.T) { + valid := []string{ + "SELECT 1", + " select * from review_log ", + "SELECT * FROM review_log;", + "SELECT * FROM review_log ; ", + "With x AS (SELECT 1) SELECT * FROM x", + "select action, count(*) from review_log group by action", + } + for _, sqlText := range valid { + t.Run("valid: "+sqlText, func(t *testing.T) { + if err := validateReadOnlySQL(sqlText); err != nil { + t.Errorf("validateReadOnlySQL(%q) = %v; want nil", sqlText, err) + } + }) + } + + invalid := []string{ + "", + " ", + "DROP TABLE review_log", + "DELETE FROM review_log", + "INSERT INTO review_log VALUES (1)", + "UPDATE review_log SET action='x'", + "CREATE TABLE evil (x)", + "ALTER TABLE review_log ADD COLUMN x", + "ATTACH DATABASE '/tmp/evil.db' AS evil", + "PRAGMA writable_schema=1", + "REPLACE INTO review_log VALUES (1)", + "SELECT 1; DROP TABLE review_log", + "SELECT 1;DROP TABLE review_log", + "SELECT 1; SELECT 2", + "-- comment\nSELECT 1", + } + for _, sqlText := range invalid { + t.Run("invalid: "+sqlText, func(t *testing.T) { + if err := validateReadOnlySQL(sqlText); err == nil { + t.Errorf("validateReadOnlySQL(%q) = nil; want an error", sqlText) + } + }) + } +} + // BenchmarkRunOnRecords measures load+query cost at various repo sizes. // Run: go test -run=^$ -bench=RunOnRecords -benchmem ./internal/reviewquery/ func BenchmarkRunOnRecords(b *testing.B) { diff --git a/storage/sqlite_query_io.go b/storage/sqlite_query_io.go index 736d9b3..2977de0 100644 --- a/storage/sqlite_query_io.go +++ b/storage/sqlite_query_io.go @@ -9,7 +9,9 @@ import ( // OpenInMemorySQLite opens a fresh in-memory sqlite database via the storage // boundary. Used by the review-query engine to build an ephemeral table that is -// discarded when the handle is closed. +// discarded when the handle is closed. The caller owns the returned handle and +// must Close() it; the in-memory database (and all its data) is destroyed once +// the last connection closes. func OpenInMemorySQLite() (*sql.DB, error) { db, err := sql.Open("sqlite", ":memory:") if err != nil {