Conversation
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
…ups. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRe-enabled a linter, added a linter group, simplified transaction rollback with a universal deferred Rollback, added template "%s" assertions in tests, improved SQL migration line parsing with a regex/newStep approach, bumped several indirect Go modules and Go version, and converted migration tests to in-memory SQLite with new test doubles and edge-case tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
file_migration.go (1)
123-159:⚠️ Potential issue | 🟠 MajorBlank leading lines now disable comment stripping.
Line 159 flips
newSteptofalseeven when the current line is empty, so a step formatted as blank line →-- comment→ SQL no longer treats that comment as leading. That can send comments toExecContextagain, which this helper is explicitly trying to avoid.🔧 Suggested fix
for step = 0; scanner.Scan(); { - if newStep && strings.HasPrefix(scanner.Text(), "--") { + text := scanner.Text() + trimmed := strings.TrimSpace(text) + + if newStep && trimmed == "" { + continue + } + + if newStep && strings.HasPrefix(trimmed, "--") { // skip leading comments continue } - if scanner.Text() == ";" { + if trimmed == ";" { log.Info("migration step", slog.String("migrationID", migrationID), slog.Int("step", step), @@ if buf.Len() > 0 { buf.WriteByte('\n') } - buf.Write(scanner.Bytes()) + buf.WriteString(text) newStep = false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@file_migration.go` around lines 123 - 159, The loop that parses migration steps marks newStep = false whenever it appends any line, but it doesn't skip empty lines, so a blank line followed by a comment will not be treated as a leading comment; update the logic around the scanner loop (the newStep variable handling inside the loop that currently sets newStep = false after buf.Write(scanner.Bytes())) to ignore empty lines when deciding to flip newStep to false (i.e., if scanner.Text() is empty or only whitespace, do not set newStep = false and do not append that blank line to the buffer), keeping the existing handling for lines starting with "--" and the step termination on ";" (use the existing variables newStep, step, buf, scanner, and tx.ExecContext to locate the code to change).
🤖 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`:
- Line 35: Update the pinned module version for modernc.org/libc in go.mod to
v1.67.6 to match modernc.org/sqlite@v1.46.1: replace the modernc.org/libc
v1.69.0 line with v1.67.6, then run `go get modernc.org/libc@v1.67.6` (or `go
mod tidy`) to update go.sum and verify builds; ensure the change is committed.
In `@migration_test.go`:
- Around line 26-32: openTempSQLite currently opens sqlite with ":memory:" which
creates a separate in-memory DB per connection; because *sql.DB is a pool this
causes flaky tests—fix by making the DB use a single connection or a shared
in-memory DSN: after creating the DB in openTempSQLite, either call
db.SetMaxOpenConns(1) and db.SetMaxIdleConns(1) to pin the pool to one
connection, or open with a shared-memory DSN such as
"file::memory:?cache=shared" instead of ":memory:" so all connections see the
same DB; apply the change in the openTempSQLite function where
sql.Open("sqlite", ":memory:") is called and keep the require.NoError and
Cleanup behavior intact.
---
Outside diff comments:
In `@file_migration.go`:
- Around line 123-159: The loop that parses migration steps marks newStep =
false whenever it appends any line, but it doesn't skip empty lines, so a blank
line followed by a comment will not be treated as a leading comment; update the
logic around the scanner loop (the newStep variable handling inside the loop
that currently sets newStep = false after buf.Write(scanner.Bytes())) to ignore
empty lines when deciding to flip newStep to false (i.e., if scanner.Text() is
empty or only whitespace, do not set newStep = false and do not append that
blank line to the buffer), keeping the existing handling for lines starting with
"--" and the step termination on ";" (use the existing variables newStep, step,
buf, scanner, and tx.ExecContext to locate the code to change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cf33586-0115-4a14-a178-fb6e66b62ed4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.golangci.yamldialects.godialects_test.gofile_migration.gogo.modmigration.gomigration_test.go
| gopkg.in/yaml.v3 v3.0.1 // indirect | ||
| gotest.tools/gotestsum v1.13.0 // indirect | ||
| modernc.org/libc v1.67.6 // indirect | ||
| modernc.org/libc v1.69.0 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo_libc="$(
sed -n 's/^[[:space:]]*modernc\.org\/libc[[:space:]]\+\(v[^[:space:]]\+\).*/\1/p' go.mod
)"
sqlite_libc="$(
curl -fsSL https://proxy.golang.org/modernc.org/sqlite/@v/v1.46.1.mod \
| sed -n 's/^[[:space:]]*modernc\.org\/libc[[:space:]]\+\(v[^[:space:]]\+\).*/\1/p'
)"
printf 'repo go.mod modernc.org/libc: %s\n' "$repo_libc"
printf 'modernc.org/sqlite@v1.46.1 expects: %s\n' "$sqlite_libc"
test -n "$repo_libc"
test -n "$sqlite_libc"
test "$repo_libc" = "$sqlite_libc"Repository: AlphaOne1/dmorph
Length of output: 141
Downgrade modernc.org/libc to v1.67.6 to match modernc.org/sqlite@v1.46.1.
The current pinned version v1.69.0 does not match the version required by modernc.org/sqlite@v1.46.1, which expects v1.67.6. Since modernc.org/sqlite documents strict version coupling for this dependency, the mismatch must be resolved before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 35, Update the pinned module version for modernc.org/libc in
go.mod to v1.67.6 to match modernc.org/sqlite@v1.46.1: replace the
modernc.org/libc v1.69.0 line with v1.67.6, then run `go get
modernc.org/libc@v1.67.6` (or `go mod tidy`) to update go.sum and verify builds;
ensure the change is committed.
…g with regex. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
migration_test.go (1)
588-610: Add connection pool settings for consistency.Unlike
openTempSQLite, this test opens an in-memory SQLite database without settingSetMaxOpenConns(1)andSetMaxIdleConns(1). While the current test passes because the context is canceled before any DB operations, adding these settings would maintain consistency and prevent potential flakiness if the test logic evolves.♻️ Suggested fix
func TestApplyFailsOnCanceledContext(t *testing.T) { t.Parallel() db, err := sql.Open("sqlite", ":memory:") - require.NoError(t, err) + db.SetMaxOpenConns(1) + db.SetMaxIdleConns(1) logger := slog.New(slog.DiscardHandler)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migration_test.go` around lines 588 - 610, The test TestApplyFailsOnCanceledContext opens an in-memory DB without configuring its connection pool, which differs from openTempSQLite; modify the test to set DB.SetMaxOpenConns(1) and DB.SetMaxIdleConns(1) right after creating db in TestApplyFailsOnCanceledContext so the connection pool is consistent with openTempSQLite and avoids potential flakiness when calling dmorph.Run with the canceled context.file_migration.go (1)
119-120: Move regex compilation to package level for efficiency.The
initialEmptyRegexis compiled on every call toapplyStepsStream. Since the pattern is constant, compiling it once at package initialization would avoid repeated compilation overhead during migrations.♻️ Suggested refactor
Add at package level (e.g., after line 19):
// initialEmptyRegex matches empty lines or lines containing only SQL comments. var initialEmptyRegex = regexp.MustCompile(`^\s*(?:--.*)?$`)Then remove line 119:
func applyStepsStream(ctx context.Context, tx *sql.Tx, r io.Reader, migrationID string, log *slog.Logger) error { const InitialScannerBufSize = 64 * 1024 const MaxScannerBufSize = 1024 * 1024 - initialEmptyRegex := regexp.MustCompile(`^\s*(?:--.*)?$`) - buf := bytes.Buffer{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@file_migration.go` around lines 119 - 120, The regex initialEmptyRegex is being compiled inside applyStepsStream on every call which wastes CPU; move its compilation to package initialization by declaring a package-level variable named initialEmptyRegex (with the same pattern `^\s*(?:--.*)?$`) and remove the local compilation inside applyStepsStream so the function uses the package-level initialEmptyRegex instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@file_migration.go`:
- Around line 119-120: The regex initialEmptyRegex is being compiled inside
applyStepsStream on every call which wastes CPU; move its compilation to package
initialization by declaring a package-level variable named initialEmptyRegex
(with the same pattern `^\s*(?:--.*)?$`) and remove the local compilation inside
applyStepsStream so the function uses the package-level initialEmptyRegex
instead.
In `@migration_test.go`:
- Around line 588-610: The test TestApplyFailsOnCanceledContext opens an
in-memory DB without configuring its connection pool, which differs from
openTempSQLite; modify the test to set DB.SetMaxOpenConns(1) and
DB.SetMaxIdleConns(1) right after creating db in TestApplyFailsOnCanceledContext
so the connection pool is consistent with openTempSQLite and avoids potential
flakiness when calling dmorph.Run with the canceled context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c83abc38-171f-4627-b54a-3becd372c463
📒 Files selected for processing (2)
file_migration.gomigration_test.go
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
migration_test.go (1)
587-607: Consider removing redundantdb.Close()call.Line 604's
db.Close()is unnecessary sinceopenTempSQLitealready registerst.Cleanup(func() { _ = db.Close() })on line 32. Whilesql.DB.Close()is idempotent and this won't cause issues, removing it improves consistency with other tests in this file.♻️ Suggested simplification
) - require.NoError(t, db.Close()) require.Error(t, err) require.ErrorContains(t, err, "context cancelled") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migration_test.go` around lines 587 - 607, The test TestApplyFailsOnCanceledContext contains a redundant explicit call to db.Close() after openTempSQLite already registered t.Cleanup to close the database; remove the require.NoError(t, db.Close()) line so the test relies on openTempSQLite's cleanup and matches other tests' pattern, leaving the context cancellation and assertions (dmorph.Run call and require.Error/require.ErrorContains) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@migration_test.go`:
- Around line 587-607: The test TestApplyFailsOnCanceledContext contains a
redundant explicit call to db.Close() after openTempSQLite already registered
t.Cleanup to close the database; remove the require.NoError(t, db.Close()) line
so the test relies on openTempSQLite's cleanup and matches other tests' pattern,
leaving the context cancellation and assertions (dmorph.Run call and
require.Error/require.ErrorContains) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e755d63-09b1-484f-94bd-d87ca5e42f01
📒 Files selected for processing (2)
file_migration.gomigration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- file_migration.go
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
|
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
… jobs, improved permission granularity, and aligned action versions. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/security.yml:
- Around line 95-107: The current printSarif step uses `grep results
govulncheck-results.sarif` which matches empty arrays; update the run script in
the step with id `printSarif` to parse the SARIF JSON
(`govulncheck-results.sarif`) with `jq` and set the `hasResults` output based on
the actual results array length (for example, use a command that computes total
results like `jq '[.runs[].results[]?] | length'` and then `if [ $COUNT -gt 0 ];
then echo "hasResults=true" >> $GITHUB_OUTPUT; else echo "hasResults=false" >>
$GITHUB_OUTPUT; fi`), ensuring `hasResults` drives the downstream upload step
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eea87cab-625e-400e-bbf1-c48bc41b9a24
📒 Files selected for processing (1)
.github/workflows/security.yml
3 participants
Summary by CodeRabbit
Bug Fixes
Tests
Chores