Conversation
WalkthroughAdds a new dawgrun interactive REPL and embedded go-repl library, including command registry and handlers for Cypher parse/translate/query, Postgres connection and kind-map management, runtime tracing, build/test automation, docs, license, and related tooling manifests. Changes
Sequence DiagramsequenceDiagram
participant User
participant REPL as REPL Loop
participant Registry as Command Registry
participant Cmd as Command Handler
participant Parser as Cypher Parser
participant Translator as SQL Translator
participant DB as Postgres DB
participant Formatter as Result Formatter
User->>REPL: Enter line
REPL->>REPL: shlex split, normalize
REPL->>Registry: Lookup command
Registry-->>REPL: Command descriptor
REPL->>Cmd: Execute with CommandContext
alt queryCypherCmd
Cmd->>Parser: Parse Cypher
Parser-->>Cmd: AST
Cmd->>Translator: Translate to SQL (use kind map if provided)
Translator-->>Cmd: SQL
Cmd->>DB: Execute in read transaction
DB-->>Cmd: Rows
Cmd->>Formatter: Format (table/json)
Formatter-->>Cmd: Output
else translate/explain
Cmd->>Parser: Parse Cypher
Parser-->>Cmd: AST
Cmd->>Translator: Translate to SQL/AST
Translator-->>Cmd: SQL/AST
Cmd->>DB: (EXPLAIN) execute if requested
DB-->>Cmd: Explain rows
end
Cmd-->>REPL: Return output + warnings
REPL->>REPL: Save history
REPL-->>User: Render output
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~80 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
seanjSO
force-pushed
the
seanj/dawgrun
branch
from
b77a170 to
083da4e
Compare
Contributor
|
I see this is in draft but do we want to pull this in any time soon? |
Contributor
Author
|
@zinic for sure! Dillon has me working on an RFC right now to get in front of the eng council to solidify the whole thing, so Very Soon™ |
seanjSO
force-pushed
the
seanj/dawgrun
branch
from
17360c3 to
842750b
Compare
seanjSO
force-pushed
the
seanj/dawgrun
branch
2 times, most recently
from
f50705c to
589cc4e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (6)
tools/dawgrun/pkg/stubs/kindmapper.go (1)
76-90: Minor:GetKindByID/GetIDByKindaren't part ofpgsql.KindMapperand use an awkward inverted-ifidiom.Two small notes:
- Per the interface in
cypher/models/pgsql/model.go:15-18, onlyMapKindsandAssertKindsare required.GetKindByID/GetIDByKindare exported helpers specific to this stub — fine, but worth a doc comment clarifying they exist for thelookup-kind/lookup-kind-idREPL commands (tools/dawgrun/pkg/commands/db.go).- The
if _, ok := …; !ok { return err } else { return val }pattern inverts the happy path. Idiomatic Go flips this:♻️ Proposed refactor
func (k *DumbKindMapper) GetKindByID(id int16) (graph.Kind, error) { - if kind, ok := k.idToKind[id]; !ok { - return nil, fmt.Errorf("%w: %d", ErrNoSuchKindID, id) - } else { - return kind, nil - } + kind, ok := k.idToKind[id] + if !ok { + return nil, fmt.Errorf("%w: %d", ErrNoSuchKindID, id) + } + return kind, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/stubs/kindmapper.go` around lines 76 - 90, GetKindByID and GetIDByKind are exported helper methods on DumbKindMapper used by the REPL but lack doc comments and currently use an inverted-if idiom; add a short doc comment to each method explaining they are helpers used by the lookup-kind / lookup-kind-id REPL commands and refactor both methods (GetKindByID and GetIDByKind) to the idiomatic Go form that checks the positive/happy path first (e.g., if kind, ok := k.idToKind[id]; ok { return kind, nil } return nil, fmt.Errorf(...)) and similarly for kindToID to return the found value immediately and only construct the error on the failure path.tools/dawgrun/pkg/go-repl/go.mod (1)
1-8: Vendored upstream module manifest — acceptable as-is.This mirrors the upstream
github.com/openengineer/go-replmanifest (go 1.15, Unlicense) and is used solely via thereplaceintools/dawgrun/go.mod. No changes required here; however, since the library is now effectively a fork living in-tree, consider whether you want to bumpgo 1.15to something closer to the toolchain used bytools/dawgrun(1.25.4) to avoid confusing IDE/tooling behavior when editing these files directly. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/go-repl/go.mod` around lines 1 - 8, The vendored go.mod currently declares "module github.com/openengineer/go-repl" with "go 1.15"; leave it as-is if you want an exact upstream mirror, or update the Go version directive from "go 1.15" to the toolchain version you use (e.g., "go 1.25" or "go 1.25.4") to avoid IDE/tooling confusion when editing this in-tree—edit the go directive in the go.mod (the "go 1.15" token) accordingly while keeping the module name and require block unchanged.tools/dawgrun/README.md (1)
37-180: Optional: normalize code-block style to fenced.
markdownlint-cli2flags mixed indented (MD046) and unlabeled fenced blocks (MD040) throughout. Not functionally important, but rendering engines (GitHub vs some doc viewers) handle indented blocks inconsistently, and command lines get no syntax highlighting. Consider converting all thedawgrun > …blocks to```text(or```shellwhere appropriate) and tagging lines 1 and 66 with a language hint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/README.md` around lines 37 - 180, Normalize the README's example command blocks that are currently indented (the many " dawgrun > ..." examples) to fenced code blocks using a language hint (use ```text for generic command output and ```shell where appropriate) so markdownlint MD046/MD040 warnings are resolved; convert each indented block into a fenced block and ensure the two prominent example blocks (the top usage example and the Parse/Translate examples) include an explicit language tag to enable consistent rendering and optional highlighting.tools/dawgrun/pkg/go-repl/LICENSE (1)
1-24: LGTM — Unlicense preserved verbatim from upstream.Since this is a fork of
github.com/openengineer/go-repl, consider adding a smallNOTICEor top-of-file comment in one of the.gofiles recording the upstream commit you forked from; helps future maintainers when rebasing patches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/go-repl/LICENSE` around lines 1 - 24, Add a short provenance note (e.g., a NOTICE or top-of-file comment) recording the upstream repo and commit you forked from (github.com/openengineer/go-repl at <commit>) alongside the existing LICENSE; place it as a comment above the package declaration in the primary Go source (e.g., above the package line in the main .go file or the package where the REPL is implemented) so future maintainers can find the upstream commit easily while preserving the Unlicense verbatim in LICENSE.tools/dawgrun/pkg/commands/registry.go (1)
22-24: Avoid returning the mutable registry map.
Registry()exposes the live package map, so any caller can delete or replace commands globally. Return a clone unless external mutation is an intentional plugin API.Defensive-copy option
package commands + +import "maps" @@ func Registry() map[string]CommandDesc { - return cmdRegistry + return maps.Clone(cmdRegistry) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/commands/registry.go` around lines 22 - 24, Registry() currently returns the live cmdRegistry map which allows callers to mutate package state; change Registry to return a defensive copy: create a new map[string]CommandDesc, iterate over cmdRegistry and copy each entry into the new map, and return that new map instead of cmdRegistry (if CommandDesc is a pointer or contains mutable fields, also clone or copy its value before inserting to avoid exposing internal mutability).tools/dawgrun/justfile (1)
6-12: Keepbuildfrom mutating the working tree.Because
builddepends onvet, every build/debug run can rewritego.mod,go.sum, and formatted source viago mod tidy/go fmt. Consider splitting this into an explicitcheck/fixrecipe and makingbuildonly compile.♻️ Proposed recipe split
-vet: +fix: go mod tidy go fmt ./... + +vet: go vet ./... -build *BUILDARGS: vet +build *BUILDARGS: go build -o dawgrun {{ BUILDARGS }} ./cmd/...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/justfile` around lines 6 - 12, The current justfile makes the build recipe depend on vet, which runs go mod tidy and go fmt and thus mutates go.mod/go.sum and source; split responsibilities by creating separate recipes (e.g., check and fix) where check runs go vet/go list checks and fix runs go mod tidy and go fmt, then change the build recipe (build *BUILDARGS) to depend only on compilation (remove the vet dependency) so running build won’t rewrite files; update references to the existing vet recipe name or replace it with the new check/fix recipes as appropriate to preserve CI/lint flows and to keep build purely compiling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 39-41: The go.mod contains a tool directive for
github.com/specterops/dawgs/tools/dawgrun/cmd/dawgrun but is missing the
required module entry; add a require statement for
github.com/specterops/dawgs/tools/dawgrun/cmd/dawgrun v0.0.0 alongside the
existing replace and tool lines so the module is included in the module graph
and resolves for go mod tidy and other operations.
In `@tools/dawgrun/cmd/dawgrun/main.go`:
- Around line 61-63: The log call in the handler.r.Loop error branch is using
fmt-style "%w" which slog won't expand; change the call to pass a plain message
and the error as an attribute instead (e.g., replace slog.Error("repl
encountered error: %w", slog.String("error", err.Error())) with slog.Error("repl
encountered error", slog.String("error", err.Error())) or slog.Error("repl
encountered error", slog.Any("error", err)) at the handler.r.Loop error handling
site so the error is recorded as a structured attribute.
In `@tools/dawgrun/go.mod`:
- Line 36: Update the pinned indirect dependency for github.com/jackc/pgx/v5 in
go.mod from v5.8.0 to v5.9.2 to address the CRITICAL GHSA-9jj7-4m8r-rfcm and the
SQL-injection GHSA-j88v-2chj-qfwx; after changing the version string for the
module declaration github.com/jackc/pgx/v5, run go mod tidy to refresh the
module graph, run the test suite and any CI checks, and if the transitive
dependency originates from github.com/specterops/dawgs coordinate with that repo
to ensure no incompatible pin remains.
In `@tools/dawgrun/pkg/commands/cypher.go`:
- Around line 252-315: The JSON path currently accumulates all rows in jsonRows
causing unbounded memory use; change it to stream the JSON array directly to
ctx.output: before iterating with result.Next() write a '[' when outputFormat ==
queryCypherOutputFormatJSON, then inside the loop call
buildCypherResultJSONRow(values) and marshal that single row (or use
json.Encoder with SetIndent) and write it immediately, prefixing with a comma
for rowCount>0; after the loop write ']' and a newline (and handle the zero-row
case to emit "[]\n" as before). Remove the jsonRows slice usage and ensure the
existing result.Error() check and rowCount logic around the loop still apply so
errors and "(0 rows)"/count printing behave correctly.
- Around line 187-190: The loop currently discards the error returned by
graph.ScanNextResult when iterating result.Next(); change it to capture the
error (e.g., err := graph.ScanNextResult(result, &value)) and handle it instead
of ignoring it: if err is non-nil, write a concise error message to the command
output or error stream (using ctx.output or an appropriate logger) and skip the
malformed row (continue) or break out if unrecoverable, otherwise print the
value as before; update the block around result.Next(), graph.ScanNextResult,
value, and ctx.output to implement this error check and handling.
In `@tools/dawgrun/pkg/commands/db.go`:
- Around line 21-42: The command handler currently takes only fields[1] as
connStr (losing tokens) and prints the full connection string (exposing
secrets); instead, build the connection string by joining all remaining fields
(e.g., connStr := strings.Join(fields[1:], " ")), use that joined connStr when
calling dawgsPg.NewPool and dawgs.Open, and stop echoing the connection string
to output—only report the connection name (use fmt.Fprintf(ctx.output, "Opened
connection '%s'\n", name)) while storing the joined connStr-backed query in
ctx.scope.connections[name].
In `@tools/dawgrun/pkg/commands/helpers.go`:
- Around line 26-30: The error path in highlightText currently echoes the full
source `text` which may expose sensitive or huge payloads; update the error
returned from the `quick.Highlight` failure (inside function highlightText) to
omit the raw `text` and instead include contextual details such as the `lexer`
and `style` parameters (and optionally the text length) in the fmt.Errorf
message so callers get useful debug info without leaking the source content.
In `@tools/dawgrun/pkg/commands/runtime.go`:
- Around line 68-82: The stop branch incorrectly sets state["run"] = true after
calling trace.Stop() and closing traceFile, causing subsequent "runtime-trace
start" to think tracing is still running; update the stop handling in the
runtime command (the case "stop" block that calls trace.Stop(), inspects
state["tracefile"] and closes traceFile) to set state["run"] = false (and keep
state["tracefile"] = nil) so the runtime trace state correctly reflects that
tracing is stopped.
- Around line 54-61: The created trace file (traceFile from os.Create) is left
open if trace.Start(traceFile) returns an error; modify the runtime.go code
around the os.Create and trace.Start calls so that if trace.Start(...) fails you
close traceFile before returning (e.g., call traceFile.Close() on error), and
consider establishing a single defer to close traceFile only when appropriate
(or close on the error path) to avoid leaking the file descriptor; locate the
os.Create(...) assignment to traceFile and the trace.Start(traceFile) call to
implement this change.
In `@tools/dawgrun/pkg/commands/types.go`:
- Around line 47-50: The Scope struct's maps (connections and connKindMaps) are
accessed concurrently (e.g., NumConns reads connections) and must be protected
to avoid concurrent read/write panics; add a sync.RWMutex (or sync.Mutex) field
to Scope and update all direct accesses and mutating code paths to use the mutex
(use RLock/RUnlock for readers like NumConns and Lock/Unlock for writers that
add/remove entries), or expose new locked accessor/mutator methods on Scope
(e.g., GetNumConns, AddConnection, RemoveConnection, GetKindMap) and refactor
callers to use those methods so all reads/writes of connections and connKindMaps
are synchronized.
- Around line 108-115: The current CommandOutput.WriteHighlightedWithStyle logs
the entire raw text on highlight failure (via slog.String("text", text)); change
this so the raw content is not emitted to logs: in the error branch keep the
error and any safe metadata (e.g., slog.String("error", err.Error()) and
optionally slog.Int("text_len", len(text)) or a short checksum), remove or
replace slog.String("text", text), and still fall back to
co.outputBuilder.WriteString(text). Update the error logging call in
WriteHighlightedWithStyle (and any related paths that call highlightText) to
avoid including the full `text` value.
In `@tools/dawgrun/pkg/go-repl/ansi.go`:
- Line 8: The ANSI helper functions in ansi.go are unconditional and break on
Windows; gate them by platform: add a build tag (e.g., //go:build !windows) to
ansi.go (package go-repl) and provide a companion ansi_windows.go that
implements the same exported helper function names as no-ops/Windows-safe
variants, or alternatively wrap ANSI use with a runtime.GOOS == "windows" check
and fallback to non-ANSI behavior; ensure the function names/signatures remain
identical so callers compile on all platforms and update package docs to state
Unix-only if you choose not to implement Windows support.
In `@tools/dawgrun/pkg/go-repl/examples/basic_repl.go`:
- Around line 24-33: The two example files both define func main (basic_repl.go
using MyHandler with h.r = repl.NewRepl(h) and calling h.r.Loop()) and
shell_wrapper.go which causes a redeclared main error; resolve by separating
examples into distinct packages or adding build tags: either move basic_repl.go
and shell_wrapper.go into their own subdirectories (each with package main and
its own main) or add top-of-file build tags (e.g., //go:build example_basic_repl
and //go:build example_shell_wrapper) so only the intended example is built;
update imports/comments as needed so MyHandler, repl.NewRepl and Loop remain
reachable in each example.
In `@tools/dawgrun/pkg/go-repl/Makefile`:
- Around line 1-15: The Makefile's command targets all, test, and
test-shell_wrapper should be declared .PHONY and a clean target added to remove
generated example binaries; update the file to add a .PHONY line that lists all,
test, and test-shell_wrapper (and any other non-file targets you add), and add a
clean target that removes built binaries produced by the pattern rule (e.g.,
delete ./examples/basic_repl and ./examples/shell_wrapper or use a wildcard like
./examples/* for generated executables), keeping the existing pattern rule
./examples/% unchanged.
In `@tools/dawgrun/pkg/go-repl/repl.go`:
- Around line 1557-1570: cleanAndAddToBuffer currently drops every byte >=127
which corrupts non-ASCII input; update it to be rune-aware by decoding msg as
runes (e.g. for _, rr := range string(msg)) instead of iterating bytes, replace
tabs with a space, and keep non-control runes (use unicode.IsControl(rr) to
filter out control characters rather than filtering by byte value), then
re-encode the kept runes to UTF-8 bytes and call
r.addBytesToBuffer(filteredBytes); reference cleanAndAddToBuffer, msg and
r.addBytesToBuffer when making the change and add the unicode import as needed.
- Around line 180-183: The slice truncation in repl.go currently keeps the
oldest entries (r.history = r.history[0:r.historyMaxLines]) and discards recent
commands; change the truncation to keep the newest entries by slicing the tail
of r.history (use r.history[len(r.history)-r.historyMaxLines:] when
len(r.history) > r.historyMaxLines), and add a guard for non-positive
r.historyMaxLines to avoid panics; update the logic around r.history and
r.historyMaxLines accordingly.
- Around line 231-251: The goroutine in notifySizeChange currently mutates Repl
fields and calls resize()/force()/writeStatus() concurrently with dispatch(),
causing races; change this to send size events to the main input loop instead:
add a resize channel (e.g., resizeCh chan struct{} or chan struct{w,h int}) on
the Repl struct, have notifySizeChange only detect size changes and send a
message on resizeCh (do not call resize()/force()/writeStatus() or mutate
r.width/r.height there), and modify the main loop (where dispatch() runs) to
select on resizeCh and perform r.resize(newW,newH) and any subsequent
force()/writeStatus() there so all shared-state mutations and terminal I/O
happen serialized in the main loop.
- Around line 1719-1722: The slice and loop use negative widths when right is
longer than terminal width w; before doing left = left[0 : w-len(right)] and the
for loop, clamp the available width to a non-negative value (e.g., avail := w -
len(right); if avail < 0 { avail = 0 }) and use that clamped avail for slicing
and for-loop bounds so left is never sliced with a negative index and the loop
never iterates with a negative count; update the code around r.statusFields(),
the left/right handling, and the for i := 0; ... loop to use this clamped avail
variable.
In `@tools/dawgrun/pkg/go-repl/stdinreader.go`:
- Around line 64-90: The goroutine that reads bytes from r.reader has three
issues: protect r.lastTime reads and writes with r.lock (i.e., wrap accesses to
r.lastTime in the same mutex used in start()), protect r.reader
mutations/readers by performing the nil assignment under r.lock or convert
r.reader to an atomic.Pointer and use atomic loads/stores so the main loop and
goroutine cannot race, and replace panic(err) on ReadByte with graceful handling
(treat io.EOF as clean shutdown and send other errors back via an error channel
or the existing bytes channel instead of panicking); update the goroutine around
ReadByte, the stopNow logic, and the place that checks r.reader (the tight loop
calling r.reader.read()) to use the same synchronization approach.
In `@tools/dawgrun/pkg/stubs/kindmapper.go`:
- Around line 45-51: MapperFromKindMap currently seeds DumbKindMapper.lastID to
-1 and stores kindMap by reference, which causes ID collisions and mutates the
caller's map when AssertKinds assigns new IDs; fix by making a defensive copy of
the incoming KindMap for idToKind, build kindToID from that copy (using
kindMap.Invert() or re-computing), and initialize lastID to the maximum key
present in the copied idToKind (or -1 if empty) so subsequent AssertKinds will
allocate new IDs above the existing range without overwriting existing entries.
---
Nitpick comments:
In `@tools/dawgrun/justfile`:
- Around line 6-12: The current justfile makes the build recipe depend on vet,
which runs go mod tidy and go fmt and thus mutates go.mod/go.sum and source;
split responsibilities by creating separate recipes (e.g., check and fix) where
check runs go vet/go list checks and fix runs go mod tidy and go fmt, then
change the build recipe (build *BUILDARGS) to depend only on compilation (remove
the vet dependency) so running build won’t rewrite files; update references to
the existing vet recipe name or replace it with the new check/fix recipes as
appropriate to preserve CI/lint flows and to keep build purely compiling.
In `@tools/dawgrun/pkg/commands/registry.go`:
- Around line 22-24: Registry() currently returns the live cmdRegistry map which
allows callers to mutate package state; change Registry to return a defensive
copy: create a new map[string]CommandDesc, iterate over cmdRegistry and copy
each entry into the new map, and return that new map instead of cmdRegistry (if
CommandDesc is a pointer or contains mutable fields, also clone or copy its
value before inserting to avoid exposing internal mutability).
In `@tools/dawgrun/pkg/go-repl/go.mod`:
- Around line 1-8: The vendored go.mod currently declares "module
github.com/openengineer/go-repl" with "go 1.15"; leave it as-is if you want an
exact upstream mirror, or update the Go version directive from "go 1.15" to the
toolchain version you use (e.g., "go 1.25" or "go 1.25.4") to avoid IDE/tooling
confusion when editing this in-tree—edit the go directive in the go.mod (the "go
1.15" token) accordingly while keeping the module name and require block
unchanged.
In `@tools/dawgrun/pkg/go-repl/LICENSE`:
- Around line 1-24: Add a short provenance note (e.g., a NOTICE or top-of-file
comment) recording the upstream repo and commit you forked from
(github.com/openengineer/go-repl at <commit>) alongside the existing LICENSE;
place it as a comment above the package declaration in the primary Go source
(e.g., above the package line in the main .go file or the package where the REPL
is implemented) so future maintainers can find the upstream commit easily while
preserving the Unlicense verbatim in LICENSE.
In `@tools/dawgrun/pkg/stubs/kindmapper.go`:
- Around line 76-90: GetKindByID and GetIDByKind are exported helper methods on
DumbKindMapper used by the REPL but lack doc comments and currently use an
inverted-if idiom; add a short doc comment to each method explaining they are
helpers used by the lookup-kind / lookup-kind-id REPL commands and refactor both
methods (GetKindByID and GetIDByKind) to the idiomatic Go form that checks the
positive/happy path first (e.g., if kind, ok := k.idToKind[id]; ok { return
kind, nil } return nil, fmt.Errorf(...)) and similarly for kindToID to return
the found value immediately and only construct the error on the failure path.
In `@tools/dawgrun/README.md`:
- Around line 37-180: Normalize the README's example command blocks that are
currently indented (the many " dawgrun > ..." examples) to fenced code blocks
using a language hint (use ```text for generic command output and ```shell where
appropriate) so markdownlint MD046/MD040 warnings are resolved; convert each
indented block into a fenced block and ensure the two prominent example blocks
(the top usage example and the Parse/Translate examples) include an explicit
language tag to enable consistent rendering and optional highlighting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bbc0c2b-3199-4d67-b0c9-01c090c51615
⛔ Files ignored due to path filters (3)
go.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumtools/dawgrun/go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
go.modtools/dawgrun/.gitignoretools/dawgrun/README.mdtools/dawgrun/cmd/dawgrun/art.txttools/dawgrun/cmd/dawgrun/banner.txttools/dawgrun/cmd/dawgrun/main.gotools/dawgrun/go.modtools/dawgrun/justfiletools/dawgrun/pkg/commands/completion.gotools/dawgrun/pkg/commands/cypher.gotools/dawgrun/pkg/commands/db.gotools/dawgrun/pkg/commands/help.gotools/dawgrun/pkg/commands/helpers.gotools/dawgrun/pkg/commands/registry.gotools/dawgrun/pkg/commands/runtime.gotools/dawgrun/pkg/commands/types.gotools/dawgrun/pkg/go-repl/.gitignoretools/dawgrun/pkg/go-repl/LICENSEtools/dawgrun/pkg/go-repl/Makefiletools/dawgrun/pkg/go-repl/README.mdtools/dawgrun/pkg/go-repl/ansi.gotools/dawgrun/pkg/go-repl/examples/basic_repl.gotools/dawgrun/pkg/go-repl/examples/shell_wrapper.gotools/dawgrun/pkg/go-repl/go.modtools/dawgrun/pkg/go-repl/handler.gotools/dawgrun/pkg/go-repl/repl.gotools/dawgrun/pkg/go-repl/repl_popover_test.gotools/dawgrun/pkg/go-repl/stdinreader.gotools/dawgrun/pkg/stubs/kindmapper.go
Comment on lines
+1557
to
+1570
| func (r *Repl) cleanAndAddToBuffer(msg []byte) { | ||
| // remove bad chars | ||
| // XXX: what about unicode? | ||
| filtered := make([]byte, 0) | ||
|
|
||
| for _, c := range msg { | ||
| if c == '\t' { | ||
| filtered = append(filtered, ' ') | ||
| } else if c >= 32 && c < 127 { | ||
| filtered = append(filtered, c) | ||
| } | ||
| } | ||
|
|
||
| r.addBytesToBuffer(filtered) |
There was a problem hiding this comment.
Do not silently drop non-ASCII query input.
cleanAndAddToBuffer removes every byte >= 127, so pasted Cypher containing Unicode labels, properties, or string literals is corrupted before execution.
Do you want me to draft a rune-aware input/rendering change so Unicode is preserved correctly?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dawgrun/pkg/go-repl/repl.go` around lines 1557 - 1570,
cleanAndAddToBuffer currently drops every byte >=127 which corrupts non-ASCII
input; update it to be rune-aware by decoding msg as runes (e.g. for _, rr :=
range string(msg)) instead of iterating bytes, replace tabs with a space, and
keep non-control runes (use unicode.IsControl(rr) to filter out control
characters rather than filtering by byte value), then re-encode the kept runes
to UTF-8 bytes and call r.addBytesToBuffer(filteredBytes); reference
cleanAndAddToBuffer, msg and r.addBytesToBuffer when making the change and add
the unicode import as needed.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
tools/dawgrun/pkg/commands/runtime.go (2)
54-61:⚠️ Potential issue | 🟠 MajorClose
traceFilewhentrace.Startfails.On Line 59-Line 60,
traceFilestays open if tracing fails to start, leaking the descriptor.🐛 Proposed fix
traceFile, err := os.Create(traceOut) if err != nil { return fmt.Errorf("error creating tracefile: %w", err) } if err := trace.Start(traceFile); err != nil { + _ = traceFile.Close() return fmt.Errorf("could not start tracing: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/commands/runtime.go` around lines 54 - 61, When creating traceFile and calling trace.Start(traceFile) in runtime.go, ensure traceFile is closed if trace.Start returns an error to avoid leaking the file descriptor; update the block around traceFile and trace.Start to call traceFile.Close() (or defer a conditional close) before returning the fmt.Errorf when trace.Start fails, referencing the traceFile variable and the trace.Start(...) call so the file is always closed on error.
68-82:⚠️ Potential issue | 🟠 Major
stopleaves runtime-trace marked as running.After stopping tracing, Line 81 sets
state["run"] = true; this makes laterstartcalls incorrectly fail as “already enabled.”🐛 Proposed fix
traceFile, ok := state["tracefile"].(*os.File) if !ok { return fmt.Errorf("could not get open tracing file") } - traceFile.Close() + if err := traceFile.Close(); err != nil { + state["run"] = false + state["tracefile"] = nil + return fmt.Errorf("could not close tracing file: %w", err) + } - state["run"] = true + state["run"] = false state["tracefile"] = nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/commands/runtime.go` around lines 68 - 82, In the "stop" case handling (the switch branch labeled case "stop") the code sets state["run"] = true after stopping tracing which incorrectly leaves the runtime marked as running; change that assignment to state["run"] = false and keep state["tracefile"] = nil after calling trace.Stop() and traceFile.Close() so subsequent calls to the start path correctly detect tracing is stopped (refer to symbols: case "stop", trace.Stop(), traceFile.Close(), state["run"], state["tracefile"]).tools/dawgrun/pkg/go-repl/stdinreader.go (1)
56-87:⚠️ Potential issue | 🔴 CriticalSynchronize shared reader state and handle stdin EOF without panic.
r.reader/r.lastTimeare accessed both with and withoutr.lock(Line 57, Line 72, Line 77, Line 86), which introduces races. Also,panic(err)on Line 68 will crash the REPL on normal stdin closure paths.🐛 Proposed fix
import ( "bufio" + "errors" + "io" "os" "sync" "time" ) @@ func (r *_StdinReader) read() { - if r.reader != nil { + r.lock.Lock() + if r.reader != nil { + r.lock.Unlock() return } - r.reader = bufio.NewReader(os.Stdin) + reader := bufio.NewReader(os.Stdin) + r.reader = reader r.lastTime = time.Now() + r.lock.Unlock() - go func() { + go func(reader *bufio.Reader) { for { - b, err := r.reader.ReadByte() + b, err := reader.ReadByte() if err != nil { - panic(err) + r.lock.Lock() + r.reader = nil + r.lock.Unlock() + if errors.Is(err, io.EOF) { + return + } + return } - stopNow := false - if b == 13 && time.Now().After(r.lastTime.Add(MACHINE_INTERVAL)) { - // it is unlikely that a carriage return followed by some text is pasted into the terminal, so we can use this as a queu to quit - stopNow = true - } - - r.lastTime = time.Now() - r.lock.Lock() - + stopNow := b == 13 && time.Now().After(r.lastTime.Add(MACHINE_INTERVAL)) + r.lastTime = time.Now() r.buffer = append(r.buffer, b) - - r.lock.Unlock() - if stopNow { r.reader = nil + r.lock.Unlock() return } + r.lock.Unlock() } - }() + }(reader) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/go-repl/stdinreader.go` around lines 56 - 87, The _StdinReader.read goroutine currently races on r.reader and r.lastTime and panics on normal stdin EOF; fix by guarding all accesses and mutations of r.reader and r.lastTime with r.lock (e.g., reading r.reader at start, checking/updating r.lastTime before/after ReadByte, and setting r.reader = nil inside the same lock) and by replacing panic(err) with proper EOF/error handling (treat io.EOF as a normal stop: clear reader under lock and return; for other errors log/propagate instead of panicking). Ensure the stopNow logic that checks MACHINE_INTERVAL still reads r.lastTime under r.lock so the comparison is race-free and the goroutine exits cleanly.tools/dawgrun/pkg/commands/db.go (1)
26-42:⚠️ Potential issue | 🟠 MajorPreserve full connection string input and stop printing secrets.
Line 27 only takes
fields[1], which drops remaining tokens. Also, Line 38 and Line 41 expose the raw connection string (potential credentials) in output/errors.🐛 Proposed fix
import ( "fmt" "strconv" + "strings" @@ name := fields[0] - connStr := fields[1] + connStr := strings.Join(fields[1:], " ") connPool, err := dawgsPg.NewPool(connStr) if err != nil { return fmt.Errorf("error opening connection pool: %w", err) } @@ }) if err != nil { - return fmt.Errorf("error opening database connection '%s': %w", connStr, err) + return fmt.Errorf("error opening database connection %q: %w", name, err) } - fmt.Fprintf(ctx.output, "Opened connection '%s': %s\n", name, connStr) + fmt.Fprintf(ctx.output, "Opened connection '%s'\n", name) ctx.scope.connections[name] = query🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/commands/db.go` around lines 26 - 42, The code slices the connection string with fields[1] (losing tokens) and prints the raw connection string in logs/errors; fix by reassembling the full connection string from remaining tokens (e.g., connStr := strings.Join(fields[1:], " ")) before calling dawgsPg.NewPool and dawgs.Open, and avoid emitting secrets: change fmt.Fprintf(ctx.output, ...) to only show the connection name (or a masked version) and update error returns from dawgs.Open/dawgsPg.NewPool to reference the connection name (or masked value) instead of including connStr; keep storing the query in ctx.scope.connections[name] as before.tools/dawgrun/cmd/dawgrun/main.go (1)
61-63:⚠️ Potential issue | 🟡 MinorUse structured slog error fields;
%win the message is not interpolated.At Line 62,
%wis treated as literal text inslog.Error, so the log message is misleading.🪵 Proposed fix
if err := handler.r.Loop(); err != nil { - slog.Error("repl encountered error: %w", slog.String("error", err.Error())) + slog.Error("repl encountered error", slog.Any("error", err)) }In Go's standard library `log/slog`, does `Logger.Error("msg: %w", ...)` interpolate `%w`, or is the message treated as a literal string?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/cmd/dawgrun/main.go` around lines 61 - 63, The log call using slog.Error with a format string ("repl encountered error: %w") treats the message as literal and does not interpolate %w; update the handler.r.Loop error logging to use structured slog fields instead—call slog.Error with a plain message like "repl encountered error" and pass the error as a field (e.g., slog.String("error", err.Error()) or slog.Any("error", err)) so the log contains the actual error; locate the call around handler.r.Loop() and replace the current slog.Error invocation accordingly.tools/dawgrun/pkg/go-repl/repl.go (4)
180-183:⚠️ Potential issue | 🟡 MinorKeep newest history entries when enforcing the limit.
This truncation keeps the oldest lines and drops the most recent ones.
🧾 Proposed fix
// truncate history buffer before save. if len(r.history) > r.historyMaxLines { - r.history = r.history[0:r.historyMaxLines] + r.history = r.history[len(r.history)-r.historyMaxLines:] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/go-repl/repl.go` around lines 180 - 183, The current truncation keeps the oldest entries and discards the newest; change the logic in the block that trims r.history to retain the most recent r.historyMaxLines entries instead of the first ones by reassigning r.history to the slice containing the last r.historyMaxLines elements (i.e., start at len(r.history)-r.historyMaxLines and take to the end), making sure to guard against negative indices when r.historyMaxLines is larger than len(r.history).
1719-1731:⚠️ Potential issue | 🔴 CriticalClamp available status width before slicing.
At Line 1721,
w-len(right)can be negative, causing a slice-bounds panic.🛡️ Proposed fix
left, right := r.statusFields() -if len(left) > w-len(right) { - left = left[0 : w-len(right)] +if len(right) > w { + right = right[:w] +} +avail := w - len(right) +if avail < 0 { + avail = 0 +} +if len(left) > avail { + left = left[:avail] } @@ -for i := 0; i < w-len(left)-len(right); i++ { +for i := 0; i < w-len(left)-len(right); i++ { fmt.Print(" ") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/go-repl/repl.go` around lines 1719 - 1731, Clamp the available status width before slicing to avoid a negative index panic: compute avail := w - len(right) and if avail < 0 set avail = 0, then use avail when truncating left (in the block that calls r.statusFields() and does left = left[0:...]) and when iterating to print padding spaces; update the uses around r.statusFields(), left/right, highlight() and the padding loop to reference avail instead of w-len(right).
231-251:⚠️ Potential issue | 🟠 MajorSerialize resize handling with the input loop.
notifySizeChange()mutates REPL state and triggers rendering concurrently withdispatch(), which risks races and garbled terminal output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/go-repl/repl.go` around lines 231 - 251, notifySizeChange currently polls and directly mutates r.width/r.height and calls r.resize from a background goroutine, causing concurrent access with dispatch(); change it to serialize resize handling through the REPL's main input/dispatch loop instead: keep getSize, but replace the background goroutine's direct r.resize call with sending a resize event (width,height) into the same event/channel that dispatch() processes (or invoke a dispatch-safe enqueue method on Repl), and let the main loop perform r.resize and state mutation so all rendering/state updates are single-threaded.
1557-1570:⚠️ Potential issue | 🟠 MajorDo not drop non-ASCII input in buffer cleaning.
The byte-range filter strips Unicode characters, which corrupts user input before evaluation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/go-repl/repl.go` around lines 1557 - 1570, The current Repl.cleanAndAddToBuffer(byte[]) drops all non-ASCII bytes; change it to preserve Unicode by decoding msg as runes (e.g., convert to string and range over runes) so you handle UTF-8 properly, replace '\t' with a space, and keep all runes >= 0x20 (and not DEL/control) instead of filtering by byte values; then re-encode the cleaned runes to bytes and call r.addBytesToBuffer(filtered) so multi-byte UTF-8 characters are not lost (update the logic inside Repl.cleanAndAddToBuffer and continue using r.addBytesToBuffer).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/dawgrun/pkg/go-repl/examples/basic_repl.go`:
- Around line 43-44: The comment above MyHandler.Eval is stale: it states there
are two return values but Eval(buffer string) returns a single string; update
the comment to accurately describe the single return (e.g., "returns stdout
output as a string" or remove the "first/second return value" phrasing) so it
correctly documents MyHandler.Eval's signature and behavior.
In `@tools/dawgrun/pkg/go-repl/examples/shell_wrapper.go`:
- Around line 75-77: The call to repl.NewRepl in ShellWrapper (h.r =
repl.NewRepl(h)) passes only the Handler argument but NewRepl requires two
parameters (handler Handler, opts *Options); update the invocation to provide
the second argument (either nil if defaults are acceptable or an Options value
such as &repl.Options{...}) so change the call to repl.NewRepl(h, nil) or
repl.NewRepl(h, &repl.Options{...}) and ensure the Options type from the repl
package is used.
In `@tools/dawgrun/pkg/go-repl/repl.go`:
- Around line 171-173: The saveHistory function can panic if r.historyFile is
nil; update saveHistory (the method on Repl that calls r.historyFile.Truncate
and subsequent operations) to early-return (no-op) when r.historyFile == nil so
it doesn't attempt to Truncate or write; locate the references to
r.historyFile.Truncate(...) and the following write/seek logic in saveHistory
and wrap them in a nil-check (or return nil) to safely handle the case where no
history file is configured.
---
Duplicate comments:
In `@tools/dawgrun/cmd/dawgrun/main.go`:
- Around line 61-63: The log call using slog.Error with a format string ("repl
encountered error: %w") treats the message as literal and does not interpolate
%w; update the handler.r.Loop error logging to use structured slog fields
instead—call slog.Error with a plain message like "repl encountered error" and
pass the error as a field (e.g., slog.String("error", err.Error()) or
slog.Any("error", err)) so the log contains the actual error; locate the call
around handler.r.Loop() and replace the current slog.Error invocation
accordingly.
In `@tools/dawgrun/pkg/commands/db.go`:
- Around line 26-42: The code slices the connection string with fields[1]
(losing tokens) and prints the raw connection string in logs/errors; fix by
reassembling the full connection string from remaining tokens (e.g., connStr :=
strings.Join(fields[1:], " ")) before calling dawgsPg.NewPool and dawgs.Open,
and avoid emitting secrets: change fmt.Fprintf(ctx.output, ...) to only show the
connection name (or a masked version) and update error returns from
dawgs.Open/dawgsPg.NewPool to reference the connection name (or masked value)
instead of including connStr; keep storing the query in
ctx.scope.connections[name] as before.
In `@tools/dawgrun/pkg/commands/runtime.go`:
- Around line 54-61: When creating traceFile and calling trace.Start(traceFile)
in runtime.go, ensure traceFile is closed if trace.Start returns an error to
avoid leaking the file descriptor; update the block around traceFile and
trace.Start to call traceFile.Close() (or defer a conditional close) before
returning the fmt.Errorf when trace.Start fails, referencing the traceFile
variable and the trace.Start(...) call so the file is always closed on error.
- Around line 68-82: In the "stop" case handling (the switch branch labeled case
"stop") the code sets state["run"] = true after stopping tracing which
incorrectly leaves the runtime marked as running; change that assignment to
state["run"] = false and keep state["tracefile"] = nil after calling
trace.Stop() and traceFile.Close() so subsequent calls to the start path
correctly detect tracing is stopped (refer to symbols: case "stop",
trace.Stop(), traceFile.Close(), state["run"], state["tracefile"]).
In `@tools/dawgrun/pkg/go-repl/repl.go`:
- Around line 180-183: The current truncation keeps the oldest entries and
discards the newest; change the logic in the block that trims r.history to
retain the most recent r.historyMaxLines entries instead of the first ones by
reassigning r.history to the slice containing the last r.historyMaxLines
elements (i.e., start at len(r.history)-r.historyMaxLines and take to the end),
making sure to guard against negative indices when r.historyMaxLines is larger
than len(r.history).
- Around line 1719-1731: Clamp the available status width before slicing to
avoid a negative index panic: compute avail := w - len(right) and if avail < 0
set avail = 0, then use avail when truncating left (in the block that calls
r.statusFields() and does left = left[0:...]) and when iterating to print
padding spaces; update the uses around r.statusFields(), left/right, highlight()
and the padding loop to reference avail instead of w-len(right).
- Around line 231-251: notifySizeChange currently polls and directly mutates
r.width/r.height and calls r.resize from a background goroutine, causing
concurrent access with dispatch(); change it to serialize resize handling
through the REPL's main input/dispatch loop instead: keep getSize, but replace
the background goroutine's direct r.resize call with sending a resize event
(width,height) into the same event/channel that dispatch() processes (or invoke
a dispatch-safe enqueue method on Repl), and let the main loop perform r.resize
and state mutation so all rendering/state updates are single-threaded.
- Around line 1557-1570: The current Repl.cleanAndAddToBuffer(byte[]) drops all
non-ASCII bytes; change it to preserve Unicode by decoding msg as runes (e.g.,
convert to string and range over runes) so you handle UTF-8 properly, replace
'\t' with a space, and keep all runes >= 0x20 (and not DEL/control) instead of
filtering by byte values; then re-encode the cleaned runes to bytes and call
r.addBytesToBuffer(filtered) so multi-byte UTF-8 characters are not lost (update
the logic inside Repl.cleanAndAddToBuffer and continue using
r.addBytesToBuffer).
In `@tools/dawgrun/pkg/go-repl/stdinreader.go`:
- Around line 56-87: The _StdinReader.read goroutine currently races on r.reader
and r.lastTime and panics on normal stdin EOF; fix by guarding all accesses and
mutations of r.reader and r.lastTime with r.lock (e.g., reading r.reader at
start, checking/updating r.lastTime before/after ReadByte, and setting r.reader
= nil inside the same lock) and by replacing panic(err) with proper EOF/error
handling (treat io.EOF as a normal stop: clear reader under lock and return; for
other errors log/propagate instead of panicking). Ensure the stopNow logic that
checks MACHINE_INTERVAL still reads r.lastTime under r.lock so the comparison is
race-free and the goroutine exits cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56c92ae0-889b-4f39-ab7f-63a27e8afa65
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumtools/dawgrun/go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
go.modtools/dawgrun/.gitignoretools/dawgrun/README.mdtools/dawgrun/cmd/dawgrun/art.txttools/dawgrun/cmd/dawgrun/banner.txttools/dawgrun/cmd/dawgrun/main.gotools/dawgrun/go.modtools/dawgrun/justfiletools/dawgrun/pkg/commands/completion.gotools/dawgrun/pkg/commands/cypher.gotools/dawgrun/pkg/commands/db.gotools/dawgrun/pkg/commands/help.gotools/dawgrun/pkg/commands/helpers.gotools/dawgrun/pkg/commands/registry.gotools/dawgrun/pkg/commands/runtime.gotools/dawgrun/pkg/commands/types.gotools/dawgrun/pkg/go-repl/.gitignoretools/dawgrun/pkg/go-repl/LICENSEtools/dawgrun/pkg/go-repl/Makefiletools/dawgrun/pkg/go-repl/README.mdtools/dawgrun/pkg/go-repl/ansi.gotools/dawgrun/pkg/go-repl/examples/basic_repl.gotools/dawgrun/pkg/go-repl/examples/shell_wrapper.gotools/dawgrun/pkg/go-repl/go.modtools/dawgrun/pkg/go-repl/handler.gotools/dawgrun/pkg/go-repl/repl.gotools/dawgrun/pkg/go-repl/repl_popover_test.gotools/dawgrun/pkg/go-repl/stdinreader.gotools/dawgrun/pkg/stubs/kindmapper.go
✅ Files skipped from review due to trivial changes (12)
- tools/dawgrun/cmd/dawgrun/art.txt
- tools/dawgrun/.gitignore
- tools/dawgrun/cmd/dawgrun/banner.txt
- tools/dawgrun/pkg/go-repl/LICENSE
- tools/dawgrun/pkg/commands/completion.go
- tools/dawgrun/pkg/go-repl/go.mod
- tools/dawgrun/go.mod
- tools/dawgrun/pkg/go-repl/README.md
- tools/dawgrun/pkg/commands/helpers.go
- tools/dawgrun/pkg/go-repl/Makefile
- tools/dawgrun/pkg/commands/types.go
- tools/dawgrun/pkg/stubs/kindmapper.go
🚧 Files skipped from review as they are similar to previous changes (5)
- tools/dawgrun/pkg/go-repl/.gitignore
- tools/dawgrun/pkg/go-repl/handler.go
- tools/dawgrun/README.md
- tools/dawgrun/pkg/commands/help.go
- tools/dawgrun/pkg/commands/cypher.go
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tools/dawgrun/pkg/go-repl/stdinreader.go (1)
37-40: ⚡ Quick winUse a ticker instead of allocating a timer every millisecond.
time.Aftercreates a new timer on every pass through this always-on 1ms loop. Atime.Tickergives the same cadence without the continuous timer churn.♻️ Suggested change
func (r *_StdinReader) start() { go func() { + ticker := time.NewTicker(MACHINE_INTERVAL) + defer ticker.Stop() + for { - <-time.After(MACHINE_INTERVAL) + <-ticker.C r.lock.Lock()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/go-repl/stdinreader.go` around lines 37 - 40, Replace the per-iteration time.After call with a single time.Ticker created outside the for loop: create ticker := time.NewTicker(MACHINE_INTERVAL) before the for { ... } loop, use <-ticker.C inside the loop instead of <-time.After(...), and ensure you call ticker.Stop() (e.g., via defer) to avoid leaking the ticker; update the anonymous goroutine in stdinreader.go accordingly.tools/dawgrun/pkg/go-repl/repl.go (3)
1366-1377: 💤 Low valueConsider logging
saveHistory()errors on quit.Line 1368 calls
saveHistory()but discards any error. If history fails to save (disk full, permissions, etc.), the user receives no feedback.💡 Optional improvement
func (r *Repl) quit() { r.clearAfterPrompt() - r.saveHistory() + if err := r.saveHistory(); err != nil { + fmt.Fprintf(os.Stderr, "\r\nWarning: could not save history: %v\r\n", err) + } fmt.Print("\n\r")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/go-repl/repl.go` around lines 1366 - 1377, Repl.quit currently ignores errors from saveHistory(); change it to capture the returned error (err := r.saveHistory()) and, if non-nil, log it to stderr or the existing logger (e.g., fmt.Fprintf(os.Stderr, "failed to save history: %v\n", err) or r.logger.Printf(...)) before proceeding to UnmakeRaw and os.Exit(0); update the function Repl.quit to check and report saveHistory() errors so users see failures (disk/permission) when quitting.
1827-1836: 💤 Low valueUnreachable return statement after infinite loop.
Line 1835 (
return nil) is dead code since theforloop on line 1827 has nobreakpath—it only exits viaquit()callingos.Exit(0).Consider either removing the unreachable return or restructuring to allow graceful shutdown without
os.Exit().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/go-repl/repl.go` around lines 1827 - 1836, The infinite for-loop that calls r.reader.read() and r.dispatch(bts) makes the final "return nil" unreachable; either remove that dead return or restructure the loop to support graceful shutdown without os.Exit. Modify the loop in repl.go (the block using r.reader.read(), r.reader.bytes, and r.dispatch) to select on the bytes channel plus a quit/ctx channel (or expose r.quit used by quit()/os.Exit) so you can break and return nil cleanly, or simply delete the unreachable "return nil" if you intend process termination via os.Exit().
246-252: 💤 Low valueConsider graceful handling when stdin is not a TTY.
term.GetSize(0)will return an error if stdin is not a terminal (e.g., piped input, CI environments). The current panic (line 248) provides no recovery path.While a REPL inherently requires a TTY, returning a sensible default or error message would improve robustness.
🛡️ Suggested improvement
getSize := func() (int, int) { w, h, err := term.GetSize(0) if err != nil { - panic(err) + // Fallback to common terminal size if not a TTY + return 80, 24 } return w, h }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/go-repl/repl.go` around lines 246 - 252, term.GetSize(0) can fail when stdin is not a TTY; instead of panicking, handle the error gracefully: in the block that calls term.GetSize(0) (currently panicking on err) either return sensible default dimensions (e.g., 80x24) and emit a non-fatal warning (using the existing logger or fmt.Fprintf(os.Stderr, ...)), or change the function signature to return an error and propagate it upstream; update callers accordingly. Ensure you reference term.GetSize(0) and replace the panic(err) with the chosen graceful fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/dawgrun/pkg/commands/db.go`:
- Around line 36-53: The current flow closes existingConn from
ctx.scope.connections[name] before calling dawgs.Open, which can leave a closed
handle and stale connKindMaps entry if the new open fails; change the logic to
call dawgs.Open first and only if it returns no error then warn/close the
existingConn and replace it via ctx.scope.AddConnection(name, querier), also
ensure any connKindMaps[name] update/migration happens atomically with the
replacement so on failure the original connection and connKindMaps remain intact
(use local variables querier and err to stage the new connection before mutating
ctx.scope or connKindMaps).
In `@tools/dawgrun/pkg/go-repl/repl.go`:
- Around line 1849-1853: Repl.UnmakeRaw currently calls r.onEnd() unguarded
which can panic if r.onEnd is nil; modify UnmakeRaw so it first checks if
r.onEnd != nil before invoking it and then set r.onEnd = nil (ensuring
idempotence if called multiple times). Locate the UnmakeRaw method on the Repl
type and add a nil-check around the call to r.onEnd to safely handle cases where
MakeRaw was never called, UnmakeRaw is called twice, or quit() already cleared
it.
- Around line 1707-1714: Replace the panic in the status-rendering branch that
checks tot and cur (the block using variables tot and cur) with a graceful
fallback: instead of panic("unexpected") return a user-friendly status like
fmt.Sprintf("%d matches", tot) and optionally emit a non-fatal debug/warning log
(e.g., via log.Printf or the package logger) indicating the unexpected state;
this avoids crashing the REPL if tot > 0 but cur == -1 while preserving visible
status output.
---
Nitpick comments:
In `@tools/dawgrun/pkg/go-repl/repl.go`:
- Around line 1366-1377: Repl.quit currently ignores errors from saveHistory();
change it to capture the returned error (err := r.saveHistory()) and, if
non-nil, log it to stderr or the existing logger (e.g., fmt.Fprintf(os.Stderr,
"failed to save history: %v\n", err) or r.logger.Printf(...)) before proceeding
to UnmakeRaw and os.Exit(0); update the function Repl.quit to check and report
saveHistory() errors so users see failures (disk/permission) when quitting.
- Around line 1827-1836: The infinite for-loop that calls r.reader.read() and
r.dispatch(bts) makes the final "return nil" unreachable; either remove that
dead return or restructure the loop to support graceful shutdown without
os.Exit. Modify the loop in repl.go (the block using r.reader.read(),
r.reader.bytes, and r.dispatch) to select on the bytes channel plus a quit/ctx
channel (or expose r.quit used by quit()/os.Exit) so you can break and return
nil cleanly, or simply delete the unreachable "return nil" if you intend process
termination via os.Exit().
- Around line 246-252: term.GetSize(0) can fail when stdin is not a TTY; instead
of panicking, handle the error gracefully: in the block that calls
term.GetSize(0) (currently panicking on err) either return sensible default
dimensions (e.g., 80x24) and emit a non-fatal warning (using the existing logger
or fmt.Fprintf(os.Stderr, ...)), or change the function signature to return an
error and propagate it upstream; update callers accordingly. Ensure you
reference term.GetSize(0) and replace the panic(err) with the chosen graceful
fallback.
In `@tools/dawgrun/pkg/go-repl/stdinreader.go`:
- Around line 37-40: Replace the per-iteration time.After call with a single
time.Ticker created outside the for loop: create ticker :=
time.NewTicker(MACHINE_INTERVAL) before the for { ... } loop, use <-ticker.C
inside the loop instead of <-time.After(...), and ensure you call ticker.Stop()
(e.g., via defer) to avoid leaking the ticker; update the anonymous goroutine in
stdinreader.go accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a5d4d0f-fc42-412f-86f9-582ec24e6cd9
📒 Files selected for processing (12)
tools/dawgrun/cmd/dawgrun/main.gotools/dawgrun/docs/RFC.mdtools/dawgrun/go.modtools/dawgrun/pkg/commands/cypher.gotools/dawgrun/pkg/commands/db.gotools/dawgrun/pkg/commands/helpers.gotools/dawgrun/pkg/commands/registry.gotools/dawgrun/pkg/commands/runtime.gotools/dawgrun/pkg/commands/types.gotools/dawgrun/pkg/go-repl/justfiletools/dawgrun/pkg/go-repl/repl.gotools/dawgrun/pkg/go-repl/stdinreader.go
✅ Files skipped from review due to trivial changes (3)
- tools/dawgrun/pkg/go-repl/justfile
- tools/dawgrun/pkg/commands/runtime.go
- tools/dawgrun/pkg/commands/helpers.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/dawgrun/pkg/go-repl/repl.go`:
- Around line 130-147: In loadOptions, make history optional by only opening the
history file when opts.HistoryFilePath is non-empty and only calling
r.loadHistory() if the file was successfully opened; likewise, only overwrite
r.historyMaxLines when opts.HistoryMaxLines is positive (keep existing default
otherwise) — update the logic around the r.historyFile assignment, the call to
r.loadHistory(), and the r.historyMaxLines assignment in Repl.loadOptions (and
keep the same behavior for any later truncation logic that uses
r.historyMaxLines).
- Around line 245-249: The getSize closure should not panic on term.GetSize
failure; change getSize to return (int, int, error) and propagate errors instead
of calling panic. In notifySizeChange, call getSize once and if it returns an
error return early (no resize notifications). In the background polling
goroutine, call getSize each tick and if it returns an error skip that iteration
(continue) rather than crashing; only send notifications when getSize succeeds.
Update all call sites in notifySizeChange to handle the new error return from
getSize.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2ca1f1b-55c0-4dd7-a968-92cbb460373c
📒 Files selected for processing (4)
tools/dawgrun/pkg/commands/db.gotools/dawgrun/pkg/commands/types.gotools/dawgrun/pkg/go-repl/repl.gotools/dawgrun/pkg/go-repl/stdinreader.go
✅ Files skipped from review due to trivial changes (1)
- tools/dawgrun/pkg/commands/db.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/dawgrun/pkg/go-repl/stdinreader.go
seanjSO
force-pushed
the
seanj/dawgrun
branch
3 times, most recently
from
ce00fa7 to
896baa0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tools/dawgrun/go.mod (1)
36-36:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpgrade
github.com/jackc/pgx/v5fromv5.8.0(critical vulns).Line 36 still pins a version flagged with critical advisories. Please bump this nested module to the patched line (at least
v5.9.2) to align with the root module’s safer pin.🔧 Proposed patch
- github.com/jackc/pgx/v5 v5.8.0 // indirect + github.com/jackc/pgx/v5 v5.9.2 // indirect#!/bin/bash set -euo pipefail # Verify pinned pgx versions in both module files. rg -n 'github.com/jackc/pgx/v5' go.mod tools/dawgrun/go.mod # Verify current advisories and first patched versions for pgx/v5. gh api graphql -f query=' { securityVulnerabilities(first: 20, ecosystem: GO, package: "github.com/jackc/pgx/v5") { nodes { advisory { ghsaId summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/go.mod` at line 36, The go.mod in tools/dawgrun still pins github.com/jackc/pgx/v5 at v5.8.0 which has critical advisories; update the module requirement in tools/dawgrun/go.mod to at least v5.9.2 (matching the root module's safer pin) by replacing the existing github.com/jackc/pgx/v5 v5.8.0 entry with github.com/jackc/pgx/v5 v5.9.2 and then run go mod tidy to reconcile dependencies (target symbols: the module line "github.com/jackc/pgx/v5" in tools/dawgrun/go.mod and subsequent go mod commands).tools/dawgrun/pkg/go-repl/stdinreader.go (1)
73-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t panic on stdin read errors; shut down reader state cleanly.
A non-EOF
ReadByteerror still panics the process. This should fail gracefully and resetr.readerso future reads can recover cleanly.🛠️ Proposed patch
b, err := r.reader.ReadByte() if err != nil { - if errors.Is(err, io.EOF) { - return - } - - panic(err) + r.lock.Lock() + r.reader = nil + r.lock.Unlock() + if errors.Is(err, io.EOF) { + return + } + return }Also applies to: 92-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/go-repl/stdinreader.go` around lines 73 - 79, Replace the panic on non-EOF ReadByte errors with graceful shutdown of the reader state: in the method that reads from stdin (the code checking err after ReadByte and touching r.reader), keep the io.EOF early return but on any other error clear/reset r.reader (e.g., set r.reader = nil) and return so future reads can recover; do the same replacement for the second occurrence of this pattern elsewhere in stdinreader.go so both read-error branches stop panicking and instead reset reader state.
🧹 Nitpick comments (1)
tools/dawgrun/pkg/commands/registry.go (1)
27-29: ⚡ Quick winAvoid exposing the mutable global registry map.
Registry()currently returns the backing map by reference, so callers can accidentally mutate global command state. Return a clone instead.♻️ Proposed patch
func Registry() map[string]CommandDesc { - return cmdRegistry + return maps.Clone(cmdRegistry) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/commands/registry.go` around lines 27 - 29, Registry() currently returns the backing map cmdRegistry by reference, allowing callers to mutate global state; change Registry() to return a cloned map by creating a new map[string]CommandDesc, iterating over cmdRegistry and copying each entry into the new map, and return that new map (leave cmdRegistry untouched). Ensure you implement this in the Registry() function so callers receive a copy rather than the original.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/dawgrun/pkg/commands/cypher.go`:
- Around line 342-356: The cypherResultTableWidth function returns the large
fallbackWidth (120) when a detected terminal width is smaller than minWidth,
causing tables to overflow on very narrow terminals; change the logic in
cypherResultTableWidth so that when term.GetSize succeeds you subtract 2 and
return width if width >= minWidth, otherwise return minWidth, and only return
fallbackWidth when term.GetSize returns an error (preserve current fallback
behavior on error). Reference the function name cypherResultTableWidth and the
constants fallbackWidth/minWidth in your change.
In `@tools/dawgrun/pkg/go-repl/README.md`:
- Around line 7-12: The README has contradictory statements about
paste/clipboard: update the README to clearly state the actual behavior (e.g.,
"OS-level paste into the input line is supported via terminal paste events, but
there is no programmatic clipboard API support"), and apply that same wording
wherever clipboard/paste is mentioned; mention the relevant knobs like
repl.Options and any status-bar/Completer notes only to clarify that this is a
terminal paste feature (not an in-app clipboard), ensuring all references are
consistent.
In `@tools/dawgrun/pkg/go-repl/stdinreader.go`:
- Around line 44-51: The code is sending on r.bytes while still holding r.lock
which can block; modify the block in the reader (the section using r.lock,
r.buffer, r.lastTime and MACHINE_INTERVAL) to copy the buffer into a local
variable and reset r.buffer while holding r.lock, then unlock and perform the
send to r.bytes (r.bytes <- msg) after releasing r.lock so the send cannot block
while the mutex is held.
In `@tools/dawgrun/pkg/stubs/kindmapper.go`:
- Around line 62-64: MapKinds must be non-mutating and must not call AssertKinds
(which creates missing kinds); change DumbKindMapper.MapKinds to perform a
read-only lookup for each kind and return an error if any kind is missing,
leaving creation to AssertKinds. Concretely: replace the AssertKinds call in
MapKinds with a non-mutating lookup loop that resolves each graph.Kind to its id
(using an existing read-only lookup helper or add one on DumbKindMapper if none
exists), collect ids into the []int16 result, and return an error as soon as any
kind is not found; keep AssertKinds as the function that creates and asserts
missing kinds. Reference: DumbKindMapper.MapKinds and
DumbKindMapper.AssertKinds; callers affected include kind.go, update.go,
relationship.go, node.go.
---
Duplicate comments:
In `@tools/dawgrun/go.mod`:
- Line 36: The go.mod in tools/dawgrun still pins github.com/jackc/pgx/v5 at
v5.8.0 which has critical advisories; update the module requirement in
tools/dawgrun/go.mod to at least v5.9.2 (matching the root module's safer pin)
by replacing the existing github.com/jackc/pgx/v5 v5.8.0 entry with
github.com/jackc/pgx/v5 v5.9.2 and then run go mod tidy to reconcile
dependencies (target symbols: the module line "github.com/jackc/pgx/v5" in
tools/dawgrun/go.mod and subsequent go mod commands).
In `@tools/dawgrun/pkg/go-repl/stdinreader.go`:
- Around line 73-79: Replace the panic on non-EOF ReadByte errors with graceful
shutdown of the reader state: in the method that reads from stdin (the code
checking err after ReadByte and touching r.reader), keep the io.EOF early return
but on any other error clear/reset r.reader (e.g., set r.reader = nil) and
return so future reads can recover; do the same replacement for the second
occurrence of this pattern elsewhere in stdinreader.go so both read-error
branches stop panicking and instead reset reader state.
---
Nitpick comments:
In `@tools/dawgrun/pkg/commands/registry.go`:
- Around line 27-29: Registry() currently returns the backing map cmdRegistry by
reference, allowing callers to mutate global state; change Registry() to return
a cloned map by creating a new map[string]CommandDesc, iterating over
cmdRegistry and copying each entry into the new map, and return that new map
(leave cmdRegistry untouched). Ensure you implement this in the Registry()
function so callers receive a copy rather than the original.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82c73aed-c8b7-468b-99b7-88e9d3923b8d
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumtools/dawgrun/go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
go.modtools/dawgrun/.gitignoretools/dawgrun/README.mdtools/dawgrun/cmd/dawgrun/art.txttools/dawgrun/cmd/dawgrun/banner.txttools/dawgrun/cmd/dawgrun/main.gotools/dawgrun/docs/RFC.mdtools/dawgrun/go.modtools/dawgrun/justfiletools/dawgrun/pkg/commands/cypher.gotools/dawgrun/pkg/commands/db.gotools/dawgrun/pkg/commands/help.gotools/dawgrun/pkg/commands/helpers.gotools/dawgrun/pkg/commands/registry.gotools/dawgrun/pkg/commands/runtime.gotools/dawgrun/pkg/commands/types.gotools/dawgrun/pkg/go-repl/.gitignoretools/dawgrun/pkg/go-repl/LICENSEtools/dawgrun/pkg/go-repl/README.mdtools/dawgrun/pkg/go-repl/ansi.gotools/dawgrun/pkg/go-repl/go.modtools/dawgrun/pkg/go-repl/handler.gotools/dawgrun/pkg/go-repl/justfiletools/dawgrun/pkg/go-repl/repl.gotools/dawgrun/pkg/go-repl/repl_popover_test.gotools/dawgrun/pkg/go-repl/stdinreader.gotools/dawgrun/pkg/stubs/kindmapper.go
✅ Files skipped from review due to trivial changes (13)
- tools/dawgrun/cmd/dawgrun/art.txt
- tools/dawgrun/.gitignore
- tools/dawgrun/cmd/dawgrun/banner.txt
- tools/dawgrun/pkg/go-repl/.gitignore
- tools/dawgrun/pkg/go-repl/justfile
- tools/dawgrun/pkg/go-repl/go.mod
- tools/dawgrun/pkg/commands/helpers.go
- tools/dawgrun/pkg/go-repl/repl_popover_test.go
- tools/dawgrun/pkg/commands/help.go
- tools/dawgrun/pkg/go-repl/handler.go
- tools/dawgrun/pkg/commands/db.go
- tools/dawgrun/pkg/go-repl/LICENSE
- tools/dawgrun/pkg/commands/types.go
🚧 Files skipped from review as they are similar to previous changes (3)
- tools/dawgrun/pkg/go-repl/ansi.go
- tools/dawgrun/justfile
- tools/dawgrun/pkg/commands/runtime.go
fix(dawgrun): history file length limit and history bug fix chore: fix custom builds in workspace context fix(dawgrun): bind materialized parameters to the output query feat(dawgrun): add query-cypher command feat(dawgrun): give query-cypher format options chore(dawgrun): readme Update to readme chore(dawgrun): tab completions chore(dawgrun): configurable highlight style chore(dawgrun): clear flags after command exec chore(dawgrun): update readme chore(dawgrun): tidy up go.* fix(dawgrun): the rabbit came to me with feedback fix(dawgrun): this rabbit is bullying me fix(dawgrun): guard history loading fix(dawgrun): don't panic if we can't get term size fix(dawgrun): guarding around stdinreader
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tools/dawgrun/pkg/commands/db.go (2)
27-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winJoin the full DSN and stop echoing it in errors.
fields[1]drops the rest of a space-delimited Postgres connection string, and Line 41 still prints the raw DSN back to the terminal on failure. That breaks valid inputs and can leak credentials.Suggested patch
import ( "fmt" "strconv" + "strings" "github.com/davecgh/go-spew/spew" "github.com/specterops/dawgs" @@ - name := fields[0] - connStr := fields[1] + name := fields[0] + connStr := strings.Join(fields[1:], " ") connPool, err := pg.NewPool(drivers.DatabaseConfiguration{ Connection: connStr, }) @@ querier, err := dawgs.Open(ctx, "pg", dawgs.Config{ ConnectionString: connStr, Pool: connPool, }) if err != nil { - return fmt.Errorf("error opening database connection '%s': %w", connStr, err) + return fmt.Errorf("error opening database connection %q: %w", name, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/commands/db.go` around lines 27 - 41, The code currently uses fields[1] which truncates space-delimited DSNs and then prints the raw DSN on failure; rebuild the full connection string by joining fields[1:] with spaces (e.g., connStr := strings.Join(fields[1:], " ")), use that connStr when calling pg.NewPool and dawgs.Open, and remove or redact the raw DSN from the error message (change fmt.Errorf("error opening database connection '%s': %w", connStr, err) to a message that references the entry name or a redacted placeholder) so credentials are not echoed.
44-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the staged replacement if overwrite fails.
If Line 47 returns an error, the new
querieropened at Lines 36-39 is leaked and never added toctx.scope.Suggested patch
if existingConn, ok := ctx.scope.connections[name]; ok { // Warn+close existing connection before overwriting it ctx.output.Warnf("Discarding previous connection for '%s'", name) if err := existingConn.Close(ctx); err != nil { + _ = querier.Close(ctx) return fmt.Errorf("could not close previous connection '%s' for overwriting: %w", name, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/commands/db.go` around lines 44 - 49, When overwriting an existing connection in ctx.scope.connections, ensure the newly opened querier (variable querier) is closed if the attempt to close the existing connection fails; modify the block that handles the overwrite (the branch that calls existingConn.Close(ctx)) to explicitly close querier (or defer a cleanup) before returning the error so the staged replacement is not leaked.tools/dawgrun/pkg/go-repl/repl.go (2)
258-281:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep resize handling on the main REPL thread.
The polling goroutine still calls
r.resize()directly. That path mutates REPL state and writes to the terminal concurrently withdispatch()/force(), so resize events can still race with normal input rendering and garble the screen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/go-repl/repl.go` around lines 258 - 281, The goroutine in notifySizeChange currently calls r.resize(newW,newH) directly and can race with dispatch()/force(); instead, stop mutating REPL state from the background goroutine and forward the size change to the main REPL thread: replace the direct call to r.resize(...) inside the ticker loop with a non-blocking send of the new dimensions to a dedicated channel (e.g., r.resizeCh) or enqueue a task using the existing dispatch()/force() mechanism so the actual call to r.resize runs on the main REPL thread; ensure notifySizeChange creates/uses r.resizeCh (or dispatch wrapper) and that the main dispatch loop consumes that channel and calls r.resize there.
505-507:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUTF-8 input is still being dropped in the byte parser.
Valid two-byte UTF-8 sequences starting with
0xC3get treated asALT + KEY, and the fallback path still strips every byte>= 127. Non-ASCII query text is corrupted before it reachesEval().Also applies to: 584-586, 1601-1614
🧹 Nitpick comments (1)
tools/dawgrun/pkg/commands/cypher.go (1)
134-137: 💤 Low valueConsider protecting
ctx.scope.connectionsaccess with the existing mutex.Lines 134 and 240 read from
ctx.scope.connectionswithout acquiringctx.scope.mu. While the REPL is currently single-threaded (one command executes at a time), theScopestruct already includes async.RWMutex(seetypes.go:52-56), suggesting thread-safety was intended.If concurrent command execution is ever added, these unprotected reads could race with connection modifications.
Example fix pattern
+ ctx.scope.mu.RLock() conn, ok := ctx.scope.connections[connName] + ctx.scope.mu.RUnlock() if !ok { return fmt.Errorf("connection %s not found; did you `open` it?", connName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dawgrun/pkg/commands/cypher.go` around lines 134 - 137, Accesses to ctx.scope.connections (e.g., in the code that looks up conn, ok := ctx.scope.connections[connName]) are not protected by the Scope mutex; update those reads to use the existing sync.RWMutex on Scope by acquiring a read lock (ctx.scope.mu.RLock()) before reading connections and releasing it (ctx.scope.mu.RUnlock()) immediately after, and similarly protect any other direct reads of ctx.scope.connections (such as the read at the other location around line 240) to ensure thread-safe access consistent with the Scope struct’s intended concurrency model.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/dawgrun/pkg/go-repl/repl.go`:
- Around line 922-929: The loop over outLines prints an extra blank line when
out ends with '\n' because strings.Split(out, "\n") produces a trailing empty
element; fix by trimming trailing newlines first (e.g., trimmed :=
strings.TrimRight(out, "\n")) and if trimmed == "" print a single newline
(fmt.Println(); r.newLine()) else split trimmed with strings.Split(trimmed,
"\n") and iterate printing each line (fmt.Print) followed by r.newLine(); update
the code paths around variables out, outLines, strings.Split and r.newLine
accordingly.
---
Duplicate comments:
In `@tools/dawgrun/pkg/commands/db.go`:
- Around line 27-41: The code currently uses fields[1] which truncates
space-delimited DSNs and then prints the raw DSN on failure; rebuild the full
connection string by joining fields[1:] with spaces (e.g., connStr :=
strings.Join(fields[1:], " ")), use that connStr when calling pg.NewPool and
dawgs.Open, and remove or redact the raw DSN from the error message (change
fmt.Errorf("error opening database connection '%s': %w", connStr, err) to a
message that references the entry name or a redacted placeholder) so credentials
are not echoed.
- Around line 44-49: When overwriting an existing connection in
ctx.scope.connections, ensure the newly opened querier (variable querier) is
closed if the attempt to close the existing connection fails; modify the block
that handles the overwrite (the branch that calls existingConn.Close(ctx)) to
explicitly close querier (or defer a cleanup) before returning the error so the
staged replacement is not leaked.
In `@tools/dawgrun/pkg/go-repl/repl.go`:
- Around line 258-281: The goroutine in notifySizeChange currently calls
r.resize(newW,newH) directly and can race with dispatch()/force(); instead, stop
mutating REPL state from the background goroutine and forward the size change to
the main REPL thread: replace the direct call to r.resize(...) inside the ticker
loop with a non-blocking send of the new dimensions to a dedicated channel
(e.g., r.resizeCh) or enqueue a task using the existing dispatch()/force()
mechanism so the actual call to r.resize runs on the main REPL thread; ensure
notifySizeChange creates/uses r.resizeCh (or dispatch wrapper) and that the main
dispatch loop consumes that channel and calls r.resize there.
---
Nitpick comments:
In `@tools/dawgrun/pkg/commands/cypher.go`:
- Around line 134-137: Accesses to ctx.scope.connections (e.g., in the code that
looks up conn, ok := ctx.scope.connections[connName]) are not protected by the
Scope mutex; update those reads to use the existing sync.RWMutex on Scope by
acquiring a read lock (ctx.scope.mu.RLock()) before reading connections and
releasing it (ctx.scope.mu.RUnlock()) immediately after, and similarly protect
any other direct reads of ctx.scope.connections (such as the read at the other
location around line 240) to ensure thread-safe access consistent with the Scope
struct’s intended concurrency model.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0ebae75-b5a7-4a61-b67b-2238644ce3ca
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumtools/dawgrun/go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
go.modtools/dawgrun/.gitignoretools/dawgrun/README.mdtools/dawgrun/cmd/dawgrun/art.txttools/dawgrun/cmd/dawgrun/banner.txttools/dawgrun/cmd/dawgrun/main.gotools/dawgrun/docs/RFC.mdtools/dawgrun/go.modtools/dawgrun/justfiletools/dawgrun/pkg/commands/cypher.gotools/dawgrun/pkg/commands/db.gotools/dawgrun/pkg/commands/help.gotools/dawgrun/pkg/commands/helpers.gotools/dawgrun/pkg/commands/registry.gotools/dawgrun/pkg/commands/runtime.gotools/dawgrun/pkg/commands/types.gotools/dawgrun/pkg/go-repl/.gitignoretools/dawgrun/pkg/go-repl/LICENSEtools/dawgrun/pkg/go-repl/README.mdtools/dawgrun/pkg/go-repl/ansi.gotools/dawgrun/pkg/go-repl/go.modtools/dawgrun/pkg/go-repl/handler.gotools/dawgrun/pkg/go-repl/justfiletools/dawgrun/pkg/go-repl/repl.gotools/dawgrun/pkg/go-repl/repl_popover_test.gotools/dawgrun/pkg/go-repl/stdinreader.gotools/dawgrun/pkg/stubs/kindmapper.go
✅ Files skipped from review due to trivial changes (12)
- tools/dawgrun/.gitignore
- tools/dawgrun/cmd/dawgrun/art.txt
- tools/dawgrun/cmd/dawgrun/banner.txt
- tools/dawgrun/pkg/go-repl/LICENSE
- tools/dawgrun/pkg/go-repl/justfile
- tools/dawgrun/pkg/go-repl/go.mod
- tools/dawgrun/go.mod
- tools/dawgrun/pkg/go-repl/handler.go
- tools/dawgrun/pkg/commands/helpers.go
- tools/dawgrun/justfile
- tools/dawgrun/pkg/commands/types.go
- tools/dawgrun/pkg/stubs/kindmapper.go
🚧 Files skipped from review as they are similar to previous changes (4)
- tools/dawgrun/pkg/go-repl/ansi.go
- tools/dawgrun/pkg/go-repl/.gitignore
- tools/dawgrun/pkg/go-repl/repl_popover_test.go
- tools/dawgrun/pkg/commands/registry.go
Comment on lines
+922
to
+929
| if len(out) > 0 { | ||
| outLines := strings.Split(out, "\n") | ||
|
|
||
| for _, outLine := range outLines { | ||
| fmt.Print(outLine) | ||
| r.newLine() | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid the extra blank line for newline-terminated output.
strings.Split(out, "\n") leaves a trailing empty element when a handler returns "...\n", so the REPL prints an extra row after commands that already newline-terminate their output.
Suggested patch
if len(out) > 0 {
outLines := strings.Split(out, "\n")
+ if len(outLines) > 0 && outLines[len(outLines)-1] == "" {
+ outLines = outLines[:len(outLines)-1]
+ }
for _, outLine := range outLines {
fmt.Print(outLine)
r.newLine()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dawgrun/pkg/go-repl/repl.go` around lines 922 - 929, The loop over
outLines prints an extra blank line when out ends with '\n' because
strings.Split(out, "\n") produces a trailing empty element; fix by trimming
trailing newlines first (e.g., trimmed := strings.TrimRight(out, "\n")) and if
trimmed == "" print a single newline (fmt.Println(); r.newLine()) else split
trimmed with strings.Split(trimmed, "\n") and iterate printing each line
(fmt.Print) followed by r.newLine(); update the code paths around variables out,
outLines, strings.Split and r.newLine accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Build
Chores