Add SQLite SQL connector support - #104
Conversation
📝 WalkthroughWalkthroughAdds SQLite-backed SQL config and connector selection, propagates the SQL connector through service and API constructors, and updates command startup to build connectors and migrations from the selected SQL configuration. Documentation now includes a minimal SQLite Go-services setup and migration caveat. ChangesSQLite Backend Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
api/assistant-api/internal/callcontext/store.go (1)
83-107:⚠️ Potential issue | 🟡 MinorRename
postgresStoreto reflect backend-agnostic abstraction; RETURNING is supported in mattn/go-sqlite3 v1.14.22.The struct on line 45 is named
postgresStorebut now abstracts overconnectors.SQLConnector, which supports both PostgreSQL and SQLite. Rename tosqlStoreor similar to accurately reflect the abstraction.SQLite compatibility is verified:
mattn/go-sqlite3 v1.14.22(Feb 2024) includes SQLite well after version 3.35.0 (Mar 2021) whenRETURNINGwas introduced, so the query is safe. Thedb.Raw(...).Scan(...)pattern onUPDATE ... RETURNINGwill work consistently across drivers for this setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/assistant-api/internal/callcontext/store.go` around lines 83 - 107, Rename the backend-specific type name postgresStore to a neutral name like sqlStore throughout the file to reflect it's using connectors.SQLConnector; update the receiver names on methods such as Claim(ctx context.Context, contextID string) (*CallContext, error) and any references to s.postgres to use the new struct field name as needed so the method implementation (including the UPDATE ... RETURNING query and db.Raw(...).Scan(&cc)) continues to compile and reference the correct struct and connector; ensure the struct declaration and all method receivers (e.g., Claim) and any constructor/new function are updated consistently.api/web-api/internal/service/vault/service.go (1)
65-71:⚠️ Potential issue | 🟡 MinorPre-existing:
Debugfformat string missing argument (unrelated to this PR, but visible in diff context).
vs.logger.Debugf("unable to delete vault %v")has a%vverb with no argument. Likely meanttx.Error. Not introduced here, so feel free to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/web-api/internal/service/vault/service.go` around lines 65 - 71, The Debugf call in the vault update/delete flow uses a "%v" verb with no argument; update the logging in the block that checks tx.Error (the if err := tx.Error; err != nil { ... } branch) to include the actual error (err or tx.Error) in the Debugf message (e.g., pass err as the format argument) so the format string matches the provided value; locate the code around tx := db.Where(...).Updates(vlt) and the subsequent if err := tx.Error check in service.go and fix vS.logger.Debugf to include the error variable.cmd/web/web.go (1)
312-313:⚠️ Potential issue | 🟠 MajorPre-existing risk worth addressing in this PR: DSN logged at info level can leak credentials.
Line 313 logs the full migration DSN. For PostgreSQL the DSN contains the password in the userinfo portion (
postgres://user:pass@host:port/db?...). While this log line is not newly introduced, the migration wiring around it changed in this PR — consider redacting before logging, e.g. parsing withnet/urland clearingURL.User, or logging host/db only.🛡️ Suggested redaction
import "net/url" func safeDSN(dsn string) string { u, err := url.Parse(dsn) if err != nil { return "<unparseable dsn>" } if u.User != nil { u.User = url.User(u.User.Username()) // drop password } return u.Redacted() // Go ≥1.15 also works } // ... app.Logger.Infof("Using DSN for migration: %s", safeDSN(dsn))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/web/web.go` around lines 312 - 313, The migration DSN is being logged in app.Logger.Infof("Using DSN for migration: %s", dsn) which can leak credentials; add a helper (e.g., safeDSN) that parses the DSN with net/url, clears the password via u.User = url.User(u.User.Username()) (or equivalent) and returns u.Redacted() or "<unparseable dsn>" on parse error, then replace the direct dsn usage in the Logger call with safeDSN(dsn) to ensure passwords are not written to logs.pkg/connectors/postgres_test.go (2)
114-138:⚠️ Potential issue | 🟡 Minor
mock.ExpectExecregistered after the call it's meant to stub.In
TestSQLConnector_Query_ErrorHandling,connector.Query(...)runs on line 135, butmock.ExpectExec("SELECT 1")...is only set up on line 136 — after the query. The expectation never applies, and the assertion only passes incidentally. Also,QueryusesRaw(...).Scan(...)which maps toExpectQuery, notExpectExec. Move the expectation before the call and useExpectQueryif you intend to stub the query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connectors/postgres_test.go` around lines 114 - 138, The test TestSQLConnector_Query_ErrorHandling registers the sqlmock expectation after calling connector.Query and uses ExpectExec, but connector.Query uses Raw(...).Scan(...) so you must set the mock expectation before invoking connector.Query and use mock.ExpectQuery(...) (not ExpectExec) with appropriate returned rows or error; update the setup around the mock variable (mock.ExpectQuery("SELECT 1")... or mock.ExpectQuery(regexp).WillReturnRows(...) / WillReturnError(...)) before calling connector.Query(ctx, "SELECT 1", &dest) so the expectation actually stubs the query and the assertion validates the intended error path.
140-163:⚠️ Potential issue | 🟡 MinorMisleading subtest names in
TestSQLConnector_EdgeCases.The subtests
"disconnect with nil db"and"db with nil db"both construct the connector with a non-nilgormDB, so they don't actually exercise the nil-db path their names advertise. Consider either settingdb: nilin these connectors or renaming the subtests to reflect what's being tested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connectors/postgres_test.go` around lines 140 - 163, The subtests in TestSQLConnector_EdgeCases are misnamed because they construct postgresConnector with db: gormDB (non-nil) but claim to test "nil db" behavior; update the test by either setting connector.db = nil when testing Disconnect and any "db with nil db" cases or rename the subtests to reflect they're testing behavior with an initialized gormDB; locate the postgresConnector creation in TestSQLConnector_EdgeCases and adjust the db field or test names accordingly (e.g., set db: nil for postgresConnector used with connector.Disconnect(ctx) or change the t.Run names).
🧹 Nitpick comments (21)
api/assistant-api/internal/callcontext/store.go (1)
45-55: Nit: renamepostgresStorenow that the backend is generic.The struct is still called
postgresStoredespite depending onconnectors.SQLConnector. ConsidersqlStore(and matchingNewStoreremains unchanged) to avoid misleading readers into thinking this is Postgres-only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/assistant-api/internal/callcontext/store.go` around lines 45 - 55, Rename the concrete type postgresStore to sqlStore to reflect that it depends on connectors.SQLConnector; update the type declaration (postgresStore -> sqlStore), the NewStore constructor to return &sqlStore{...} (keeping NewStore name/signature unchanged), and update all method receivers that reference postgresStore to use sqlStore so tests and usages continue to compile (e.g., any methods with receiver (p *postgresStore) should become (s *sqlStore)).pkg/authenticators/service_authenticator.go (1)
22-26: LGTM — parameter is accepted but unused.Note the
postgresparameter is still not stored on the struct (same as before the rename). If service auth is expected to query DB later, this is a no-op placeholder; otherwise consider dropping the parameter entirely to simplify the call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/authenticators/service_authenticator.go` around lines 22 - 26, The NewServiceAuthenticator function accepts a postgres connectors.SQLConnector parameter but does not store or use it; either persist it on the serviceAuthenticator struct or remove the unused parameter from the signature and all call sites. To keep DB access available later, add a field (e.g., postgres connectors.SQLConnector) to the serviceAuthenticator type and set it in NewServiceAuthenticator (assign postgres to that field); otherwise remove the postgres parameter from NewServiceAuthenticator and update any callers to stop passing it. Ensure references use the serviceAuthenticator type and NewServiceAuthenticator constructor name when making edits.pkg/configs/sqlite_config.go (1)
8-12: Consider SQLite-specific tuning knobs.For SQLite, a single writer is the common pattern (
MaxOpenConns=1) and WAL/journal/busy-timeout settings materially affect correctness under concurrency. Consider also exposingjournal_mode,busy_timeout_ms, andforeign_keysvia this config so the connector can apply them at open time, rather than hardcoding inNewSQLiteConnector.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/configs/sqlite_config.go` around lines 8 - 12, The SQLiteConfig struct lacks SQLite-specific tuning knobs; add fields to SQLiteConfig (e.g., JournalMode string `mapstructure:"journal_mode"`, BusyTimeoutMS int `mapstructure:"busy_timeout_ms"`, ForeignKeys bool `mapstructure:"foreign_keys"`) and update validation tags as needed, then modify NewSQLiteConnector to use these config values when opening/initializing the DB: set PRAGMA journal_mode, PRAGMA foreign_keys, and PRAGMA busy_timeout (or append them to the DSN) and ensure MaxOpenConnection is applied (recommend setting MaxOpenConnection to 1 by default for single-writer scenarios) so the connector uses config-driven settings rather than hardcoded values.api/endpoint-api/router/endpoint.go (1)
13-32: LGTM — both route constructors now take the genericSQLConnector; gRPC registrations unchanged.Same optional rename consideration applies to the
Postgresparameter name (see note on the assistant services); safe to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/endpoint-api/router/endpoint.go` around lines 13 - 32, Parameter name Postgres is misleading because both EndpointReaderApiRoute and InvokeApiRoute accept the generic connectors.SQLConnector; rename the Postgres parameter to a neutral name like sqlConnector or db in EndpointReaderApiRoute and InvokeApiRoute (and update all call sites) so the signature matches the type and intent, leaving the protos.RegisterEndpointServiceServer and protos.RegisterDeploymentServer gRPC registrations unchanged.api/web-api/config/config_test.go (1)
242-245: Minor: brittle substring assertion on error text.
strings.Contains(err.Error(), "set only one")couples the test to the exact wording ofResolveSQLConfig's error. If you later expose a sentinel error (e.g.,configs.ErrMultipleSQLBackends), prefererrors.Isfor a more stable assertion. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/web-api/config/config_test.go` around lines 242 - 245, The test in GetApplicationConfig error assertion is brittle; change the assertion to use errors.Is(err, configs.ErrMultipleSQLBackends) instead of substring matching and ensure the resolver returns the sentinel error (configs.ErrMultipleSQLBackends) from ResolveSQLConfig (or whatever function constructs that validation error) so the test can reliably use errors.Is; update the test after the call to GetApplicationConfig(vConfig) to assert errors.Is(err, configs.ErrMultipleSQLBackends) and, if needed, make ResolveSQLConfig return that sentinel error where it currently builds the "set only one" message.api/assistant-api/internal/services/assistant/tool.impl.service.go (1)
31-31: Optional: renamepostgresfield now that the type is generic.The field is now a generic
SQLConnector(Postgres or SQLite). Consider renaming tosql/dbacross services in a follow-up to avoid misleading readers. Not blocking — many call sites, best done as a single rename PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/assistant-api/internal/services/assistant/tool.impl.service.go` at line 31, Rename the struct field currently named `postgres` (type connectors.SQLConnector) to a neutral name like `sql` or `db` in the tool implementation (e.g., in the struct declared in tool.impl.service.go) and update all usages and constructors accordingly (e.g., replace `s.postgres` with `s.sql`/`s.db`, update NewToolService parameter and assignment, and any methods that call the connector) so the name reflects the generic SQLConnector type rather than Postgres; perform the same rename consistently across related services and call sites in this package.api/assistant-api/config/config_test.go (1)
150-152: Minor: error formatter prints a pointer address.When
PostgresConfigis non-nil butDBNamemismatches,%vofappConfig.PostgresConfigprints the pointer. Consider*appConfig.PostgresConfig(guarded by the nil check already in the condition) or justappConfig.PostgresConfig.DBNamefor readable failure output.Proposed tweak
- if appConfig.PostgresConfig == nil || appConfig.PostgresConfig.DBName != "assistant_db" { - t.Errorf("Expected PostgresConfig.DBName to be 'assistant_db', but got %v", appConfig.PostgresConfig) - } + if appConfig.PostgresConfig == nil { + t.Errorf("Expected PostgresConfig to be populated, got nil") + } else if appConfig.PostgresConfig.DBName != "assistant_db" { + t.Errorf("Expected PostgresConfig.DBName to be 'assistant_db', but got %q", appConfig.PostgresConfig.DBName) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/assistant-api/config/config_test.go` around lines 150 - 152, The test's failure message prints the pointer address because t.Errorf uses %v on appConfig.PostgresConfig; change the error call in the test to print the actual DBName (appConfig.PostgresConfig.DBName) or dereference the struct (*appConfig.PostgresConfig) since the preceding nil check ensures it's safe—update the t.Errorf invocation to include the readable value instead of the pointer.api/web-api/api/proxy/activity.go (1)
24-46: Consider renaming thepostgresfield todb/sql.Now that the type is the generic
SQLConnector, the field namepostgresis misleading (it could be SQLite). A rename across the struct and wiring would improve readability, but it's purely cosmetic — feel free to defer. Same applies toassistant.impl.service.go,analysis.impl.service.go,endpoint.go,sip.go, andexternal.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/web-api/api/proxy/activity.go` around lines 24 - 46, Rename the misleading field named `postgres` (type connectors.SQLConnector) to a more generic name such as `db` or `sql` across the webActivityApi struct and its usages: update the field declaration in the struct, the initializer in NewActivityGRPC (replace `postgres: postgres,` with `db: postgres` or `sql: postgres`), the argument passed to web_api.NewWebApi (currently `postgres`) and any calls that use that field such as internal_vault_service.NewVaultService(logger, postgres) to use the new name (e.g., `internal_vault_service.NewVaultService(logger, db)`); apply the same rename pattern in the other files mentioned (assistant.impl.service.go, analysis.impl.service.go, endpoint.go, sip.go, external.go) to keep names consistent.api/integration-api/config/config_test.go (1)
130-207: Good coverage for success + fallback + selection behavior.
TestGetApplicationConfig_SQLiteasserts the SQLite-populated / Postgres-nil path, andTestGetApplicationConfig_MultipleSQLConfigsasserts the mutual-exclusion validation error. This matches the backend-tests guideline of covering success, fallback, and factory/selection behavior.One small suggestion: the
"set only one"substring match is brittle — if the validation message inpkg/configs/sql_config.gois ever reworded, this test silently drifts. Consider exporting a sentinel error (e.g.ErrMultipleSQLConfigs) and usingerrors.Isinstead of string matching.As per coding guidelines: "Include at least a success path, fallback/error path, and factory/selection behavior assertion in backend tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/integration-api/config/config_test.go` around lines 130 - 207, The test's brittle string-match should be replaced with an exported sentinel error from the SQL config validation and checked with errors.Is: add/export ErrMultipleSQLConfigs in pkg/configs/sql_config.go where the "set only one" error is created, ensure GetApplicationConfig returns that error on multiple DB configs, then update TestGetApplicationConfig_MultipleSQLConfigs to assert errors.Is(err, configs.ErrMultipleSQLConfigs) (and keep the existing success assertions in TestGetApplicationConfig_SQLite unchanged).pkg/connectors/postgres.go (2)
34-36: Constructor name no longer matches return type — consider noting this in a follow-up.
NewPostgresConnectornow returns the genericSQLConnector, which is fine, but future SQLite/other constructors will share the same return type. If aNewSQLConnectorfactory (or driver-selection helper) already exists insql.go(as implied by the AI summary), consider whetherNewPostgresConnectorshould remain public or be demoted to an internal helper invoked by the factory to avoid two parallel construction paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connectors/postgres.go` around lines 34 - 36, The constructor NewPostgresConnector currently returns the generic SQLConnector which can conflict with a centralized factory; change the public API by making NewPostgresConnector an internal helper (rename to newPostgresConnector) and update the factory function (e.g., NewSQLConnector in sql.go) to call newPostgresConnector when the Postgres driver is selected, or alternatively keep NewPostgresConnector exported but add a one-line doc note pointing callers to NewSQLConnector for typical usage; update any references to NewPostgresConnector to use the new name or the factory accordingly and ensure postgresConnector and SQLConnector symbols remain unchanged.
22-26: MoveSQLConnectorinterface frompostgres.gotosql.gofor improved discoverability and better separation of concerns.Both
sqlite.goandpostgres.goimplement this backend-agnostic interface. Defining it insql.goalongside the factory functionNewSQLConnector()co-locates the interface with its dispatcher entry point and removes the tight coupling to the PostgreSQL implementation file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connectors/postgres.go` around lines 22 - 26, Move the SQLConnector interface declaration out of postgres.go and into sql.go next to the factory NewSQLConnector(): remove the type SQLConnector { Connector; Query(ctx context.Context, qry string, dest interface{}) error; DB(ctx context.Context) *gorm.DB } from postgres.go and add the identical interface definition in sql.go so both sqlite.go and postgres.go can import/implement it; ensure package declaration and imports are correct and run go vet/build to catch any missing references.cmd/assistant/assistant.go (1)
49-49: Stale name:Postgresfield now holds aSQLConnector.The field (and downstream locals/params like
postgresinNewAssistantDeploymentGRPCApi,NewKnowledgeGRPCApi,newSessionPipeline,WebApi.postgres) is no longer postgres-specific. Consider renaming toSQL/sql(orDB/db) in a follow-up sweep; leaving the old name is a readability hazard for newcomers reasoning about SQLite deployments.Also applies to: 227-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/assistant/assistant.go` at line 49, Rename the stale Postgres field to a neutral name (e.g., SQL or DB) throughout the codebase: change the struct field Postgres (type connectors.SQLConnector) to SQL (or DB), then update all identifiers and usages that refer to it — specifically the parameters/locals named postgres in NewAssistantDeploymentGRPCApi, NewKnowledgeGRPCApi, newSessionPipeline, WebApi.postgres and any other references (including the occurrence noted around line 227). Ensure constructor signatures, assignments, method receivers and internal references are updated to the new name so compilation and semantics remain correct.pkg/configs/sql_config_test.go (2)
13-42: Minor: redundant viperSetcalls in the postgres subtest.
v.Set(...)values are written butResolveSQLConfigis invoked with a fully-populated&PostgresConfig{...}literal, so the viper state has no effect on the assertion. Either drive the config from viper (e.g., viav.Unmarshal) to exercise the viper→struct path, or drop thev.Setcalls to keep the test minimal. Same applies to thesqlitesubtest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/configs/sql_config_test.go` around lines 13 - 42, The postgres subtest currently sets many viper keys via v.Set but then calls ResolveSQLConfig with a fully-populated &PostgresConfig literal so the viper state is unused; fix by either removing the redundant v.Set calls to keep the test minimal, or change the test to build the expected config by unmarshalling v into a PostgresConfig (using v.Unmarshal or by passing v-derived config into ResolveSQLConfig) so the viper→struct path is exercised; apply the same change pattern to the sqlite subtest; look for symbols viper.NewWithOptions, v.Set, ResolveSQLConfig, and PostgresConfig to locate the code to update.
66-78: Useerrors.Isinstead of!=for sentinel comparisons.The tests at lines 66 and 77 currently use direct
!=comparisons, which will break ifResolveSQLConfigwraps these sentinel errors withfmt.Errorf(...%w...)(as required by the repo's error-wrapping guideline). Replace witherrors.Is(err, ErrNoSQLConfigConfigured)anderrors.Is(err, ErrMultipleSQLConfigsDetected)to make the tests robust for future error-wrapping changes. Remember to add"errors"to the import block.Proposed change
- if err != ErrNoSQLConfigConfigured { + if !errors.Is(err, ErrNoSQLConfigConfigured) { t.Fatalf("ResolveSQLConfig() error = %v, want %v", err, ErrNoSQLConfigConfigured) } @@ - if err != ErrMultipleSQLConfigsDetected { + if !errors.Is(err, ErrMultipleSQLConfigsDetected) { t.Fatalf("ResolveSQLConfig() error = %v, want %v", err, ErrMultipleSQLConfigsDetected) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/configs/sql_config_test.go` around lines 66 - 78, The test compares sentinel errors with != which breaks if ResolveSQLConfig wraps errors; change the comparisons to use errors.Is(err, ErrNoSQLConfigConfigured) and errors.Is(err, ErrMultipleSQLConfigsDetected) in the two failing asserts, and add the "errors" package to the test file imports so the tests correctly detect wrapped errors returned by ResolveSQLConfig.api/web-api/api/proxy/endpoint.go (1)
22-42: LGTM.Type swap to
connectors.SQLConnectorpropagates cleanly intoweb_api.NewWebApi. Consider renaming thepostgresfield/parameter to something backend-agnostic (e.g.,sql/db) in a follow-up — not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/web-api/api/proxy/endpoint.go` around lines 22 - 42, The code uses the name postgres for a SQL connector parameter and struct field even though the type is connectors.SQLConnector; rename the parameter/field to a backend-agnostic name (e.g., sql or db) to reflect the general type. Update the NewEndpointGRPC function signature parameter name, the webEndpointApi struct literal field, and the webEndpointGRPCApi/webEndpointApi definitions/usages (e.g., postgres -> sql) and ensure every reference (including the call to web_api.NewWebApi and any other placements of postgres) is renamed consistently across this diff so compilation and calls still match.api/assistant-api/config/config.go (1)
94-108: DRY: extract shared SQL-backend selection helper.This
ResolveSQLConfig→ type switch → exclusivity nil-out block is duplicated verbatim inapi/web-api/config/config.go(lines 94–106), and will need to be repeated for every future service config (endpoint-api, etc.). Consider moving it intopkg/configs(e.g.,configs.ApplySelectedSQL(&cfg.PostgresConfig, &cfg.SQLiteConfig, v, validate)) so each service just calls a single helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/assistant-api/config/config.go` around lines 94 - 108, The code repeats a type-switch that applies the result of configs.ResolveSQLConfig to set either config.PostgresConfig or config.SQLiteConfig and nil-out the other; extract this logic into a shared helper in pkg/configs (e.g., configs.ApplySelectedSQL) that accepts pointers to the two config fields plus the resolved value and performs the type switch and exclusivity assignments, then replace the duplicated block in config.go with a call to that new helper using ResolveSQLConfig's returned sqlConfig, keeping the existing error handling and validator usage (symbols to update: configs.ResolveSQLConfig, config.PostgresConfig, config.SQLiteConfig, and add configs.ApplySelectedSQL).api/assistant-api/internal/services/assistant/conversaction.impl.service.go (1)
28-43: LGTM — connector type swap only.
assistantConversationService.postgresfield andNewAssistantConversationServiceconstructor correctly updated toconnectors.SQLConnector; no behavior changes.Nit (out of scope): the filename
conversaction.impl.service.golooks like a typo (conversaction→conversation) from a previous change and does not follow the<domain>.<qualifier>.<entity>.goconvention cleanly. Worth renaming in a follow-up PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/assistant-api/internal/services/assistant/conversaction.impl.service.go` around lines 28 - 43, The file name contains a typo "conversaction.impl.service.go" that should be renamed to follow the convention and fix spelling; rename the file to "conversation.impl.service.go" and update any references or build constraints so the package still compiles (ensure imports, go:generate or test files refer to the new filename), verifying symbols assistantConversationService and NewAssistantConversationService remain reachable and tests/build succeed after the rename.cmd/integration/integration.go (1)
295-300: Consider replacing the magic"sqlite"string with a typed constant or capability method.Comparing
sqlConfig.DriverName() == "sqlite"couples this bootstrap code to a string literal that must stay in sync with whateverSQLiteConfig.DriverName()returns. If the SQL abstraction later adds more backends (or the driver name canonicalization changes), this check silently drifts. Aconfigs.DriverSQLiteconstant or asqlConfig.SupportsBundledMigrations()helper on theSQLConfiginterface would make intent explicit and prevent divergence.♻️ Example refactor
- if sqlConfig.DriverName() == "sqlite" { + if sqlConfig.DriverName() == configs.DriverSQLite { app.Logger.Warnf("Skipping migrations for %s: bundled migrations are PostgreSQL-specific today.", sqlConfig.DisplayName()) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/integration/integration.go` around lines 295 - 300, Replace the hard-coded "sqlite" comparison with a typed capability check: add a SupportsBundledMigrations() bool method to the SQLConfig interface (or alternatively define a configs.DriverSQLite constant and use that instead of the literal), implement SupportsBundledMigrations() on the SQLiteConfig to return false, and change the code in integration.go to call sqlConfig.SupportsBundledMigrations() (or compare to configs.DriverSQLite) instead of sqlConfig.DriverName() == "sqlite" so the intent is explicit and future driver name changes won't break this check.pkg/connectors/postgres_test.go (1)
61-84:TestSQLConnector_Disconnectignores the returned error.
err = connector.Disconnect(ctx)is assigned but never asserted. Considerassert.NoError(t, err)(or the expected error) alongside the existingassert.Nil(t, connector.db)so the test actually validates Disconnect's contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connectors/postgres_test.go` around lines 61 - 84, The test TestSQLConnector_Disconnect currently assigns the result of connector.Disconnect(ctx) to err but never asserts it; update the test to assert the expected outcome of the disconnect call (e.g., assert.NoError(t, err) if disconnect should succeed or the appropriate assert for an expected error) in addition to the existing assert.Nil(t, connector.db) so Disconnect's contract is validated; locate the call to connector.Disconnect in TestSQLConnector_Disconnect and add the error assertion immediately after the call.api/web-api/internal/connect/microsoft.go (1)
59-99: Parameter namepostgresis now misleading.The type was generalized to
connectors.SQLConnector, but the parameter is still namedpostgresin all three constructors. Consider renaming tosqlConn(or similar) for clarity — purely cosmetic, feel free to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/web-api/internal/connect/microsoft.go` around lines 59 - 99, Rename the misleading parameter name "postgres" to something generic like "sqlConn" in the three constructors NewMicrosoftAuthenticationConnect, NewMicrosoftSharepointConnect, and NewMicrosoftOnedriveConnect; update the function signatures and all uses inside each function (notably the NewExternalConnect(...) call that currently passes "postgres") to use "sqlConn" so the parameter name matches its type connectors.SQLConnector and improves clarity.pkg/connectors/sqlite.go (1)
56-58:SetConnMaxLifetimeis hardcoded to 1 hour in both SQLite and PostgreSQL connectors.Neither
SQLiteConfignorPostgresConfigexpose a lifetime configuration field. If connection lifetime configurability is needed, add a field to both config structs and wire it through; otherwise, extract the hardcodedtime.Hourto a named constant for consistency across connectors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connectors/sqlite.go` around lines 56 - 58, The connectors currently hardcode sqlDB.SetConnMaxLifetime(time.Hour) in sqlite.go (and likewise in the Postgres connector) but neither SQLiteConfig nor PostgresConfig expose a lifetime field; either add a Lifetime (or ConnMaxLifetime) time.Duration field to both SQLiteConfig and PostgresConfig and wire it into the code path that creates sqlDB (use sqlt.cfg.ConnMaxLifetime / postgresCfg.ConnMaxLifetime when calling SetConnMaxLifetime), or if configurability is not desired, extract the literal time.Hour into a single named package-level constant (e.g., defaultConnMaxLifetime) and use that constant in both sqlite.go and the Postgres connector for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/integration-api/config/config_test.go`:
- Around line 119-121: The test's failure message prints the pointer address
because it logs appConfig.PostgresConfig with %v; change the error message to
dereference and/or print the actual DBName field (guarding for nil) so the
output shows the value. Specifically, in the failing assertion around
appConfig.PostgresConfig and PostgresConfig.DBName, update the t.Errorf to check
nil first and then include appConfig.PostgresConfig.DBName (or use
fmt.Sprintf("%+v", *appConfig.PostgresConfig) after nil-check) so the error
shows the struct contents or the DBName value rather than a pointer address.
In `@pkg/configs/sql_config.go`:
- Around line 107-125: The prefix detection currently inspects os.Environ()
causing false positives; update hasConfigPrefix (and thus detectSQLPrefixes) to
stop scanning process env and instead rely on Viper-only data: iterate
v.AllKeys() (normalize keys as already done) and return true only when a
normalized key starts with the target prefix AND v.IsSet(originalKey) (or
otherwise confirmed by Viper), so only configuration known to this Viper
instance triggers detection; remove the os.Environ() loop and any reliance on
global environment variables in hasConfigPrefix.
- Around line 97-105: SelectedSQLConfig currently can return a non-nil interface
holding a typed nil and panics when both configs are provided; change its
signature to SelectedSQLConfig(postgres *PostgresConfig, sqlite *SQLiteConfig)
(SQLConfig, error) and return explicit errors instead of panicking: if both
postgres and sqlite are non-nil return (nil, errors.New("requires exactly one
SQL config; both provided")), if both are nil return (nil, nil) or better (nil,
errors.New("no SQL config provided")) to avoid a typed-nil SQLConfig, and when
exactly one is set return that concrete config and nil error (ensure you return
the concrete sqlite or postgres value directly, not a nil pointer wrapped in the
interface); follow ResolveSQLConfig's error-returning pattern and import errors
as needed.
- Around line 38-48: The MigrationDSN builds the Postgres URL with fmt.Sprintf
which doesn't escape special characters in PostgresConfig.Auth.User and
.Password; change MigrationDSN to construct the URL using net/url (e.g.,
url.UserPassword or url.User and url.URL) so username/password are
percent-encoded and set Scheme="postgres", Host as fmt.Sprintf("%s:%d", c.Host,
c.Port), Path="/"+c.DBName, and RawQuery to include sslmode=c.SslMode; update
imports to include "net/url" and return url.String() from MigrationDSN.
- Around line 62-63: Add the sqlite3 database driver import and tests: import
the side-effect driver `_
"github.com/golang-migrate/migrate/v4/database/sqlite3"` into each command
entrypoint that constructs migrations (files using migrate.New():
cmd/web/web.go, cmd/integration/integration.go, cmd/endpoint/endpoint.go,
cmd/assistant/assistant.go) so the sqlite3 driver is registered at runtime; and
update sql_config_test.go to add test cases asserting
SQLiteConfig.MigrationDSN() returns "sqlite3://"+Path and that
PostgresConfig.MigrationDSN() still returns the expected Postgres DSN format
(use the existing PostgresConfig and SQLiteConfig constructors/fields in the
tests to validate their MigrationDSN() outputs).
In `@pkg/configs/sqlite_config.go`:
- Around line 8-12: Rename the typo'd fields and tags in both SQLiteConfig and
PostgresConfig: change MaxIdealConnection -> MaxIdleConnection and
mapstructure:"max_ideal_connection" -> "max_idle_connection" (and update any
references to these struct fields), then update tests and README accordingly
(notably pkg/configs/sql_config_test.go and any env/config examples); also
replace validate:"required" on the int fields MaxIdleConnection and
MaxOpenConnection with validate:"gte=0" (or remove the required tag and ensure
sensible defaults are applied) so zero is allowed by validation and database/sql
semantics are preserved.
In `@pkg/connectors/sql.go`:
- Around line 13-22: The NewSQLConnector function currently returns nil on
unknown config types which leads to silent nil-pointer panics; change its
signature to return (SQLConnector, error) (or at minimum log.Fatal/error in the
default branch) and update callers to handle the error. Inside NewSQLConnector,
switch on config types as before (case *configs.PostgresConfig ->
NewPostgresConnector, case *configs.SQLiteConfig -> NewSQLiteConnector) but on
the default path return a descriptive error (e.g., fmt.Errorf("unsupported SQL
config type: %T", config)) instead of nil; ensure all call sites that call
NewSQLConnector are updated to check the returned error and stop startup on
failure.
In `@pkg/connectors/sqlite_test.go`:
- Around line 16-60: Add missing negative and idempotency coverage: add a test
that constructs NewSQLiteConnector (or via NewSQLConnector) with an invalid
config (e.g., Path: "" or a non-writable path) and assert that
connector.Connect(ctx) returns an error; add an assertion that Disconnect is
idempotent by calling connector.Disconnect(ctx) twice (or calling Disconnect on
a never-connected connector) and ensuring no panic and appropriate error/no-op
behavior; and extend TestNewSQLConnector_SQLite to call connector.Connect(ctx)
and assert connector.Name() (e.g., equals "SQLITE sqlite3://...") to verify
factory selection dispatched the SQLite implementation. Reference
NewSQLiteConnector, NewSQLConnector, Connect, Disconnect, Name, and
ensureSQLiteParentDir when adding these checks.
In `@pkg/connectors/sqlite.go`:
- Around line 33-35: The DB(ctx) method on sqliteConnector may dereference nil
when sqlt.db is unset; add a nil guard in sqliteConnector.DB that checks if
sqlt.db == nil and, if so, returns a *gorm.DB whose Error field is set to a
descriptive error (e.g., using errors.New("sqlite connector not connected"))
instead of calling WithContext; otherwise return sqlt.db.WithContext(ctx).
Reference the sqliteConnector.DB method and set the Error on the returned
*gorm.DB so callers can check .Error safely.
- Around line 89-102: Disconnect currently dereferences sqlt.db without checking
for nil and will panic if Connect never succeeded; update
sqliteConnector.Disconnect to first check if sqlt.db == nil and return nil (or a
clear error) before calling sqlt.db.DB(), and only then call db.Close(); also
fix the logger messages in Disconnect (replace ambiguous "Disconnecting sqlite
client %s." with clearer text like "Failed to get underlying DB: %v" for the
DB() error and "Failed to close sqlite client: %v" for Close() error) so the
logged messages accurately describe the failures.
In `@README.md`:
- Around line 189-190: The README and config struct use a typo "Ideal" instead
of "Idle": rename the struct field MaxIdealConnection to MaxIdleConnection and
update its mapstructure tag from "max_ideal_connection" to
"max_idle_connection", then update all references/usages and example env vars
(change SQLITE__MAX_IDEAL_CONNECTION to SQLITE__MAX_IDLE_CONNECTION) to keep the
env-var contract consistent; ensure any code that reads the old tag/name
(parsing/config loading, tests, docs) is updated to use the new symbol and tag
so the change is applied project-wide.
---
Outside diff comments:
In `@api/assistant-api/internal/callcontext/store.go`:
- Around line 83-107: Rename the backend-specific type name postgresStore to a
neutral name like sqlStore throughout the file to reflect it's using
connectors.SQLConnector; update the receiver names on methods such as Claim(ctx
context.Context, contextID string) (*CallContext, error) and any references to
s.postgres to use the new struct field name as needed so the method
implementation (including the UPDATE ... RETURNING query and
db.Raw(...).Scan(&cc)) continues to compile and reference the correct struct and
connector; ensure the struct declaration and all method receivers (e.g., Claim)
and any constructor/new function are updated consistently.
In `@api/web-api/internal/service/vault/service.go`:
- Around line 65-71: The Debugf call in the vault update/delete flow uses a "%v"
verb with no argument; update the logging in the block that checks tx.Error (the
if err := tx.Error; err != nil { ... } branch) to include the actual error (err
or tx.Error) in the Debugf message (e.g., pass err as the format argument) so
the format string matches the provided value; locate the code around tx :=
db.Where(...).Updates(vlt) and the subsequent if err := tx.Error check in
service.go and fix vS.logger.Debugf to include the error variable.
In `@cmd/web/web.go`:
- Around line 312-313: The migration DSN is being logged in
app.Logger.Infof("Using DSN for migration: %s", dsn) which can leak credentials;
add a helper (e.g., safeDSN) that parses the DSN with net/url, clears the
password via u.User = url.User(u.User.Username()) (or equivalent) and returns
u.Redacted() or "<unparseable dsn>" on parse error, then replace the direct dsn
usage in the Logger call with safeDSN(dsn) to ensure passwords are not written
to logs.
In `@pkg/connectors/postgres_test.go`:
- Around line 114-138: The test TestSQLConnector_Query_ErrorHandling registers
the sqlmock expectation after calling connector.Query and uses ExpectExec, but
connector.Query uses Raw(...).Scan(...) so you must set the mock expectation
before invoking connector.Query and use mock.ExpectQuery(...) (not ExpectExec)
with appropriate returned rows or error; update the setup around the mock
variable (mock.ExpectQuery("SELECT 1")... or
mock.ExpectQuery(regexp).WillReturnRows(...) / WillReturnError(...)) before
calling connector.Query(ctx, "SELECT 1", &dest) so the expectation actually
stubs the query and the assertion validates the intended error path.
- Around line 140-163: The subtests in TestSQLConnector_EdgeCases are misnamed
because they construct postgresConnector with db: gormDB (non-nil) but claim to
test "nil db" behavior; update the test by either setting connector.db = nil
when testing Disconnect and any "db with nil db" cases or rename the subtests to
reflect they're testing behavior with an initialized gormDB; locate the
postgresConnector creation in TestSQLConnector_EdgeCases and adjust the db field
or test names accordingly (e.g., set db: nil for postgresConnector used with
connector.Disconnect(ctx) or change the t.Run names).
---
Nitpick comments:
In `@api/assistant-api/config/config_test.go`:
- Around line 150-152: The test's failure message prints the pointer address
because t.Errorf uses %v on appConfig.PostgresConfig; change the error call in
the test to print the actual DBName (appConfig.PostgresConfig.DBName) or
dereference the struct (*appConfig.PostgresConfig) since the preceding nil check
ensures it's safe—update the t.Errorf invocation to include the readable value
instead of the pointer.
In `@api/assistant-api/config/config.go`:
- Around line 94-108: The code repeats a type-switch that applies the result of
configs.ResolveSQLConfig to set either config.PostgresConfig or
config.SQLiteConfig and nil-out the other; extract this logic into a shared
helper in pkg/configs (e.g., configs.ApplySelectedSQL) that accepts pointers to
the two config fields plus the resolved value and performs the type switch and
exclusivity assignments, then replace the duplicated block in config.go with a
call to that new helper using ResolveSQLConfig's returned sqlConfig, keeping the
existing error handling and validator usage (symbols to update:
configs.ResolveSQLConfig, config.PostgresConfig, config.SQLiteConfig, and add
configs.ApplySelectedSQL).
In `@api/assistant-api/internal/callcontext/store.go`:
- Around line 45-55: Rename the concrete type postgresStore to sqlStore to
reflect that it depends on connectors.SQLConnector; update the type declaration
(postgresStore -> sqlStore), the NewStore constructor to return &sqlStore{...}
(keeping NewStore name/signature unchanged), and update all method receivers
that reference postgresStore to use sqlStore so tests and usages continue to
compile (e.g., any methods with receiver (p *postgresStore) should become (s
*sqlStore)).
In `@api/assistant-api/internal/services/assistant/conversaction.impl.service.go`:
- Around line 28-43: The file name contains a typo
"conversaction.impl.service.go" that should be renamed to follow the convention
and fix spelling; rename the file to "conversation.impl.service.go" and update
any references or build constraints so the package still compiles (ensure
imports, go:generate or test files refer to the new filename), verifying symbols
assistantConversationService and NewAssistantConversationService remain
reachable and tests/build succeed after the rename.
In `@api/assistant-api/internal/services/assistant/tool.impl.service.go`:
- Line 31: Rename the struct field currently named `postgres` (type
connectors.SQLConnector) to a neutral name like `sql` or `db` in the tool
implementation (e.g., in the struct declared in tool.impl.service.go) and update
all usages and constructors accordingly (e.g., replace `s.postgres` with
`s.sql`/`s.db`, update NewToolService parameter and assignment, and any methods
that call the connector) so the name reflects the generic SQLConnector type
rather than Postgres; perform the same rename consistently across related
services and call sites in this package.
In `@api/endpoint-api/router/endpoint.go`:
- Around line 13-32: Parameter name Postgres is misleading because both
EndpointReaderApiRoute and InvokeApiRoute accept the generic
connectors.SQLConnector; rename the Postgres parameter to a neutral name like
sqlConnector or db in EndpointReaderApiRoute and InvokeApiRoute (and update all
call sites) so the signature matches the type and intent, leaving the
protos.RegisterEndpointServiceServer and protos.RegisterDeploymentServer gRPC
registrations unchanged.
In `@api/integration-api/config/config_test.go`:
- Around line 130-207: The test's brittle string-match should be replaced with
an exported sentinel error from the SQL config validation and checked with
errors.Is: add/export ErrMultipleSQLConfigs in pkg/configs/sql_config.go where
the "set only one" error is created, ensure GetApplicationConfig returns that
error on multiple DB configs, then update
TestGetApplicationConfig_MultipleSQLConfigs to assert errors.Is(err,
configs.ErrMultipleSQLConfigs) (and keep the existing success assertions in
TestGetApplicationConfig_SQLite unchanged).
In `@api/web-api/api/proxy/activity.go`:
- Around line 24-46: Rename the misleading field named `postgres` (type
connectors.SQLConnector) to a more generic name such as `db` or `sql` across the
webActivityApi struct and its usages: update the field declaration in the
struct, the initializer in NewActivityGRPC (replace `postgres: postgres,` with
`db: postgres` or `sql: postgres`), the argument passed to web_api.NewWebApi
(currently `postgres`) and any calls that use that field such as
internal_vault_service.NewVaultService(logger, postgres) to use the new name
(e.g., `internal_vault_service.NewVaultService(logger, db)`); apply the same
rename pattern in the other files mentioned (assistant.impl.service.go,
analysis.impl.service.go, endpoint.go, sip.go, external.go) to keep names
consistent.
In `@api/web-api/api/proxy/endpoint.go`:
- Around line 22-42: The code uses the name postgres for a SQL connector
parameter and struct field even though the type is connectors.SQLConnector;
rename the parameter/field to a backend-agnostic name (e.g., sql or db) to
reflect the general type. Update the NewEndpointGRPC function signature
parameter name, the webEndpointApi struct literal field, and the
webEndpointGRPCApi/webEndpointApi definitions/usages (e.g., postgres -> sql) and
ensure every reference (including the call to web_api.NewWebApi and any other
placements of postgres) is renamed consistently across this diff so compilation
and calls still match.
In `@api/web-api/config/config_test.go`:
- Around line 242-245: The test in GetApplicationConfig error assertion is
brittle; change the assertion to use errors.Is(err,
configs.ErrMultipleSQLBackends) instead of substring matching and ensure the
resolver returns the sentinel error (configs.ErrMultipleSQLBackends) from
ResolveSQLConfig (or whatever function constructs that validation error) so the
test can reliably use errors.Is; update the test after the call to
GetApplicationConfig(vConfig) to assert errors.Is(err,
configs.ErrMultipleSQLBackends) and, if needed, make ResolveSQLConfig return
that sentinel error where it currently builds the "set only one" message.
In `@api/web-api/internal/connect/microsoft.go`:
- Around line 59-99: Rename the misleading parameter name "postgres" to
something generic like "sqlConn" in the three constructors
NewMicrosoftAuthenticationConnect, NewMicrosoftSharepointConnect, and
NewMicrosoftOnedriveConnect; update the function signatures and all uses inside
each function (notably the NewExternalConnect(...) call that currently passes
"postgres") to use "sqlConn" so the parameter name matches its type
connectors.SQLConnector and improves clarity.
In `@cmd/assistant/assistant.go`:
- Line 49: Rename the stale Postgres field to a neutral name (e.g., SQL or DB)
throughout the codebase: change the struct field Postgres (type
connectors.SQLConnector) to SQL (or DB), then update all identifiers and usages
that refer to it — specifically the parameters/locals named postgres in
NewAssistantDeploymentGRPCApi, NewKnowledgeGRPCApi, newSessionPipeline,
WebApi.postgres and any other references (including the occurrence noted around
line 227). Ensure constructor signatures, assignments, method receivers and
internal references are updated to the new name so compilation and semantics
remain correct.
In `@cmd/integration/integration.go`:
- Around line 295-300: Replace the hard-coded "sqlite" comparison with a typed
capability check: add a SupportsBundledMigrations() bool method to the SQLConfig
interface (or alternatively define a configs.DriverSQLite constant and use that
instead of the literal), implement SupportsBundledMigrations() on the
SQLiteConfig to return false, and change the code in integration.go to call
sqlConfig.SupportsBundledMigrations() (or compare to configs.DriverSQLite)
instead of sqlConfig.DriverName() == "sqlite" so the intent is explicit and
future driver name changes won't break this check.
In `@pkg/authenticators/service_authenticator.go`:
- Around line 22-26: The NewServiceAuthenticator function accepts a postgres
connectors.SQLConnector parameter but does not store or use it; either persist
it on the serviceAuthenticator struct or remove the unused parameter from the
signature and all call sites. To keep DB access available later, add a field
(e.g., postgres connectors.SQLConnector) to the serviceAuthenticator type and
set it in NewServiceAuthenticator (assign postgres to that field); otherwise
remove the postgres parameter from NewServiceAuthenticator and update any
callers to stop passing it. Ensure references use the serviceAuthenticator type
and NewServiceAuthenticator constructor name when making edits.
In `@pkg/configs/sql_config_test.go`:
- Around line 13-42: The postgres subtest currently sets many viper keys via
v.Set but then calls ResolveSQLConfig with a fully-populated &PostgresConfig
literal so the viper state is unused; fix by either removing the redundant v.Set
calls to keep the test minimal, or change the test to build the expected config
by unmarshalling v into a PostgresConfig (using v.Unmarshal or by passing
v-derived config into ResolveSQLConfig) so the viper→struct path is exercised;
apply the same change pattern to the sqlite subtest; look for symbols
viper.NewWithOptions, v.Set, ResolveSQLConfig, and PostgresConfig to locate the
code to update.
- Around line 66-78: The test compares sentinel errors with != which breaks if
ResolveSQLConfig wraps errors; change the comparisons to use errors.Is(err,
ErrNoSQLConfigConfigured) and errors.Is(err, ErrMultipleSQLConfigsDetected) in
the two failing asserts, and add the "errors" package to the test file imports
so the tests correctly detect wrapped errors returned by ResolveSQLConfig.
In `@pkg/configs/sqlite_config.go`:
- Around line 8-12: The SQLiteConfig struct lacks SQLite-specific tuning knobs;
add fields to SQLiteConfig (e.g., JournalMode string
`mapstructure:"journal_mode"`, BusyTimeoutMS int
`mapstructure:"busy_timeout_ms"`, ForeignKeys bool
`mapstructure:"foreign_keys"`) and update validation tags as needed, then modify
NewSQLiteConnector to use these config values when opening/initializing the DB:
set PRAGMA journal_mode, PRAGMA foreign_keys, and PRAGMA busy_timeout (or append
them to the DSN) and ensure MaxOpenConnection is applied (recommend setting
MaxOpenConnection to 1 by default for single-writer scenarios) so the connector
uses config-driven settings rather than hardcoded values.
In `@pkg/connectors/postgres_test.go`:
- Around line 61-84: The test TestSQLConnector_Disconnect currently assigns the
result of connector.Disconnect(ctx) to err but never asserts it; update the test
to assert the expected outcome of the disconnect call (e.g., assert.NoError(t,
err) if disconnect should succeed or the appropriate assert for an expected
error) in addition to the existing assert.Nil(t, connector.db) so Disconnect's
contract is validated; locate the call to connector.Disconnect in
TestSQLConnector_Disconnect and add the error assertion immediately after the
call.
In `@pkg/connectors/postgres.go`:
- Around line 34-36: The constructor NewPostgresConnector currently returns the
generic SQLConnector which can conflict with a centralized factory; change the
public API by making NewPostgresConnector an internal helper (rename to
newPostgresConnector) and update the factory function (e.g., NewSQLConnector in
sql.go) to call newPostgresConnector when the Postgres driver is selected, or
alternatively keep NewPostgresConnector exported but add a one-line doc note
pointing callers to NewSQLConnector for typical usage; update any references to
NewPostgresConnector to use the new name or the factory accordingly and ensure
postgresConnector and SQLConnector symbols remain unchanged.
- Around line 22-26: Move the SQLConnector interface declaration out of
postgres.go and into sql.go next to the factory NewSQLConnector(): remove the
type SQLConnector { Connector; Query(ctx context.Context, qry string, dest
interface{}) error; DB(ctx context.Context) *gorm.DB } from postgres.go and add
the identical interface definition in sql.go so both sqlite.go and postgres.go
can import/implement it; ensure package declaration and imports are correct and
run go vet/build to catch any missing references.
In `@pkg/connectors/sqlite.go`:
- Around line 56-58: The connectors currently hardcode
sqlDB.SetConnMaxLifetime(time.Hour) in sqlite.go (and likewise in the Postgres
connector) but neither SQLiteConfig nor PostgresConfig expose a lifetime field;
either add a Lifetime (or ConnMaxLifetime) time.Duration field to both
SQLiteConfig and PostgresConfig and wire it into the code path that creates
sqlDB (use sqlt.cfg.ConnMaxLifetime / postgresCfg.ConnMaxLifetime when calling
SetConnMaxLifetime), or if configurability is not desired, extract the literal
time.Hour into a single named package-level constant (e.g.,
defaultConnMaxLifetime) and use that constant in both sqlite.go and the Postgres
connector for consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 939a718d-9839-4447-a479-7e022eea3a2a
📒 Files selected for processing (91)
README.mdapi/assistant-api/api/assistant-deployment/assistant_deployment.goapi/assistant-api/api/assistant/assistant.goapi/assistant-api/api/knowledge/indexer.goapi/assistant-api/api/knowledge/knowledge.goapi/assistant-api/api/talk/talk.goapi/assistant-api/config/config.goapi/assistant-api/config/config_test.goapi/assistant-api/internal/adapters/internal/requestor.goapi/assistant-api/internal/adapters/request.goapi/assistant-api/internal/adapters/request_test.goapi/assistant-api/internal/callcontext/store.goapi/assistant-api/internal/services/assistant/analysis.impl.service.goapi/assistant-api/internal/services/assistant/assistant.deployment.impl.service.goapi/assistant-api/internal/services/assistant/assistant.impl.service.goapi/assistant-api/internal/services/assistant/conversaction.impl.service.goapi/assistant-api/internal/services/assistant/knowledge.impl.service.goapi/assistant-api/internal/services/assistant/telemetry.impl.service.goapi/assistant-api/internal/services/assistant/tool.impl.service.goapi/assistant-api/internal/services/assistant/webhook.impl.service.goapi/assistant-api/internal/services/knowledge/document.impl.service.goapi/assistant-api/internal/services/knowledge/knowledge.impl.service.goapi/assistant-api/router/assistant.goapi/assistant-api/router/health-check.goapi/assistant-api/router/knowledge.goapi/assistant-api/sip/sip.goapi/assistant-api/socket/pipeline.goapi/assistant-api/socket/socket.goapi/endpoint-api/api/endpoint.goapi/endpoint-api/api/invoker-endpoint.goapi/endpoint-api/config/config.goapi/endpoint-api/config/config_test.goapi/endpoint-api/internal/service/endpoint/service.goapi/endpoint-api/internal/service/log/service.goapi/endpoint-api/router/endpoint.goapi/endpoint-api/router/health-check.goapi/integration-api/api/audit_logging.goapi/integration-api/api/integration.goapi/integration-api/api/unified_provider.goapi/integration-api/config/config.goapi/integration-api/config/config_test.goapi/integration-api/internal/service/audit/service.goapi/integration-api/router/health-check.goapi/integration-api/router/provider.goapi/web-api/api/auth.goapi/web-api/api/connect.goapi/web-api/api/notification.goapi/web-api/api/organization.goapi/web-api/api/project.goapi/web-api/api/proxy/activity.goapi/web-api/api/proxy/assistant.goapi/web-api/api/proxy/assistant_deployment.goapi/web-api/api/proxy/document.goapi/web-api/api/proxy/endpoint.goapi/web-api/api/proxy/invoke.goapi/web-api/api/proxy/knowledge.goapi/web-api/api/vault.goapi/web-api/api/web.goapi/web-api/authenticator/authenticator.goapi/web-api/config/config.goapi/web-api/config/config_test.goapi/web-api/internal/connect/atlassian.goapi/web-api/internal/connect/external.goapi/web-api/internal/connect/github.goapi/web-api/internal/connect/gitlab.goapi/web-api/internal/connect/google.goapi/web-api/internal/connect/hubspot.goapi/web-api/internal/connect/linkedin.goapi/web-api/internal/connect/microsoft.goapi/web-api/internal/connect/notion.goapi/web-api/internal/connect/slack.goapi/web-api/internal/service/notification/service.goapi/web-api/internal/service/organization/service.goapi/web-api/internal/service/project/service.goapi/web-api/internal/service/user/service.goapi/web-api/internal/service/vault/service.goapi/web-api/router/health-check.goapi/web-api/router/web.gocmd/assistant/assistant.gocmd/endpoint/endpoint.gocmd/integration/integration.gocmd/web/web.gopkg/authenticators/service_authenticator.gopkg/configs/sql_config.gopkg/configs/sql_config_test.gopkg/configs/sqlite_config.gopkg/connectors/postgres.gopkg/connectors/postgres_test.gopkg/connectors/sql.gopkg/connectors/sqlite.gopkg/connectors/sqlite_test.go
| if appConfig.PostgresConfig == nil || appConfig.PostgresConfig.DBName != "endpoint_db" { | ||
| t.Errorf("Expected PostgresConfig.DBName to be 'endpoint_db', but got %v", appConfig.PostgresConfig) | ||
| } |
There was a problem hiding this comment.
Nit: error message prints a pointer address, not struct contents.
appConfig.PostgresConfig is a *PostgresConfig; %v will render the pointer (e.g. 0xc0001…) rather than the struct fields, which defeats the purpose of the debug output. Dereference (guarded) or print DBName explicitly.
Proposed tweak
- if appConfig.PostgresConfig == nil || appConfig.PostgresConfig.DBName != "endpoint_db" {
- t.Errorf("Expected PostgresConfig.DBName to be 'endpoint_db', but got %v", appConfig.PostgresConfig)
- }
+ if appConfig.PostgresConfig == nil {
+ t.Errorf("Expected PostgresConfig to be populated, got nil")
+ } else if appConfig.PostgresConfig.DBName != "endpoint_db" {
+ t.Errorf("Expected PostgresConfig.DBName to be 'endpoint_db', but got %q", appConfig.PostgresConfig.DBName)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if appConfig.PostgresConfig == nil || appConfig.PostgresConfig.DBName != "endpoint_db" { | |
| t.Errorf("Expected PostgresConfig.DBName to be 'endpoint_db', but got %v", appConfig.PostgresConfig) | |
| } | |
| if appConfig.PostgresConfig == nil { | |
| t.Errorf("Expected PostgresConfig to be populated, got nil") | |
| } else if appConfig.PostgresConfig.DBName != "endpoint_db" { | |
| t.Errorf("Expected PostgresConfig.DBName to be 'endpoint_db', but got %q", appConfig.PostgresConfig.DBName) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/integration-api/config/config_test.go` around lines 119 - 121, The test's
failure message prints the pointer address because it logs
appConfig.PostgresConfig with %v; change the error message to dereference and/or
print the actual DBName field (guarding for nil) so the output shows the value.
Specifically, in the failing assertion around appConfig.PostgresConfig and
PostgresConfig.DBName, update the t.Errorf to check nil first and then include
appConfig.PostgresConfig.DBName (or use fmt.Sprintf("%+v",
*appConfig.PostgresConfig) after nil-check) so the error shows the struct
contents or the DBName value rather than a pointer address.
| func (c *PostgresConfig) MigrationDSN() string { | ||
| return fmt.Sprintf( | ||
| "postgres://%s:%s@%s:%d/%s?sslmode=%s", | ||
| c.Auth.User, | ||
| c.Auth.Password, | ||
| c.Host, | ||
| c.Port, | ||
| c.DBName, | ||
| c.SslMode, | ||
| ) | ||
| } |
There was a problem hiding this comment.
URL-encode Postgres credentials in MigrationDSN.
The DSN is built via raw fmt.Sprintf, so any @, :, /, ?, #, or space in User/Password will produce a malformed URL and either fail to parse or connect to the wrong host/DB. Build via net/url instead.
🛡️ Proposed fix
-func (c *PostgresConfig) MigrationDSN() string {
- return fmt.Sprintf(
- "postgres://%s:%s@%s:%d/%s?sslmode=%s",
- c.Auth.User,
- c.Auth.Password,
- c.Host,
- c.Port,
- c.DBName,
- c.SslMode,
- )
-}
+func (c *PostgresConfig) MigrationDSN() string {
+ u := url.URL{
+ Scheme: "postgres",
+ User: url.UserPassword(c.Auth.User, c.Auth.Password),
+ Host: fmt.Sprintf("%s:%d", c.Host, c.Port),
+ Path: c.DBName,
+ }
+ q := url.Values{}
+ q.Set("sslmode", c.SslMode)
+ u.RawQuery = q.Encode()
+ return u.String()
+}(also add "net/url" to imports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/configs/sql_config.go` around lines 38 - 48, The MigrationDSN builds the
Postgres URL with fmt.Sprintf which doesn't escape special characters in
PostgresConfig.Auth.User and .Password; change MigrationDSN to construct the URL
using net/url (e.g., url.UserPassword or url.User and url.URL) so
username/password are percent-encoded and set Scheme="postgres", Host as
fmt.Sprintf("%s:%d", c.Host, c.Port), Path="/"+c.DBName, and RawQuery to include
sslmode=c.SslMode; update imports to include "net/url" and return url.String()
from MigrationDSN.
| func (c *SQLiteConfig) MigrationDSN() string { | ||
| return "sqlite3://" + c.Path |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where MigrationDSN is consumed and which migrate driver is imported.
rg -nP -C3 '\bMigrationDSN\s*\('
rg -nP -C2 'golang-migrate|migrate/database/sqlite3|migrate/source'Repository: rapidaai/voice-ai
Length of output: 6262
🏁 Script executed:
# Look at the code around where MigrationDSN is used, specifically the migrate.New call
rg -A10 'dsn := sqlConfig.MigrationDSN\(\)' cmd/web/web.go | head -30Repository: rapidaai/voice-ai
Length of output: 472
🏁 Script executed:
# Check if sqlite3 database driver is imported anywhere
rg 'database/sqlite3' --Repository: rapidaai/voice-ai
Length of output: 43
🏁 Script executed:
# Look for test files in pkg/configs to understand the expected behavior
fd -e '*_test.go' pkg/configsRepository: rapidaai/voice-ai
Length of output: 410
🏁 Script executed:
# Check go.mod to see what sqlite3-related packages might be there
rg 'sqlite|mattn' go.modRepository: rapidaai/voice-ai
Length of output: 239
🏁 Script executed:
# Find test files in pkg/configs directory properly
fd . pkg/configs -e _test.goRepository: rapidaai/voice-ai
Length of output: 43
🏁 Script executed:
# Verify the exact import path needed for sqlite3 driver in golang-migrate/v4
rg 'database/sqlite' --Repository: rapidaai/voice-ai
Length of output: 43
🏁 Script executed:
# Check if there are any mentions of SQLite in test files throughout the repo
rg -l 'SQLiteConfig|sqlite' --include='*_test.go'Repository: rapidaai/voice-ai
Length of output: 502
🏁 Script executed:
# Look at the full context of how migrate.New is used in all cmd files
rg -B5 -A15 'migrate\.New' cmd/Repository: rapidaai/voice-ai
Length of output: 4848
🏁 Script executed:
# Check if MigrationDSN is used elsewhere besides migration setup
rg 'MigrationDSN' -A2 -B2Repository: rapidaai/voice-ai
Length of output: 1641
🏁 Script executed:
# Look at pkg/configs/sql_config.go to see the full file and interface
cat pkg/configs/sql_config.goRepository: rapidaai/voice-ai
Length of output: 3220
🏁 Script executed:
# Check if there's a sql_config_test.go file
ls -la pkg/configs/*test* 2>/dev/null || echo "No test files found"Repository: rapidaai/voice-ai
Length of output: 982
🏁 Script executed:
# Check the sql_config_test.go to see if SQLiteConfig is tested
cat pkg/configs/sql_config_test.goRepository: rapidaai/voice-ai
Length of output: 3236
🏁 Script executed:
# Verify what the golang-migrate sqlite3 driver expects by checking the actual driver
# Search web for golang-migrate sqlite3 DSN format
web_search
golang-migrate v4 sqlite3 driver DSN formatRepository: rapidaai/voice-ai
Length of output: 164
🌐 Web query:
golang-migrate v4 sqlite3 database driver DSN format documentation
💡 Result:
The DSN format for the golang-migrate v4 sqlite3 database driver is sqlite3://path/to/database?query-parameters. Key details: - Scheme: sqlite3:// - Path: The database file path (e.g., /path/to/database.db or :memory: for in-memory). - Query parameters: - x-migrations-table: Name of the migrations table (defaults to schema_migrations). - x-no-tx-wrap: Set to true to disable implicit transaction wrapping (allows explicit BEGIN/COMMIT in migrations). - Additional query parameters supported by the underlying github.com/mattn/go-sqlite3 driver (e.g., _busy_timeout, _journal_mode) can be used, as they are passed through. - Example: sqlite3:///tmp/mydb.db?x-migrations-table=my_migrations&x-no-tx-wrap=true - The driver uses github.com/mattn/go-sqlite3 (requires CGO) and automatically wraps migrations in transactions by default (migrations should not contain explicit BEGIN/COMMIT unless x-no-tx-wrap=true). Official documentation is in the package README at pkg.go.dev/github.com/golang-migrate/migrate/v4/database/sqlite3 and source code confirms parsing logic: strips "sqlite3://" prefix, uses remaining as go-sqlite3 DSN after filtering custom params.
Citations:
- 1: https://pkg.go.dev/github.com/golang-migrate/migrate/v4/database/sqlite3
- 2: https://github.com/golang-migrate/migrate/blob/master/database/sqlite3/sqlite3.go
🏁 Script executed:
# Check if there are test files that actually test MigrationDSN for SQLiteConfig
rg -A 5 'MigrationDSN|sqlite3://' pkg/configs/sql_config_test.goRepository: rapidaai/voice-ai
Length of output: 43
🏁 Script executed:
# Look for any mentions of how migrations are tested with SQLite
rg -l 'MigrationDSN|migrate\.New' --type go | head -20Repository: rapidaai/voice-ai
Length of output: 184
Add missing sqlite3 driver import and test coverage for MigrationDSN().
The DSN format sqlite3:// + path is correct for golang-migrate v4's sqlite3 driver. However, two issues need to be addressed:
-
Missing driver import: The sqlite3 database driver is not imported in cmd/web/web.go, cmd/integration/integration.go, cmd/endpoint/endpoint.go, or cmd/assistant/assistant.go. Add
_ "github.com/golang-migrate/migrate/v4/database/sqlite3"to each file so migrate.New() can access the driver at runtime. -
Missing test coverage: sql_config_test.go does not test the new MigrationDSN() method for SQLiteConfig (only tests DriverName()). Add test cases to verify MigrationDSN() returns the correct DSN format for both PostgresConfig and SQLiteConfig.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/configs/sql_config.go` around lines 62 - 63, Add the sqlite3 database
driver import and tests: import the side-effect driver `_
"github.com/golang-migrate/migrate/v4/database/sqlite3"` into each command
entrypoint that constructs migrations (files using migrate.New():
cmd/web/web.go, cmd/integration/integration.go, cmd/endpoint/endpoint.go,
cmd/assistant/assistant.go) so the sqlite3 driver is registered at runtime; and
update sql_config_test.go to add test cases asserting
SQLiteConfig.MigrationDSN() returns "sqlite3://"+Path and that
PostgresConfig.MigrationDSN() still returns the expected Postgres DSN format
(use the existing PostgresConfig and SQLiteConfig constructors/fields in the
tests to validate their MigrationDSN() outputs).
| func SelectedSQLConfig(postgres *PostgresConfig, sqlite *SQLiteConfig) SQLConfig { | ||
| if postgres != nil && sqlite != nil { | ||
| panic("SelectedSQLConfig requires exactly one SQL config; both postgres and sqlite were provided") | ||
| } | ||
| if sqlite != nil { | ||
| return sqlite | ||
| } | ||
| return postgres | ||
| } |
There was a problem hiding this comment.
SelectedSQLConfig can return a typed-nil SQLConfig and panics instead of erroring.
Two issues:
- When both
postgresandsqliteare nil, this returns(*PostgresConfig)(nil)wrapped in anSQLConfiginterface — a non-nil interface with a nil concrete value. Any caller doingif cfg != nil { cfg.DriverName() }will NPE rather than take the nil-guard branch. panic(...)is unfriendly for a config-selection helper that ultimately runs at startup; returning an error is more consistent withResolveSQLConfig's style.
🛡️ Proposed fix
-func SelectedSQLConfig(postgres *PostgresConfig, sqlite *SQLiteConfig) SQLConfig {
- if postgres != nil && sqlite != nil {
- panic("SelectedSQLConfig requires exactly one SQL config; both postgres and sqlite were provided")
- }
- if sqlite != nil {
- return sqlite
- }
- return postgres
-}
+func SelectedSQLConfig(postgres *PostgresConfig, sqlite *SQLiteConfig) (SQLConfig, error) {
+ switch {
+ case postgres != nil && sqlite != nil:
+ return nil, ErrMultipleSQLConfigsDetected
+ case sqlite != nil:
+ return sqlite, nil
+ case postgres != nil:
+ return postgres, nil
+ default:
+ return nil, ErrNoSQLConfigConfigured
+ }
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/configs/sql_config.go` around lines 97 - 105, SelectedSQLConfig currently
can return a non-nil interface holding a typed nil and panics when both configs
are provided; change its signature to SelectedSQLConfig(postgres
*PostgresConfig, sqlite *SQLiteConfig) (SQLConfig, error) and return explicit
errors instead of panicking: if both postgres and sqlite are non-nil return
(nil, errors.New("requires exactly one SQL config; both provided")), if both are
nil return (nil, nil) or better (nil, errors.New("no SQL config provided")) to
avoid a typed-nil SQLConfig, and when exactly one is set return that concrete
config and nil error (ensure you return the concrete sqlite or postgres value
directly, not a nil pointer wrapped in the interface); follow ResolveSQLConfig's
error-returning pattern and import errors as needed.
| func detectSQLPrefixes(v *viper.Viper) (bool, bool) { | ||
| return hasConfigPrefix(v, "POSTGRES"), hasConfigPrefix(v, "SQLITE") | ||
| } | ||
|
|
||
| func hasConfigPrefix(v *viper.Viper, prefix string) bool { | ||
| prefix = strings.ToUpper(prefix) + "__" | ||
| for _, key := range v.AllKeys() { | ||
| normalizedKey := strings.ToUpper(strings.ReplaceAll(key, ".", "__")) | ||
| if strings.HasPrefix(normalizedKey, prefix) { | ||
| return true | ||
| } | ||
| } | ||
| for _, env := range os.Environ() { | ||
| name, _, found := strings.Cut(env, "=") | ||
| if found && strings.HasPrefix(strings.ToUpper(name), prefix) { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
Prefix detection can produce false positives from unrelated process env vars.
hasConfigPrefix scans os.Environ() for any variable starting with POSTGRES__ / SQLITE__, independent of whether Viper is actually bound to those keys or whether this process' Viper uses an env prefix (e.g. RAPIDA_). In containerized environments where both a parent-provided POSTGRES__HOST and a configured SQLITE__PATH can legitimately coexist (e.g., sidecars, shared compose files), this will spuriously trip ErrMultipleSQLConfigsDetected, and configuration issues surface only at boot.
Consider relying on Viper alone (v.AllKeys() + v.IsSet) or checking only the prefix(es) explicitly bound via v.BindEnv/v.SetEnvPrefix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/configs/sql_config.go` around lines 107 - 125, The prefix detection
currently inspects os.Environ() causing false positives; update hasConfigPrefix
(and thus detectSQLPrefixes) to stop scanning process env and instead rely on
Viper-only data: iterate v.AllKeys() (normalize keys as already done) and return
true only when a normalized key starts with the target prefix AND
v.IsSet(originalKey) (or otherwise confirmed by Viper), so only configuration
known to this Viper instance triggers detection; remove the os.Environ() loop
and any reliance on global environment variables in hasConfigPrefix.
| func TestSQLiteConnector_Lifecycle(t *testing.T) { | ||
| logger, _ := commons.NewApplicationLogger() | ||
| path := filepath.Join(t.TempDir(), "data", "rapida.db") | ||
| connector := NewSQLiteConnector(&configs.SQLiteConfig{ | ||
| Path: path, | ||
| MaxIdealConnection: 1, | ||
| MaxOpenConnection: 1, | ||
| }, logger) | ||
|
|
||
| ctx := context.Background() | ||
| require.NoError(t, connector.Connect(ctx)) | ||
| assert.True(t, connector.IsConnected(ctx)) | ||
| assert.Equal(t, "SQLITE sqlite3://"+path, connector.Name()) | ||
|
|
||
| db := connector.DB(ctx) | ||
| require.NoError(t, db.Exec("CREATE TABLE test_items (name TEXT NOT NULL)").Error) | ||
| require.NoError(t, db.Exec("INSERT INTO test_items(name) VALUES (?)", "sqlite").Error) | ||
|
|
||
| var rows []struct { | ||
| Name string | ||
| } | ||
| require.NoError(t, connector.Query(ctx, "SELECT name FROM test_items", &rows)) | ||
| require.Len(t, rows, 1) | ||
| assert.Equal(t, "sqlite", rows[0].Name) | ||
| assert.FileExists(t, path) | ||
|
|
||
| require.NoError(t, connector.Disconnect(ctx)) | ||
| assert.False(t, connector.IsConnected(ctx)) | ||
| } | ||
|
|
||
| func TestNewSQLConnector_SQLite(t *testing.T) { | ||
| logger, _ := commons.NewApplicationLogger() | ||
| connector := NewSQLConnector(&configs.SQLiteConfig{ | ||
| Path: filepath.Join(t.TempDir(), "rapida.db"), | ||
| MaxIdealConnection: 1, | ||
| MaxOpenConnection: 1, | ||
| }, logger) | ||
| require.NotNil(t, connector) | ||
| } | ||
|
|
||
| func TestEnsureSQLiteParentDir(t *testing.T) { | ||
| path := filepath.Join(t.TempDir(), "nested", "sqlite.db") | ||
| require.NoError(t, ensureSQLiteParentDir(path)) | ||
| _, err := os.Stat(filepath.Dir(path)) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Add an error/fallback path to round out coverage.
Per coding guidelines, backend tests should include at least a success path, fallback/error path, and factory/selection behavior assertion. Current coverage is success-only:
- No negative test for
NewSQLiteConnector(e.g.Path: ""or a path under a non-writable directory) to assertConnectreturns an error. - No assertion that
Disconnectis idempotent (calling twice, or calling when not connected). TestNewSQLConnector_SQLiteonly asserts non-nil; consider exercisingConnect+Name()to prove the factory actually dispatched to the SQLite implementation (selection behavior).
As per coding guidelines: "Include at least a success path, fallback/error path, and factory/selection behavior assertion in backend tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/connectors/sqlite_test.go` around lines 16 - 60, Add missing negative and
idempotency coverage: add a test that constructs NewSQLiteConnector (or via
NewSQLConnector) with an invalid config (e.g., Path: "" or a non-writable path)
and assert that connector.Connect(ctx) returns an error; add an assertion that
Disconnect is idempotent by calling connector.Disconnect(ctx) twice (or calling
Disconnect on a never-connected connector) and ensuring no panic and appropriate
error/no-op behavior; and extend TestNewSQLConnector_SQLite to call
connector.Connect(ctx) and assert connector.Name() (e.g., equals "SQLITE
sqlite3://...") to verify factory selection dispatched the SQLite
implementation. Reference NewSQLiteConnector, NewSQLConnector, Connect,
Disconnect, Name, and ensureSQLiteParentDir when adding these checks.
| func (sqlt *sqliteConnector) DB(ctx context.Context) *gorm.DB { | ||
| return sqlt.db.WithContext(ctx) | ||
| } |
There was a problem hiding this comment.
DB(ctx) will panic if called before Connect (or after Disconnect).
sqlt.db is nil until Connect succeeds, and Disconnect explicitly sets it back to nil. Calling sqlt.db.WithContext(ctx) in that state dereferences nil. A nil guard (returning a *gorm.DB with an Error set, or a sentinel) would be safer and matches how callers treat the returned handle's .Error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/connectors/sqlite.go` around lines 33 - 35, The DB(ctx) method on
sqliteConnector may dereference nil when sqlt.db is unset; add a nil guard in
sqliteConnector.DB that checks if sqlt.db == nil and, if so, returns a *gorm.DB
whose Error field is set to a descriptive error (e.g., using errors.New("sqlite
connector not connected")) instead of calling WithContext; otherwise return
sqlt.db.WithContext(ctx). Reference the sqliteConnector.DB method and set the
Error on the returned *gorm.DB so callers can check .Error safely.
| func (sqlt *sqliteConnector) Disconnect(ctx context.Context) error { | ||
| sqlt.logger.Debug("Disconnecting sqlite client.") | ||
| db, err := sqlt.db.DB() | ||
| sqlt.db = nil | ||
| if err != nil { | ||
| sqlt.logger.Errorf("Disconnecting sqlite client %s.", err) | ||
| return err | ||
| } | ||
| if err := db.Close(); err != nil { | ||
| sqlt.logger.Debug("Disconnecting sqlite client %s.", err) | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Disconnect will panic if called without a prior successful Connect.
If sqlt.db is nil (e.g., Connect failed or was never called), sqlt.db.DB() on line 91 dereferences a nil *gorm.DB and panics. Add a nil-guard at the top:
🛡️ Proposed fix
func (sqlt *sqliteConnector) Disconnect(ctx context.Context) error {
sqlt.logger.Debug("Disconnecting sqlite client.")
+ if sqlt.db == nil {
+ return nil
+ }
db, err := sqlt.db.DB()
sqlt.db = nilAlso note: the error-log message on line 98 reads "Disconnecting sqlite client %s." — it should describe the failure (e.g., "Failed to close sqlite client").
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (sqlt *sqliteConnector) Disconnect(ctx context.Context) error { | |
| sqlt.logger.Debug("Disconnecting sqlite client.") | |
| db, err := sqlt.db.DB() | |
| sqlt.db = nil | |
| if err != nil { | |
| sqlt.logger.Errorf("Disconnecting sqlite client %s.", err) | |
| return err | |
| } | |
| if err := db.Close(); err != nil { | |
| sqlt.logger.Debug("Disconnecting sqlite client %s.", err) | |
| return err | |
| } | |
| return nil | |
| } | |
| func (sqlt *sqliteConnector) Disconnect(ctx context.Context) error { | |
| sqlt.logger.Debug("Disconnecting sqlite client.") | |
| if sqlt.db == nil { | |
| return nil | |
| } | |
| db, err := sqlt.db.DB() | |
| sqlt.db = nil | |
| if err != nil { | |
| sqlt.logger.Errorf("Disconnecting sqlite client %s.", err) | |
| return err | |
| } | |
| if err := db.Close(); err != nil { | |
| sqlt.logger.Debug("Disconnecting sqlite client %s.", err) | |
| return err | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/connectors/sqlite.go` around lines 89 - 102, Disconnect currently
dereferences sqlt.db without checking for nil and will panic if Connect never
succeeded; update sqliteConnector.Disconnect to first check if sqlt.db == nil
and return nil (or a clear error) before calling sqlt.db.DB(), and only then
call db.Close(); also fix the logger messages in Disconnect (replace ambiguous
"Disconnecting sqlite client %s." with clearer text like "Failed to get
underlying DB: %v" for the DB() error and "Failed to close sqlite client: %v"
for Close() error) so the logged messages accurately describe the failures.
| SQLITE__MAX_OPEN_CONNECTION=1 | ||
| SQLITE__MAX_IDEAL_CONNECTION=1 |
There was a problem hiding this comment.
Typo: MAX_IDEAL_CONNECTION should be MAX_IDLE_CONNECTION.
"Ideal" is almost certainly meant to be "Idle" (as in sql.DB.SetMaxIdleConns). The same typo appears in pkg/configs/sqlite_config.go (MaxIdealConnection / mapstructure:"max_ideal_connection"), so fixing it requires a coordinated rename across the struct field, mapstructure tag, README, and any example env files. Shipping the typo will bake it into the public env-var contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 189 - 190, The README and config struct use a typo
"Ideal" instead of "Idle": rename the struct field MaxIdealConnection to
MaxIdleConnection and update its mapstructure tag from "max_ideal_connection" to
"max_idle_connection", then update all references/usages and example env vars
(change SQLITE__MAX_IDEAL_CONNECTION to SQLITE__MAX_IDLE_CONNECTION) to keep the
env-var contract consistent; ensure any code that reads the old tag/name
(parsing/config loading, tests, docs) is updated to use the new symbol and tag
so the change is applied project-wide.
|
@iamprashant, What do you should i do? think should i follow all the comments from coderabbit? i think some of them are pointless. |
|
Thank @hetsaraiya for creating PR. Will take a look at it in while. |
There was a problem hiding this comment.
Pull request overview
Adds first-class SQLite support across the Go services by introducing a shared SQL configuration/connector abstraction, allowing deployments to run on either Postgres or SQLite (but not both) and skipping bundled migrations when SQLite is selected (since migrations are currently Postgres-specific).
Changes:
- Added
SQLiteConfig,SQLConfigselection helpers, and a newsqliteGORM connector. - Updated service bootstrap/config to select exactly one SQL backend (Postgres or SQLite) and wire it through a unified
SQLConnector. - Updated docs and tests to cover SQLite configuration and multi-SQL rejection behavior.
Reviewed changes
Copilot reviewed 91 out of 91 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/connectors/sqlite_test.go | Adds lifecycle and helper tests for the new SQLite connector. |
| pkg/connectors/sqlite.go | Implements SQLite GORM connector and connection setup helpers. |
| pkg/connectors/sql.go | Adds NewSQLConnector factory to create Postgres/SQLite connectors from a SQLConfig. |
| pkg/connectors/postgres_test.go | Renames tests to reflect SQLConnector naming. |
| pkg/connectors/postgres.go | Renames exported connector interface to SQLConnector and updates constructor return type. |
| pkg/configs/sqlite_config.go | Adds SQLite configuration struct for viper/mapstructure/validation. |
| pkg/configs/sql_config_test.go | Adds tests for SQL config detection/selection and multi-config rejection. |
| pkg/configs/sql_config.go | Adds SQL backend detection/selection logic + DSN/display helpers for migration wiring. |
| pkg/authenticators/service_authenticator.go | Updates constructor signature to accept connectors.SQLConnector. |
| cmd/web/web.go | Wires SQL connector selection and skips migrations when SQLite is selected. |
| cmd/integration/integration.go | Wires SQL connector selection and skips migrations when SQLite is selected. |
| cmd/endpoint/endpoint.go | Wires SQL connector selection and skips migrations when SQLite is selected. |
| cmd/assistant/assistant.go | Wires SQL connector selection and skips migrations when SQLite is selected. |
| api/web-api/router/web.go | Updates router signatures to accept connectors.SQLConnector. |
| api/web-api/router/health-check.go | Updates health-check route wiring to accept connectors.SQLConnector. |
| api/web-api/internal/service/vault/service.go | Updates service constructor/deps to accept connectors.SQLConnector. |
| api/web-api/internal/service/user/service.go | Updates service constructor/deps to accept connectors.SQLConnector. |
| api/web-api/internal/service/project/service.go | Updates service constructor/deps to accept connectors.SQLConnector. |
| api/web-api/internal/service/organization/service.go | Updates service constructor/deps to accept connectors.SQLConnector. |
| api/web-api/internal/service/notification/service.go | Updates service constructor/deps to accept connectors.SQLConnector. |
| api/web-api/internal/connect/slack.go | Updates connect wiring to accept connectors.SQLConnector. |
| api/web-api/internal/connect/notion.go | Updates connect wiring to accept connectors.SQLConnector. |
| api/web-api/internal/connect/microsoft.go | Updates connect wiring to accept connectors.SQLConnector. |
| api/web-api/internal/connect/linkedin.go | Updates connect wiring to accept connectors.SQLConnector. |
| api/web-api/internal/connect/hubspot.go | Updates connect wiring to accept connectors.SQLConnector. |
| api/web-api/internal/connect/google.go | Updates connect wiring to accept connectors.SQLConnector. |
| api/web-api/internal/connect/gitlab.go | Updates connect wiring to accept connectors.SQLConnector. |
| api/web-api/internal/connect/github.go | Updates connect wiring to accept connectors.SQLConnector. |
| api/web-api/internal/connect/external.go | Updates shared external connect wiring to accept connectors.SQLConnector. |
| api/web-api/internal/connect/atlassian.go | Updates connect wiring to accept connectors.SQLConnector. |
| api/web-api/config/config_test.go | Adds SQLite + multi-SQL config validation tests for web-api config. |
| api/web-api/config/config.go | Updates config struct to support Postgres/SQLite pointers + SQL selection helper. |
| api/web-api/authenticator/authenticator.go | Updates authenticators wiring to accept connectors.SQLConnector. |
| api/web-api/api/web.go | Updates API wiring to accept connectors.SQLConnector. |
| api/web-api/api/vault.go | Updates vault API wiring to accept connectors.SQLConnector. |
| api/web-api/api/proxy/knowledge.go | Updates proxy API wiring to accept connectors.SQLConnector. |
| api/web-api/api/proxy/invoke.go | Updates proxy API wiring to accept connectors.SQLConnector. |
| api/web-api/api/proxy/endpoint.go | Updates proxy API wiring to accept connectors.SQLConnector. |
| api/web-api/api/proxy/document.go | Updates proxy API wiring to accept connectors.SQLConnector. |
| api/web-api/api/proxy/assistant_deployment.go | Updates proxy API wiring to accept connectors.SQLConnector. |
| api/web-api/api/proxy/assistant.go | Updates proxy API wiring to accept connectors.SQLConnector. |
| api/web-api/api/proxy/activity.go | Updates proxy API wiring to accept connectors.SQLConnector. |
| api/web-api/api/project.go | Updates project API wiring to accept connectors.SQLConnector. |
| api/web-api/api/organization.go | Updates organization API wiring to accept connectors.SQLConnector. |
| api/web-api/api/notification.go | Updates notification API wiring to accept connectors.SQLConnector. |
| api/web-api/api/connect.go | Updates connect API wiring to accept connectors.SQLConnector. |
| api/web-api/api/auth.go | Updates auth API wiring to accept connectors.SQLConnector. |
| api/integration-api/router/provider.go | Updates router wiring to accept connectors.SQLConnector. |
| api/integration-api/router/health-check.go | Updates health-check route wiring to accept connectors.SQLConnector. |
| api/integration-api/internal/service/audit/service.go | Updates audit service deps to accept connectors.SQLConnector. |
| api/integration-api/config/config_test.go | Adds SQLite + multi-SQL config validation tests for integration-api config. |
| api/integration-api/config/config.go | Updates config struct to support Postgres/SQLite pointers + SQL selection helper. |
| api/integration-api/api/unified_provider.go | Updates API wiring to accept connectors.SQLConnector. |
| api/integration-api/api/integration.go | Updates API wiring to accept connectors.SQLConnector. |
| api/integration-api/api/audit_logging.go | Updates API wiring to accept connectors.SQLConnector. |
| api/endpoint-api/router/health-check.go | Updates health-check route wiring to accept connectors.SQLConnector. |
| api/endpoint-api/router/endpoint.go | Updates router wiring to accept connectors.SQLConnector. |
| api/endpoint-api/internal/service/log/service.go | Updates log service deps to accept connectors.SQLConnector. |
| api/endpoint-api/internal/service/endpoint/service.go | Updates endpoint service deps to accept connectors.SQLConnector. |
| api/endpoint-api/config/config_test.go | Adds SQLite + multi-SQL config validation tests for endpoint-api config. |
| api/endpoint-api/config/config.go | Updates config struct to support Postgres/SQLite pointers + SQL selection helper. |
| api/endpoint-api/api/invoker-endpoint.go | Updates invoker API wiring to accept connectors.SQLConnector. |
| api/endpoint-api/api/endpoint.go | Updates endpoint API wiring to accept connectors.SQLConnector. |
| api/assistant-api/socket/socket.go | Updates socket engine wiring to accept connectors.SQLConnector. |
| api/assistant-api/socket/pipeline.go | Updates pipeline wiring to accept connectors.SQLConnector. |
| api/assistant-api/sip/sip.go | Updates SIP engine wiring to accept connectors.SQLConnector. |
| api/assistant-api/router/knowledge.go | Updates router wiring to accept connectors.SQLConnector. |
| api/assistant-api/router/health-check.go | Updates health-check route wiring to accept connectors.SQLConnector. |
| api/assistant-api/router/assistant.go | Updates router wiring to accept connectors.SQLConnector. |
| api/assistant-api/internal/services/knowledge/knowledge.impl.service.go | Updates service deps to accept connectors.SQLConnector. |
| api/assistant-api/internal/services/knowledge/document.impl.service.go | Updates service deps to accept connectors.SQLConnector. |
| api/assistant-api/internal/services/assistant/webhook.impl.service.go | Updates service deps to accept connectors.SQLConnector. |
| api/assistant-api/internal/services/assistant/tool.impl.service.go | Updates service deps to accept connectors.SQLConnector. |
| api/assistant-api/internal/services/assistant/telemetry.impl.service.go | Updates service deps to accept connectors.SQLConnector. |
| api/assistant-api/internal/services/assistant/knowledge.impl.service.go | Updates service deps to accept connectors.SQLConnector. |
| api/assistant-api/internal/services/assistant/conversaction.impl.service.go | Updates service deps to accept connectors.SQLConnector. |
| api/assistant-api/internal/services/assistant/assistant.impl.service.go | Updates service deps to accept connectors.SQLConnector. |
| api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go | Updates service deps to accept connectors.SQLConnector. |
| api/assistant-api/internal/services/assistant/analysis.impl.service.go | Updates service deps to accept connectors.SQLConnector. |
| api/assistant-api/internal/callcontext/store.go | Updates store deps to accept connectors.SQLConnector. |
| api/assistant-api/internal/adapters/request_test.go | Updates test documentation/signature expectations to connectors.SQLConnector. |
| api/assistant-api/internal/adapters/request.go | Updates adapter wiring to accept connectors.SQLConnector. |
| api/assistant-api/internal/adapters/internal/requestor.go | Updates requestor deps to accept connectors.SQLConnector. |
| api/assistant-api/config/config_test.go | Adds SQLite + multi-SQL config validation tests for assistant-api config. |
| api/assistant-api/config/config.go | Updates config struct to support Postgres/SQLite pointers + SQL selection helper. |
| api/assistant-api/api/talk/talk.go | Updates conversation/talk API wiring to accept connectors.SQLConnector. |
| api/assistant-api/api/knowledge/knowledge.go | Updates knowledge API wiring to accept connectors.SQLConnector. |
| api/assistant-api/api/knowledge/indexer.go | Updates indexer API wiring to accept connectors.SQLConnector. |
| api/assistant-api/api/assistant/assistant.go | Updates assistant API wiring to accept connectors.SQLConnector. |
| api/assistant-api/api/assistant-deployment/assistant_deployment.go | Updates deployment API wiring to accept connectors.SQLConnector. |
| README.md | Documents minimal SQLite setup and migration caveat. |
| func (sqlt *sqliteConnector) Disconnect(ctx context.Context) error { | ||
| sqlt.logger.Debug("Disconnecting sqlite client.") | ||
| db, err := sqlt.db.DB() | ||
| sqlt.db = nil | ||
| if err != nil { |
There was a problem hiding this comment.
Disconnect() assumes sqlt.db is non-nil and calls sqlt.db.DB() unconditionally. If Disconnect() is called before Connect() succeeds, or called twice during shutdown, this will panic. Add a nil guard (and consider making Disconnect idempotent) before dereferencing sqlt.db.
| if err := db.Close(); err != nil { | ||
| sqlt.logger.Debug("Disconnecting sqlite client %s.", err) | ||
| return err |
There was a problem hiding this comment.
On db.Close() error, this logs via logger.Debug with a printf-style format string, so the %s placeholder will not be expanded. Use the formatted logging method (e.g., Debugf/Errorf) and log close failures at an error level so they are visible in production.
| func NewSQLConnector(config configs.SQLConfig, logger commons.Logger) SQLConnector { | ||
| switch cfg := config.(type) { | ||
| case *configs.PostgresConfig: | ||
| return NewPostgresConnector(cfg, logger) | ||
| case *configs.SQLiteConfig: | ||
| return NewSQLiteConnector(cfg, logger) | ||
| default: | ||
| return nil | ||
| } |
There was a problem hiding this comment.
NewSQLConnector returns nil for unknown SQLConfig implementations. That makes misconfiguration or future extension failures show up later as nil-pointer panics (e.g., when calling Connect/DB). Prefer failing fast (panic with a clear message) or changing the API to return (SQLConnector, error).
| func SelectedSQLConfig(postgres *PostgresConfig, sqlite *SQLiteConfig) SQLConfig { | ||
| if postgres != nil && sqlite != nil { | ||
| panic("SelectedSQLConfig requires exactly one SQL config; both postgres and sqlite were provided") | ||
| } | ||
| if sqlite != nil { | ||
| return sqlite | ||
| } | ||
| return postgres | ||
| } |
There was a problem hiding this comment.
SelectedSQLConfig panics when both configs are non-nil. Since this helper is exposed and used by service configs, a panic here will crash the process (and bypass normal config validation) if a caller constructs the config struct incorrectly. Prefer returning (SQLConfig, error) or reusing ResolveSQLConfig to produce a validated selection without panicking.
| func hasConfigPrefix(v *viper.Viper, prefix string) bool { | ||
| prefix = strings.ToUpper(prefix) + "__" | ||
| for _, key := range v.AllKeys() { | ||
| normalizedKey := strings.ToUpper(strings.ReplaceAll(key, ".", "__")) | ||
| if strings.HasPrefix(normalizedKey, prefix) { | ||
| return true | ||
| } | ||
| } | ||
| for _, env := range os.Environ() { | ||
| name, _, found := strings.Cut(env, "=") | ||
| if found && strings.HasPrefix(strings.ToUpper(name), prefix) { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
ResolveSQLConfig/hasConfigPrefix inspects the full process environment (os.Environ) in addition to the provided viper instance. This can make behavior and unit tests non-deterministic (e.g., unrelated POSTGRES__/SQLITE__ vars in the environment can trigger ErrMultipleSQLConfigsDetected). Consider relying on viper’s env binding (AutomaticEnv + key replacer) and only checking viper keys, or make environment inspection injectable/opt-in for tests.
| func TestGetApplicationConfig_SQLite(t *testing.T) { | ||
| vConfig := viper.NewWithOptions(viper.KeyDelimiter("__")) | ||
| vConfig.Set("SERVICE_NAME", "endpoint-api") | ||
| vConfig.Set("HOST", "0.0.0.0") | ||
| vConfig.Set("PORT", 9005) | ||
| vConfig.Set("LOG_LEVEL", "debug") | ||
| vConfig.Set("SECRET", "rpd_pks") | ||
| vConfig.Set("ENV", "development") | ||
|
|
||
| vConfig.Set("SQLITE__PATH", filepath.Join(t.TempDir(), "integration.db")) | ||
| vConfig.Set("SQLITE__MAX_OPEN_CONNECTION", 1) | ||
| vConfig.Set("SQLITE__MAX_IDEAL_CONNECTION", 1) | ||
|
|
There was a problem hiding this comment.
These new integration-api config tests set SERVICE_NAME to "endpoint-api" and use endpoint-oriented paths/names (e.g., assets/endpoint, integration.db). This looks like copy/paste from endpoint-api and makes the integration-api config tests misleading. Align SERVICE_NAME and path prefixes with integration-api to reflect the intended configuration.
| func (sqlt *sqliteConnector) DB(ctx context.Context) *gorm.DB { | ||
| return sqlt.db.WithContext(ctx) | ||
| } |
There was a problem hiding this comment.
sqliteConnector.DB() calls sqlt.db.WithContext(ctx) without checking whether sqlt.db is nil. If DB() is called before Connect() or after Disconnect(), this will panic. Consider returning nil (and documenting it) or guarding with an explicit panic/message so failures are clearer and safer in shutdown/health-check paths.
| if err := db.Exec("PRAGMA foreign_keys = ON").Error; err != nil { | ||
| sqlt.logger.Errorf("Failed to enable sqlite foreign keys %s.", err) | ||
| return err | ||
| } |
There was a problem hiding this comment.
SQLite foreign key enforcement is connection-specific. Executing PRAGMA foreign_keys = ON once may only affect the current connection, and pooled connections can still run with foreign keys disabled when MaxOpenConnection > 1. Prefer enabling it via the SQLite DSN options (e.g., _foreign_keys=1) or by ensuring the PRAGMA is applied for every new connection.
- Rebase onto upstream main and resolve merge conflicts - Rename MaxIdealConnection to MaxIdleConnection (config, env, docker, README) - Use validate gte=0 for pool ints; allow unlimited (0) pool sizes - SelectedSQLConfig and SQLConfig() return (SQLConfig, error); no panic/typed-nil - NewSQLConnector returns (SQLConnector, error); fail fast on bad config - Postgres MigrationDSN via net/url; add MigrationDSN tests - hasConfigPrefix uses viper AllKeys/IsSet only (no os.Environ) - SQLite: nil guards on DB/Disconnect/Query; FK via DSN _foreign_keys=1 - Register golang-migrate sqlite3 driver in cmd/* entrypoints - Fix integration-api config tests (service name, DB, asset paths) - Expand sqlite connector tests; fix viper nested host keys in SQL tests - PostgresConnector type alias for SQLConnector (upstream SIP compat)
8c1a5b7 to
8e9fb2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/assistant-api/config/config.go`:
- Around line 118-141: The config-loading paths currently return unwrapped
errors, making it hard to tell whether failure came from SQL resolution, struct
validation, or SQL selection. Update GetApplicationConfig() to wrap the errors
from configs.ResolveSQLConfig and validate.Struct with step-specific context
using fmt.Errorf and %w, and do the same in AssistantConfig.SQLConfig() for
configs.SelectedSQLConfig so callers can identify the failing phase from the
returned error.
- Around line 98-104: InitConfig currently swallows ReadInConfig failures by
logging them and still returning success, which hides malformed config,
permission, and ENV_PATH issues. Update the InitConfig flow around
vConfig.ReadInConfig() to return the read error instead of always returning nil,
and handle the missing-file case explicitly by checking for
viper.ConfigFileNotFoundError (or equivalent) if missing configs are allowed.
Keep the existing config-loading logic intact, but make sure callers can see the
real failure from InitConfig rather than a later validation error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1f90391-c677-432c-8c41-6fea87c9848b
📒 Files selected for processing (12)
README.mdapi/assistant-api/api/assistant-deployment/assistant_deployment.goapi/assistant-api/api/assistant/assistant.goapi/assistant-api/api/knowledge/indexer.goapi/assistant-api/api/knowledge/knowledge.goapi/assistant-api/api/talk/talk.goapi/assistant-api/config/config.goapi/assistant-api/config/config_test.goapi/assistant-api/internal/adapters/internal/requestor.goapi/assistant-api/internal/adapters/request.goapi/assistant-api/internal/adapters/request_test.goapi/assistant-api/internal/callcontext/store.go
💤 Files with no reviewable changes (4)
- api/assistant-api/internal/adapters/request_test.go
- api/assistant-api/internal/adapters/internal/requestor.go
- api/assistant-api/internal/adapters/request.go
- api/assistant-api/internal/callcontext/store.go
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- api/assistant-api/api/assistant/assistant.go
- api/assistant-api/api/knowledge/indexer.go
- api/assistant-api/api/assistant-deployment/assistant_deployment.go
- api/assistant-api/config/config_test.go
- api/assistant-api/api/talk/talk.go
- api/assistant-api/api/knowledge/knowledge.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/assistant-api/config/config.go`:
- Around line 118-141: The config-loading paths currently return unwrapped
errors, making it hard to tell whether failure came from SQL resolution, struct
validation, or SQL selection. Update GetApplicationConfig() to wrap the errors
from configs.ResolveSQLConfig and validate.Struct with step-specific context
using fmt.Errorf and %w, and do the same in AssistantConfig.SQLConfig() for
configs.SelectedSQLConfig so callers can identify the failing phase from the
returned error.
- Around line 98-104: InitConfig currently swallows ReadInConfig failures by
logging them and still returning success, which hides malformed config,
permission, and ENV_PATH issues. Update the InitConfig flow around
vConfig.ReadInConfig() to return the read error instead of always returning nil,
and handle the missing-file case explicitly by checking for
viper.ConfigFileNotFoundError (or equivalent) if missing configs are allowed.
Keep the existing config-loading logic intact, but make sure callers can see the
real failure from InitConfig rather than a later validation error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1f90391-c677-432c-8c41-6fea87c9848b
📒 Files selected for processing (12)
README.mdapi/assistant-api/api/assistant-deployment/assistant_deployment.goapi/assistant-api/api/assistant/assistant.goapi/assistant-api/api/knowledge/indexer.goapi/assistant-api/api/knowledge/knowledge.goapi/assistant-api/api/talk/talk.goapi/assistant-api/config/config.goapi/assistant-api/config/config_test.goapi/assistant-api/internal/adapters/internal/requestor.goapi/assistant-api/internal/adapters/request.goapi/assistant-api/internal/adapters/request_test.goapi/assistant-api/internal/callcontext/store.go
💤 Files with no reviewable changes (4)
- api/assistant-api/internal/adapters/request_test.go
- api/assistant-api/internal/adapters/internal/requestor.go
- api/assistant-api/internal/adapters/request.go
- api/assistant-api/internal/callcontext/store.go
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- api/assistant-api/api/assistant/assistant.go
- api/assistant-api/api/knowledge/indexer.go
- api/assistant-api/api/assistant-deployment/assistant_deployment.go
- api/assistant-api/config/config_test.go
- api/assistant-api/api/talk/talk.go
- api/assistant-api/api/knowledge/knowledge.go
🛑 Comments failed to post (2)
api/assistant-api/config/config.go (2)
98-104: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,180p' api/assistant-api/config/config.go | cat -nRepository: rapidaai/voice-ai
Length of output: 6542
🏁 Script executed:
rg -n "InitConfig\(\)|ENV_PATH|assistant.yaml" -S .Repository: rapidaai/voice-ai
Length of output: 2294
🏁 Script executed:
sed -n '1,140p' api/web-api/config/config.go | cat -n && printf '\n---\n' && sed -n '1,180p' api/assistant-api/config/config_test.go | cat -nRepository: rapidaai/voice-ai
Length of output: 10908
Return
ReadInConfig()failures instead of swallowing them.
InitConfig()logs the error and still returnsnil, so malformed YAML, permission issues, or a badENV_PATHonly show up later as misleading validation errors. If missing files are allowed, handleviper.ConfigFileNotFoundErrorexplicitly; otherwise return the read error here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/assistant-api/config/config.go` around lines 98 - 104, InitConfig currently swallows ReadInConfig failures by logging them and still returning success, which hides malformed config, permission, and ENV_PATH issues. Update the InitConfig flow around vConfig.ReadInConfig() to return the read error instead of always returning nil, and handle the missing-file case explicitly by checking for viper.ConfigFileNotFoundError (or equivalent) if missing configs are allowed. Keep the existing config-loading logic intact, but make sure callers can see the real failure from InitConfig rather than a later validation error.
118-141: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the target file and locate related config helpers. ast-grep outline api/assistant-api/config/config.go --view expanded || true printf '\n--- config.go (relevant slices) ---\n' sed -n '1,220p' api/assistant-api/config/config.go | cat -n printf '\n--- search ResolveSQLConfig / SelectedSQLConfig definitions and call sites ---\n' rg -n "ResolveSQLConfig|SelectedSQLConfig|validate\.Struct\(&config\)|SQLConfig\(\)" api pkg cmd --glob '*.go' printf '\n--- open matching helper definitions if found ---\n' for f in $(rg -l "func .*ResolveSQLConfig|func .*SelectedSQLConfig" api pkg cmd --glob '*.go' || true); do echo "### $f" sed -n '1,260p' "$f" | cat -n doneRepository: rapidaai/voice-ai
Length of output: 23987
🏁 Script executed:
#!/bin/bash set -euo pipefail for f in api/endpoint-api/config/config.go api/integration-api/config/config.go api/web-api/config/config.go api/assistant-api/config/config.go; do echo "### $f" sed -n '1,220p' "$f" | cat -n echo doneRepository: rapidaai/voice-ai
Length of output: 16617
Wrap the config-load errors with context.
GetApplicationConfig()still returns raw errors from SQL resolution and final validation, andSQLConfig()returns the selector error directly. Wrap each withfmt.Errorf("%s: %w", ...)so startup logs and callers can tell which step failed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/assistant-api/config/config.go` around lines 118 - 141, The config-loading paths currently return unwrapped errors, making it hard to tell whether failure came from SQL resolution, struct validation, or SQL selection. Update GetApplicationConfig() to wrap the errors from configs.ResolveSQLConfig and validate.Struct with step-specific context using fmt.Errorf and %w, and do the same in AssistantConfig.SQLConfig() for configs.SelectedSQLConfig so callers can identify the failing phase from the returned error.Source: Coding guidelines
- InitConfig: return ReadInConfig errors; allow missing default yaml only - GetApplicationConfig/SQLConfig: wrap errors with step context (all APIs) - Move SQLConnector interface to pkg/connectors/sql.go - Rename callcontext postgresStore to sqlStore - SafeMigrationDSNForLog helper + test (for future migrate logging) - sql_config_test: use errors.Is for sentinels - vault service: fix Debugf missing error argument
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 (2)
api/endpoint-api/config/config.go (1)
31-56: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd
config_test.gocoverage for the new config flow.
InitConfig,GetApplicationConfig, andSQLConfig()now own missing-file handling, backend selection, and wrapped error paths, but this package’s tests were not updated with the change. Please cover theENV_PATH/not-found branch, postgres vs sqlite normalization, and SQL selection errors. As per coding guidelines, backend changes inapi/**must include corresponding*_test.goupdates in the same package, and backend tests should include success, fallback/error, and factory/selection assertions.Also applies to: 59-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/endpoint-api/config/config.go` around lines 31 - 56, The config package tests need to be updated to cover the new InitConfig flow and related backend selection behavior. Add config_test.go cases for InitConfig’s ENV_PATH branch and the not-found fallback, plus wrapped read errors when a custom path is set, using InitConfig as the target symbol. Also add assertions for GetApplicationConfig and SQLConfig to verify postgres vs sqlite normalization and the SQL selection/error paths, so the package has success, fallback/error, and factory/selection coverage.Source: Coding guidelines
api/integration-api/config/config.go (1)
26-50: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd
config_test.gocoverage for the new integration config flow.This package now changes missing-file behavior, SQL backend resolution, and wrapped error propagation, but the same-package tests were not updated with the new branches. Please cover the
ENV_PATH/not-found case, postgres vs sqlite normalization, and SQL selection failures. As per coding guidelines, backend changes inapi/**must include corresponding*_test.goupdates in the same package, and backend tests should include success, fallback/error, and factory/selection assertions.Also applies to: 54-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/integration-api/config/config.go` around lines 26 - 50, `InitConfig` needs same-package test coverage for the new config-loading branches: add tests that exercise `ENV_PATH` being set with a missing file versus the default `./env/integration.yaml` not-found fallback, and assert the wrapped error path when `ReadInConfig` fails for reasons other than `viper.ConfigFileNotFoundError`. Update or add `config_test.go` in the same package so the new behavior is verified alongside the existing config flow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/assistant-api/internal/callcontext/store.go`:
- Around line 46-56: Update store_test.go to cover the widened
SQLConnector-based contract in NewStore and sqlStore. Add assertions that the
constructor wires the provided connectors.SQLConnector and logger correctly,
then add at least one success-path test and one failure-path test through the
generic store methods to exercise the broader backend behavior. Make sure the
tests in the callcontext package also include the required factory/selection and
fallback/error coverage for the SQL-backed implementation.
In `@pkg/connectors/sql.go`:
- Around line 18-27: Add same-package tests in pkg/connectors to cover the new
SQL factory contract and backend dispatch. Update or add *_test.go cases around
the relevant factory/selection symbols in this package (for example
SQLConnector, PostgresConnector, and any new factory/dispatch function) to
assert postgres/sqlite selection, fallback behavior, and nil/unsupported config
errors. Make sure the tests include success, error, and backend-selection
assertions so the widened connector surface is fully covered.
---
Outside diff comments:
In `@api/endpoint-api/config/config.go`:
- Around line 31-56: The config package tests need to be updated to cover the
new InitConfig flow and related backend selection behavior. Add config_test.go
cases for InitConfig’s ENV_PATH branch and the not-found fallback, plus wrapped
read errors when a custom path is set, using InitConfig as the target symbol.
Also add assertions for GetApplicationConfig and SQLConfig to verify postgres vs
sqlite normalization and the SQL selection/error paths, so the package has
success, fallback/error, and factory/selection coverage.
In `@api/integration-api/config/config.go`:
- Around line 26-50: `InitConfig` needs same-package test coverage for the new
config-loading branches: add tests that exercise `ENV_PATH` being set with a
missing file versus the default `./env/integration.yaml` not-found fallback, and
assert the wrapped error path when `ReadInConfig` fails for reasons other than
`viper.ConfigFileNotFoundError`. Update or add `config_test.go` in the same
package so the new behavior is verified alongside the existing config flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4f1e017-6a56-4d8a-a3aa-3f56fcc339f1
📒 Files selected for processing (11)
api/assistant-api/config/config.goapi/assistant-api/internal/callcontext/store.goapi/endpoint-api/config/config.goapi/integration-api/config/config.goapi/web-api/config/config.goapi/web-api/internal/service/vault/service.gopkg/configs/migration_dsn_log.gopkg/configs/migration_dsn_log_test.gopkg/configs/sql_config_test.gopkg/connectors/postgres.gopkg/connectors/sql.go
💤 Files with no reviewable changes (1)
- pkg/connectors/postgres.go
✅ Files skipped from review due to trivial changes (1)
- pkg/configs/migration_dsn_log.go
🚧 Files skipped from review as they are similar to previous changes (3)
- api/web-api/internal/service/vault/service.go
- api/web-api/config/config.go
- api/assistant-api/config/config.go
| type sqlStore struct { | ||
| sql connectors.SQLConnector | ||
| logger commons.Logger | ||
| } | ||
|
|
||
| func NewStore(postgres connectors.PostgresConnector, logger commons.Logger) Store { | ||
| return &postgresStore{ | ||
| postgres: postgres, | ||
| logger: logger, | ||
| func NewStore(sql connectors.SQLConnector, logger commons.Logger) Store { | ||
| return &sqlStore{ | ||
| sql: sql, | ||
| logger: logger, | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Update store_test.go for the widened SQL store contract.
NewStore and the store methods now accept connectors.SQLConnector, but the package tests were not updated to exercise that broader backend contract. Please add coverage for constructor wiring plus at least one success path and one failure path through the generic SQL store. As per coding guidelines, backend changes in api/** must include corresponding *_test.go updates in the same package, and backend tests should include success, fallback/error, and factory/selection assertions.
Also applies to: 58-189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/assistant-api/internal/callcontext/store.go` around lines 46 - 56, Update
store_test.go to cover the widened SQLConnector-based contract in NewStore and
sqlStore. Add assertions that the constructor wires the provided
connectors.SQLConnector and logger correctly, then add at least one success-path
test and one failure-path test through the generic store methods to exercise the
broader backend behavior. Make sure the tests in the callcontext package also
include the required factory/selection and fallback/error coverage for the
SQL-backed implementation.
Source: Coding guidelines
| // SQLConnector is the shared database connector abstraction (Postgres, SQLite, etc.). | ||
| type SQLConnector interface { | ||
| Connector | ||
| Query(ctx context.Context, qry string, dest interface{}) error | ||
| DB(ctx context.Context) *gorm.DB | ||
| } | ||
|
|
||
| // PostgresConnector is an alias for SQLConnector for backward compatibility. | ||
| type PostgresConnector = SQLConnector | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add pkg/connectors coverage for the new SQL factory contract.
This change widens the exported connector surface and adds backend dispatch, but there’s no same-package test update covering postgres/sqlite selection plus nil/unsupported config errors. As per coding guidelines, backend changes in pkg/** must include corresponding *_test.go updates in the same package, including success, fallback/error, and factory/selection assertions.
Also applies to: 28-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/connectors/sql.go` around lines 18 - 27, Add same-package tests in
pkg/connectors to cover the new SQL factory contract and backend dispatch.
Update or add *_test.go cases around the relevant factory/selection symbols in
this package (for example SQLConnector, PostgresConnector, and any new
factory/dispatch function) to assert postgres/sqlite selection, fallback
behavior, and nil/unsupported config errors. Make sure the tests include
success, error, and backend-selection assertions so the widened connector
surface is fully covered.
Source: Coding guidelines
- pkg/connectors/sql_test.go: postgres/sqlite dispatch, nil, unsupported type - Remove skipped TestNewSQLConnector; dedupe nil test from sqlite_test - callcontext: rename testSQLConnector; NewStore wiring + Get not-found - endpoint/integration init_config_test: ENV_PATH missing/invalid yaml, default missing file allowed, SQLConfig postgres and no-backend errors
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/assistant-api/internal/callcontext/store_test.go (1)
29-75: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose the GORM SQLite handle in
newTestStore.Each call to
newTestStoreopens a real SQLite connection and never closes the underlyingsql.DB. Repeated tests can accumulate open handles in the package test process.Suggested patch
db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) if err != nil { t.Fatalf("failed to create sqlite db: %v", err) } + sqlDB, err := db.DB() + if err != nil { + t.Fatalf("failed to get sql db: %v", err) + } + t.Cleanup(func() { _ = sqlDB.Close() }) if err := db.Exec(`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/assistant-api/internal/callcontext/store_test.go` around lines 29 - 75, The newTestStore helper opens a real in-memory SQLite connection via gorm.Open but never closes the underlying sql.DB, so add cleanup in the helper by retrieving the raw DB from the GORM handle and closing it when the test ends. Use the existing newTestStore setup around gorm.Open, db.Exec, and NewStore to locate the change, and register the close with the test lifecycle so repeated tests do not leak handles.
🧹 Nitpick comments (3)
api/integration-api/config/init_config_test.go (1)
63-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd SQLite and ambiguous-config selection cases here.
This file only asserts Postgres selection and the no-backend error. The new config contract also needs a SQLite-only success case and a both-configured rejection case, otherwise the new selection branch can regress without this package noticing.
As per coding guidelines, "Include at least a success path, fallback/error path, and factory/selection behavior assertion in backend tests." As per PR objectives, acceptance requires "successful startup with only SQLite config" and "validation rejection of ambiguous multi-SQL configuration."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/integration-api/config/init_config_test.go` around lines 63 - 91, Add coverage for the missing SQL selection branches in SQLConfig by extending the IntegrationConfig tests. In init_config_test.go, add a SQLite-only success case that verifies cfg.SQLConfig() selects the SQLite backend and returns the expected driver name, and add a both-configured rejection case that asserts ambiguous SQL config is rejected with the expected error. Keep the existing Postgres and no-backend assertions, and use IntegrationConfig.SQLConfig plus GetApplicationConfig to exercise the selection logic end to end.Source: Coding guidelines
api/endpoint-api/config/init_config_test.go (1)
63-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover SQLite and multi-backend rejection in these config tests too.
These assertions only prove the Postgres path and the no-backend error. Since this PR adds SQLite selection and explicit rejection of ambiguous SQL config, both cases should be pinned in this package as well.
As per coding guidelines, "Include at least a success path, fallback/error path, and factory/selection behavior assertion in backend tests." As per PR objectives, acceptance requires "successful startup with only SQLite config" and "validation rejection of ambiguous multi-SQL configuration."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/endpoint-api/config/init_config_test.go` around lines 63 - 91, The current tests only cover the Postgres success path and the no-backend error in EndpointConfig.SQLConfig, so add coverage for the new SQLite path and ambiguous backend rejection. In init_config_test.go, extend the SQLConfig tests around GetApplicationConfig and SQLConfig to assert that a config with only SQLite selected returns a SQLite driver, and that a config with both SQL backends configured fails with the expected selection/validation error. Keep the existing no-backend assertion and make sure the new cases use the same EndpointConfig and SQLConfig symbols so the selection behavior is pinned in this package.Source: Coding guidelines
pkg/connectors/sql_test.go (1)
16-16: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFail fast if logger setup fails.
These tests ignore
commons.NewApplicationLogger()errors, which can turn a setup failure into a misleading connector failure later. Userequire.NoError(t, err)before passing the logger onward.Also applies to: 33-33, 48-48, 55-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connectors/sql_test.go` at line 16, The test setup is ignoring errors from commons.NewApplicationLogger(), which can hide logger initialization failures and cause misleading connector test failures later. In each affected test in sql_test.go, capture the returned error from commons.NewApplicationLogger() and assert it with require.NoError(t, err) before using the logger variable, then pass the logger onward only after that check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/connectors/sql_test.go`:
- Around line 42-45: The SQLite connector test currently calls Connect on a real
database but never exercises cleanup, so update the test to close the connector
during teardown. Add a cleanup step in the same test function (around the
existing connector.Connect and connector.Name assertions) that invokes the
connector’s disconnect/close path so the lifecycle is fully covered and temp-dir
file handles are released. Use the connector instance created in this test and
the existing connect/disconnect methods on the SQL connector type to keep the
fix localized.
---
Outside diff comments:
In `@api/assistant-api/internal/callcontext/store_test.go`:
- Around line 29-75: The newTestStore helper opens a real in-memory SQLite
connection via gorm.Open but never closes the underlying sql.DB, so add cleanup
in the helper by retrieving the raw DB from the GORM handle and closing it when
the test ends. Use the existing newTestStore setup around gorm.Open, db.Exec,
and NewStore to locate the change, and register the close with the test
lifecycle so repeated tests do not leak handles.
---
Nitpick comments:
In `@api/endpoint-api/config/init_config_test.go`:
- Around line 63-91: The current tests only cover the Postgres success path and
the no-backend error in EndpointConfig.SQLConfig, so add coverage for the new
SQLite path and ambiguous backend rejection. In init_config_test.go, extend the
SQLConfig tests around GetApplicationConfig and SQLConfig to assert that a
config with only SQLite selected returns a SQLite driver, and that a config with
both SQL backends configured fails with the expected selection/validation error.
Keep the existing no-backend assertion and make sure the new cases use the same
EndpointConfig and SQLConfig symbols so the selection behavior is pinned in this
package.
In `@api/integration-api/config/init_config_test.go`:
- Around line 63-91: Add coverage for the missing SQL selection branches in
SQLConfig by extending the IntegrationConfig tests. In init_config_test.go, add
a SQLite-only success case that verifies cfg.SQLConfig() selects the SQLite
backend and returns the expected driver name, and add a both-configured
rejection case that asserts ambiguous SQL config is rejected with the expected
error. Keep the existing Postgres and no-backend assertions, and use
IntegrationConfig.SQLConfig plus GetApplicationConfig to exercise the selection
logic end to end.
In `@pkg/connectors/sql_test.go`:
- Line 16: The test setup is ignoring errors from
commons.NewApplicationLogger(), which can hide logger initialization failures
and cause misleading connector test failures later. In each affected test in
sql_test.go, capture the returned error from commons.NewApplicationLogger() and
assert it with require.NoError(t, err) before using the logger variable, then
pass the logger onward only after that check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 635e2294-d7a2-49de-aa72-d3389a7afd49
📒 Files selected for processing (6)
api/assistant-api/internal/callcontext/store_test.goapi/endpoint-api/config/init_config_test.goapi/integration-api/config/init_config_test.gopkg/connectors/postgres_test.gopkg/connectors/sql_test.gopkg/connectors/sqlite_test.go
💤 Files with no reviewable changes (1)
- pkg/connectors/sqlite_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connectors/postgres_test.go
| ctx := context.Background() | ||
| require.NoError(t, connector.Connect(ctx)) | ||
| assert.Contains(t, connector.Name(), "SQLITE") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the SQLite connector in test cleanup.
This test opens a real SQLite database under t.TempDir() but never disconnects it. That leaves the lifecycle only half-tested and can leak file handles during temp-dir cleanup.
Suggested patch
ctx := context.Background()
require.NoError(t, connector.Connect(ctx))
+ t.Cleanup(func() {
+ require.NoError(t, connector.Disconnect(ctx))
+ })
assert.Contains(t, connector.Name(), "SQLITE")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ctx := context.Background() | |
| require.NoError(t, connector.Connect(ctx)) | |
| assert.Contains(t, connector.Name(), "SQLITE") | |
| } | |
| ctx := context.Background() | |
| require.NoError(t, connector.Connect(ctx)) | |
| t.Cleanup(func() { | |
| require.NoError(t, connector.Disconnect(ctx)) | |
| }) | |
| assert.Contains(t, connector.Name(), "SQLITE") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/connectors/sql_test.go` around lines 42 - 45, The SQLite connector test
currently calls Connect on a real database but never exercises cleanup, so
update the test to close the connector during teardown. Add a cleanup step in
the same test function (around the existing connector.Connect and connector.Name
assertions) that invokes the connector’s disconnect/close path so the lifecycle
is fully covered and temp-dir file handles are released. Use the connector
instance created in this test and the existing connect/disconnect methods on the
SQL connector type to keep the fix localized.
Summary
Adds SQLite support for Go services alongside Postgres.
Resolves #78.
Changes
Validation
Summary by CodeRabbit
New Features
POSTGRES__*vs SQLiteSQLITE__*), ensuring only one is enabled.Bug Fixes
Documentation
SQLITE__*example settings and migration caveats.