Repository navigation
refactor(storage): serve both SQL backends from one store implementation - #586
Merged
Merged
Conversation
…ends SQLite and PostgreSQL stores were written twice across 17 domains. Comparing the halves, the differences were never business logic: the driver handle, `?` vs $n placeholders, the no-rows sentinel, the Exec result shape, and a handful of DDL type names. internal/storage/sqlx absorbs exactly those five. Value binding is deliberately not abstracted. Probing both drivers showed they already agree: a Go bool binds to SQLite INTEGER and PostgreSQL BOOLEAN, an INTEGER scans into *bool, and TEXT/JSON/JSONB all scan into []byte. The boolToSQLite helpers and int-vs-bool scan split in the existing stores were accidental divergence, not a dialect requirement. DDL type tokens expand to the column types the hand-written stores already used, so CREATE TABLE IF NOT EXISTS stays a no-op against existing databases and fresh ones get a byte-identical schema. sqlxtest.Run executes a suite against every available dialect, which is what lets one store test cover both backends. Today 18 of 22 PostgreSQL store implementations have no test that touches a database; the four that exist only assert generated SQL strings. Conformance suite verified against SQLite and a live PostgreSQL 18. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
First domain onto sqlx. store_sqlite.go and store_postgresql.go were 96% identical once placeholders and type names were normalized; they become one store_sql.go. Adds ResolveSQLBackend, the two-way dispatch (SQL, Mongo) the remaining domains will use in place of ResolveBackend's three-way split. tagging had no store test at all. It now has one, and sqlxtest runs it against SQLite and PostgreSQL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tore Both pairs were mechanically identical apart from placeholders, the no-rows sentinel and the Exec result shape. scanStoredFile and scanStoredResponseRow lose their notFound parameter: with sqlx there is one sentinel to compare. filestore's test carried its own four-backend harness keyed on TEST_DATABASE_DSN and MONGO_TEST_DSN, where the postgres case had no CI path. It now shares one behavioural suite across memory, both SQL dialects and Mongo, and covers the not-found and validation paths it previously skipped. responsestore's suite moves onto sqlxtest: 7 PostgreSQL subtests now run where none did before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both stores now run on sqlx. Their tests move onto sqlxtest and gain the coverage the SQLite-only suites never had: pagination and unknown-cursor handling for batch, ordering and delete semantics for pricing overrides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
These two carried the patterns the earlier domains did not: nullable columns, boolean columns, and ALTER TABLE migrations. All three turned out to be portable once probed against both drivers. Nullable columns scanned into sql.NullString on SQLite and *string on PostgreSQL. Both drivers handle *string identically, so one scan function serves both. Boolean columns were bound through a boolToSQLite int helper on SQLite and as a Go bool on PostgreSQL. Both drivers bind bool directly. DDL defaults are spelled TRUE/FALSE, which PostgreSQL requires on BOOLEAN and SQLite accepts against its INTEGER column. ADD COLUMN migrations used IF NOT EXISTS on PostgreSQL, which SQLite has no syntax for, so the SQLite half pattern-matched the error text instead. Both engines reject a duplicate add with a recognisable message, so sqlx.AddColumns tolerates it once for both. The two duplicate-column matchers disagreed: guardrails required "column" in the message, authkeys accepted any "already exists". Kept the stricter form so an unrelated already-exists failure cannot pass for an applied migration. guardrails' local nullableString/nullableStringValue duplicated sqlutil and are gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… from one store Three config-row stores with the same List/Get/Upsert/Delete shape. mcpgateway's display_name migration read PRAGMA table_info to decide whether to add the column, which only works on SQLite; it becomes an AddColumns entry plus the backfill UPDATE, and the migration test now runs on both dialects from the pre-display_name table shape. virtualmodels' UpsertAll atomicity test needed two independent databases in one run, so it splits into a commit case and a rollback case — each gets its own database from sqlxtest, which reads better than the two-stores-in-one-test form it replaces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e store These two are where the dialects genuinely diverge, so the split is kept where it is real and removed everywhere else. conversationstore's MergeMetadata, AppendItems and DeleteItem must mutate JSON server-side in a single UPDATE or two concurrent Responses turns overwrite each other's exchange. SQLite expresses that with JSON1 functions, PostgreSQL with jsonb operators, and there is no portable spelling. The statements now live side by side behind a jsonMutations interface while the surrounding logic — rows-affected interpretation, duplicate detection, re-read — is shared. Their tests run on both engines for the first time. failover's legacy rename is likewise irreducible: PostgreSQL renames columns in place, SQLite copies rows into a reshaped table (which is also where padded primary keys get trimmed). Both are kept verbatim behind a dialect switch rather than rewritten, since they run against databases in the field. The migration test now covers both engines. The PostgreSQL migration had no test at all before this — the survey found the two halves had drifted into independently written trim logic with only one of them verified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both stores carry a config-vs-manual precedence upsert and a config-replace transaction; those are now written once. Their pre-scope and add-column migrations unify too — only the column introspection differs per engine (PRAGMA versus information_schema), so the rebuild statements are shared and just the lookup switches. Each file also held the same prepared-statement upsert twice, once inline and once in a helper; that collapses to one. budget's SumUsageCost stays dialect-specific and is marked as such: SQLite keeps the usage timestamp as text and must convert it, while PostgreSQL compares a real timestamp column and has to quote "usage" as a reserved word. The rate-limit pre-scope migration now runs on PostgreSQL in tests for the first time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
database/sql's BeginTx can only start SQLite's default deferred transaction. Every InTx caller here reads before it writes — a MAX, or an existing row — and under a deferred transaction two of them can both read and then collide when the second tries to upgrade, surfacing as SQLITE_BUSY rather than one waiting. InTx now runs BEGIN IMMEDIATE on a dedicated connection, which is what the workflows store already did by hand. Writing the conformance test for this disproved the stronger claim it started with. PostgreSQL's default READ COMMITTED does not serialize read-then-allocate at all: both transactions read the same MAX and the second fails on the unique index. That difference is pre-existing — the two workflow stores already behave this way — so the suite asserts atomicity, which does hold on both, and the isolation difference is documented on InTx rather than hidden. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Create and EnsureManagedDefaultGlobal each carried their own copy of the next-version lookup, the deactivate-previous update and the thirteen-column insert — four copies across the two backends. They now share nextVersionFor, activeGlobalVersion and insertVersion, and the SQLite half no longer hand-rolls BEGIN IMMEDIATE now that InTx does it. The store had two migration tests, both SQLite-only and both asserting only that the constructor returned non-nil. They now run on both engines and assert the behaviour that matters: version allocation, activation retiring the previous version, the managed default being idempotent, and an operator-authored version never being overwritten. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every persisted subsystem shipped two constructors: New, which called storage.New(ctx, cfg.Storage.BackendConfig()) itself, and NewWithSharedStorage, which reused a handle. Both resolved the same configuration, so the first was never opening a different database — only another connection to the same one. app.New picked between them fifteen times with an identical nil check, and nineteen Result types carried a Storage field plus close plumbing purely to record which branch had run. When audit logging and usage tracking were both disabled no handle was claimed, and the fallback path opened a separate connection per subsystem — up to fifteen SQLite handles or pgx pools against one database. app.New now opens storage once, before any subsystem, and passes it to all of them. Shutdown's fifteen copies of the same six-line close block become an ordered list, with the shared connection closed last, after the subsystems that flush through it. Order was already load-bearing and is still spelled out explicitly. 194 lines to 102. closerOf handles the two nils that reach it: a subsystem that never initialized is a typed-nil *Result whose method value would panic, and an unset storage.Storage is a nil interface that reflect reports as invalid rather than as a nil pointer. An existing shutdown test caught the second case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two hand-written workflow stores disagreed on how to store one column: SQLite kept unix seconds in an INTEGER, PostgreSQL kept a TIMESTAMPTZ and bound a time.Time. Unifying them onto the SQLite representation — which every other table in this schema already uses on both engines — left existing PostgreSQL deployments with a column the shared scan cannot read. A smoke test against a real PostgreSQL database that already had the table caught it: startup failed with "cannot scan timestamptz into *int64". The unit suite could not, because sqlxtest builds each case a fresh schema and so never meets a table created by an earlier release. NewSQLStore now converts the column in place via EXTRACT(EPOCH FROM ...), which preserves the instant, and a test covers the conversion from the legacy table shape. workflow_versions was the only table affected; every other PostgreSQL table already stored BIGINT unix seconds. Adds the first end-to-end app.New/Shutdown test. Nothing covered that path before, which is how the nil-interface bug in the new shutdown ordering reached a running binary rather than a failing test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Extends the survey with what was executed, the three things it got wrong and how each surfaced, and the four items deliberately not done with reasons. The most useful finding for future work: a conformance suite over fresh schemas proves the dialects agree but says nothing about upgrading a database written by an older release. That gap let a PostgreSQL schema break through to a running binary, and migration tests now start from the legacy table shape. Deletes possible-refactoring.md: four of its ten items are stale (they name dashboard JS replaced by the Svelte app, and failover helpers that no longer exist) and the four still-live ones are carried into the survey. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TimeFromUnix and StringFromNullable took sql.NullInt64 and sql.NullString. Both drivers scan nullable columns into *int64 and *string identically, so the unified stores use those directly and nothing calls these any more. deadcode is back to its four documented deliberate keeps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The audit write path is the highest-traffic store in the gateway and had two
implementations. Unifying it needed two more portable pieces:
- {serial_pk}, since the attempts table declares an auto-assigned key as
INTEGER PRIMARY KEY AUTOINCREMENT on SQLite and BIGSERIAL on PostgreSQL.
- Dialect.TimestampArg. The engines genuinely disagree here: PostgreSQL binds
a time.Time to TIMESTAMPTZ, while SQLite has no date type and these columns
hold RFC3339 text, which is what the readers parse back. The store keeps
writing text on SQLite rather than letting the driver pick.
INSERT OR IGNORE becomes ON CONFLICT DO NOTHING, which both engines accept, and
the two bool-to-int conversions go away. The functional indexes over JSON paths
stay dialect-specific — json_extract versus #>> — and are built rather than
tokenised.
Replaces the two PostgreSQL tests that only asserted a generated SQL string
with tests that execute on both engines: round trip, duplicate-id tolerance,
chunking past the 999-parameter limit, the retention sweep, and the legacy
`model` column rename.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The store suites are dialect-parameterised, but without GOMODEL_TEST_POSTGRES_URL every PostgreSQL subtest skips — CI would stay green while covering only SQLite, which is the situation this refactor set out to fix. Adds a postgres:18-alpine service to the go-tests job so all 91 PostgreSQL subtests execute. Also corrects the survey's outcome numbers now that auditlog is migrated, and records why the usage store is not: its RecalculatePricing is built from the reader's condition builders, and the two implementations differ semantically (SQLite paginates by id; PostgreSQL takes every row with FOR UPDATE), so it should follow the reader rather than lead it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Too many files changed for review. ( Bypass the limit by tagging |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe refactor introduces a shared SQL abstraction for SQLite and PostgreSQL, migrates domain stores to unified ChangesUnified SQL storage refactor
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
internal/app/app.go (1)
499-511: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead nil check:
usageReadStoragecan no longer be nil.
usageReadStorage := sharedStorageis always non-nil now (the earlierstorage.Newerror path already returns viafail), soif usageReadStorage != nilat Line 502 is now vestigial. Same for theif usageStorage != nilcheck insideinitAdminat Line 1004/1017 forauditStorage/usageReadStorage.🤖 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 `@internal/app/app.go` around lines 499 - 511, Remove the vestigial nil guard around usage.NewReader in the usage reader initialization block, since sharedStorage has already been validated and returned on failure. Also remove the corresponding usageStorage nil check in initAdmin for auditStorage/usageReadStorage, while preserving the existing reader error handling and initialization behavior.
🤖 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 @.github/workflows/test.yml:
- Around line 53-54: Update the postgres service image in the workflow to use a
specific sha256 digest instead of the mutable postgres:18-alpine tag, while
preserving the PostgreSQL 18 Alpine image selection.
In `@internal/auditlog/factory.go`:
- Around line 42-48: Update auditlog.New to validate cfg is non-nil before
accessing cfg.Logging.Enabled, returning a clean error consistent with the
sibling factory constructors. Preserve the existing disabled-logging and
nil-storage behavior after this guard.
In `@internal/authkeys/store_sql.go`:
- Around line 64-71: The comment above the sqlx.AddColumns and db.Schema calls
incorrectly claims idx_auth_keys_enabled depends on a migrated column. Remove or
revise that rationale to accurately describe the ordering without asserting
migration dependence, while leaving the migration and index execution unchanged.
In `@internal/failover/store_sql_migrate.go`:
- Around line 39-44: Update the SQLite migration flow before its legacy-shape
early return so it unconditionally trims existing primary_model keys whenever
the primary_model column is present. Keep the existing
source/targets/description check and copy behavior unchanged, ensuring the
trim-only case is handled like migratePostgresRuleTable.
In `@internal/filestore/store_test.go`:
- Around line 42-68: Update newMongoTestStore to bound the sanitized t.Name()
component before constructing the MongoDB database name, preserving the prefix
and timestamp while ensuring the final UTF-8 byte length stays within MongoDB’s
64-byte limit. Reuse the existing identifier-sanitization/truncation approach
used by sqlxtest.sanitizeIdentifier rather than introducing unrelated changes.
In `@internal/storage/sqlx/dialect.go`:
- Around line 56-65: Update the PostgreSQL type mapping in the dialect
configuration so TypeJSONText resolves to TEXT rather than JSONB. Keep TypeJSON
mapped to JSONB and leave the other mappings unchanged, ensuring fallback_models
created through internal failover table setup matches legacy schema types.
In `@internal/storage/sqlx/rebind_test.go`:
- Around line 72-76: Update the test case describing `SELECT $1, ?` so it does
not present the mixed-placeholder collision as expected valid behavior. Rename
the case to explicitly identify mixed placeholder styles and adjust its
expectation or coverage to match the `rebind` contract; document in `rebind`
that queries must use a single placeholder style if that is the intended
behavior.
- Around line 99-115: Update the PostgreSQL expectation in the rebind test’s
dialect table to use a valid boolean default, matching the store’s FALSE/TRUE
representation, while leaving the SQLite expectation and other type mappings
unchanged.
In `@internal/storage/sqlx/sqlx.go`:
- Around line 40-44: Reuse the canonical Row interface from
internal/storage/sqlx/sqlx.go instead of redeclaring the scanner interface.
Update scanSQLCredential in internal/providers/credentials_store_sql.go (lines
164-166), scanSQLRule in internal/ratelimit/store_sql.go (lines 176-197), and
scanSQLVirtualModel in internal/virtualmodels/store_sql.go (lines 169-197) to
accept sqlx.Row, adding or preserving the required sqlx imports.
In `@internal/workflows/store_sql_test.go`:
- Around line 143-145: The second Deactivate assertion should support wrapped
sentinel errors. Add the errors package to the test imports and replace the
direct ErrNotFound comparison in the Deactivate test with errors.Is while
preserving the existing failure message.
In `@internal/workflows/store_sql.go`:
- Around line 22-45: Split the combined sqlSchema declaration into separate
table and index definitions, such as sqlTable and sqlIndexes, following the
pattern used by the sibling authkeys store. Update NewSQLStore to apply the
table definition from the table symbol and iterate over the index collection
independently, removing its reliance on sqlSchema[0] and sqlSchema[1:].
---
Outside diff comments:
In `@internal/app/app.go`:
- Around line 499-511: Remove the vestigial nil guard around usage.NewReader in
the usage reader initialization block, since sharedStorage has already been
validated and returned on failure. Also remove the corresponding usageStorage
nil check in initAdmin for auditStorage/usageReadStorage, while preserving the
existing reader error handling and initialization behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 499ef38c-fb33-4b89-9487-647c238591a0
📒 Files selected for processing (131)
.github/workflows/test.ymldocs/dev/2026-07-25_backend-refactor-survey.mddocs/dev/possible-refactoring.mdinternal/app/app.gointernal/app/lifecycle_test.gointernal/auditlog/auditlog_test.gointernal/auditlog/factory.gointernal/auditlog/reader_sqlite_boundary_test.gointernal/auditlog/stats_test.gointernal/auditlog/store_postgresql.gointernal/auditlog/store_postgresql_test.gointernal/auditlog/store_sql.gointernal/auditlog/store_sql_test.gointernal/auditlog/store_sqlite.gointernal/auditlog/store_sqlite_test.gointernal/authkeys/factory.gointernal/authkeys/store_postgresql.gointernal/authkeys/store_sql.gointernal/authkeys/store_sql_test.gointernal/authkeys/store_sqlite.gointernal/authkeys/store_sqlite_test.gointernal/batch/factory.gointernal/batch/store_postgresql.gointernal/batch/store_sql.gointernal/batch/store_sql_test.gointernal/batch/store_sqlite.gointernal/batch/store_sqlite_test.gointernal/batch/store_test.gointernal/budget/factory.gointernal/budget/store_postgresql.gointernal/budget/store_sql.gointernal/budget/store_sql_test.gointernal/budget/store_sqlite.gointernal/budget/store_sqlite_test.gointernal/conversationstore/factory.gointernal/conversationstore/store_persistent.gointernal/conversationstore/store_postgresql.gointernal/conversationstore/store_sql.gointernal/conversationstore/store_sql_json.gointernal/conversationstore/store_sql_test.gointernal/conversationstore/store_sqlite.gointernal/conversationstore/store_sqlite_test.gointernal/core/responses_field_parity_test.gointernal/failover/factory.gointernal/failover/store_postgresql.gointernal/failover/store_sql.gointernal/failover/store_sql_migrate.gointernal/failover/store_sql_test.gointernal/failover/store_sqlite.gointernal/failover/store_sqlite_test.gointernal/filestore/factory.gointernal/filestore/store.gointernal/filestore/store_postgresql.gointernal/filestore/store_sql.gointernal/filestore/store_sqlite.gointernal/filestore/store_test.gointernal/guardrails/factory.gointernal/guardrails/store.gointernal/guardrails/store_postgresql.gointernal/guardrails/store_sql.gointernal/guardrails/store_sql_test.gointernal/guardrails/store_sqlite.gointernal/guardrails/store_sqlite_test.gointernal/mcpgateway/factory.gointernal/mcpgateway/store_postgresql.gointernal/mcpgateway/store_sql.gointernal/mcpgateway/store_sql_test.gointernal/mcpgateway/store_sqlite.gointernal/mcpgateway/store_sqlite_test.gointernal/modeldata/merge.gointernal/pricingoverrides/factory.gointernal/pricingoverrides/store_postgresql.gointernal/pricingoverrides/store_sql.gointernal/pricingoverrides/store_sql_test.gointernal/pricingoverrides/store_sqlite.gointernal/pricingoverrides/store_sqlite_test.gointernal/providers/bedrockmantle/auth.gointernal/providers/credentials_store_postgresql.gointernal/providers/credentials_store_sql.gointernal/providers/credentials_store_sql_test.gointernal/providers/credentials_store_sqlite_test.gointernal/providers/credentials_wire.gointernal/ratelimit/factory.gointernal/ratelimit/store_postgresql.gointernal/ratelimit/store_sql.gointernal/ratelimit/store_sql_migrate.gointernal/ratelimit/store_sql_test.gointernal/ratelimit/store_sqlite.gointernal/ratelimit/store_sqlite_test.gointernal/responsestore/factory.gointernal/responsestore/store_persistent.gointernal/responsestore/store_postgresql.gointernal/responsestore/store_sql.gointernal/responsestore/store_sql_test.gointernal/responsestore/store_sqlite_test.gointernal/storage/sqlbackend.gointernal/storage/sqlutil/sqlutil.gointernal/storage/sqlx/conformance_test.gointernal/storage/sqlx/dialect.gointernal/storage/sqlx/postgres.gointernal/storage/sqlx/rebind.gointernal/storage/sqlx/rebind_test.gointernal/storage/sqlx/schema.gointernal/storage/sqlx/sqlite.gointernal/storage/sqlx/sqlx.gointernal/storage/sqlx/sqlxtest/sqlxtest.gointernal/tagging/factory.gointernal/tagging/store_postgresql.gointernal/tagging/store_sql.gointernal/tagging/store_sql_test.gointernal/usage/factory.gointernal/virtualmodels/balancer_test.gointernal/virtualmodels/config_overlay_test.gointernal/virtualmodels/factory.gointernal/virtualmodels/helpers_test.gointernal/virtualmodels/seed_test.gointernal/virtualmodels/service_test.gointernal/virtualmodels/store_postgresql.gointernal/virtualmodels/store_sql.gointernal/virtualmodels/store_sqlite.gointernal/virtualmodels/store_test.gointernal/workflows/factory.gointernal/workflows/store_postgresql.gointernal/workflows/store_sql.gointernal/workflows/store_sql_migrate.gointernal/workflows/store_sql_test.gointernal/workflows/store_sqlite.gointernal/workflows/store_sqlite_test.gotests/e2e/budget_test.gotests/e2e/failover_test.gotests/e2e/ratelimit_test.go
💤 Files with no reviewable changes (43)
- internal/failover/store_postgresql.go
- docs/dev/possible-refactoring.md
- internal/budget/store_sqlite_test.go
- internal/responsestore/store_postgresql.go
- internal/pricingoverrides/store_sqlite_test.go
- internal/batch/store_sqlite_test.go
- internal/virtualmodels/store_sqlite.go
- internal/guardrails/store_sqlite.go
- internal/auditlog/store_sqlite.go
- internal/failover/store_sqlite_test.go
- internal/guardrails/store_sqlite_test.go
- internal/authkeys/store_sqlite_test.go
- internal/auditlog/store_postgresql_test.go
- internal/authkeys/store_sqlite.go
- internal/tagging/store_postgresql.go
- internal/pricingoverrides/store_sqlite.go
- internal/providers/credentials_store_postgresql.go
- internal/conversationstore/store_sqlite_test.go
- internal/filestore/store_sqlite.go
- internal/mcpgateway/store_sqlite.go
- internal/conversationstore/store_postgresql.go
- internal/mcpgateway/store_postgresql.go
- internal/virtualmodels/store_postgresql.go
- internal/batch/store_postgresql.go
- internal/pricingoverrides/store_postgresql.go
- internal/budget/store_postgresql.go
- internal/batch/store_sqlite.go
- internal/conversationstore/store_sqlite.go
- internal/ratelimit/store_sqlite.go
- internal/failover/store_sqlite.go
- internal/filestore/store_postgresql.go
- internal/mcpgateway/store_sqlite_test.go
- internal/ratelimit/store_postgresql.go
- internal/budget/store_sqlite.go
- internal/providers/credentials_store_sqlite_test.go
- internal/auditlog/store_postgresql.go
- internal/responsestore/store_sqlite_test.go
- internal/guardrails/store_postgresql.go
- internal/storage/sqlutil/sqlutil.go
- internal/ratelimit/store_sqlite_test.go
- internal/authkeys/store_postgresql.go
- internal/guardrails/store.go
- internal/workflows/store_postgresql.go
Comment on lines
+53
to
+54
| postgres: | ||
| image: postgres:18-alpine |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
Consider pinning the PostgreSQL image to a digest.
Static analysis flags the postgres:18-alpine tag as mutable/unpinned. For a CI-only service this is low risk, but pinning to a sha256: digest is cheap supply-chain hardening if you want to close the finding.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 54-54: unpinned image references (unpinned-images): container image is not pinned to a SHA256 hash
(unpinned-images)
🤖 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 @.github/workflows/test.yml around lines 53 - 54, Update the postgres service
image in the workflow to use a specific sha256 digest instead of the mutable
postgres:18-alpine tag, while preserving the PostgreSQL 18 Alpine image
selection.
Source: Linters/SAST tools
Comment on lines
+39
to
+44
| if len(columns) == 0 { | ||
| return nil | ||
| } | ||
| if !columns["source"] && !columns["targets"] && !columns["description"] { | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
SQLite skips the trim-only case that PostgreSQL handles. The early return fires whenever none of source/targets/description remain, so a SQLite database already renamed by an earlier non-trimming version never gets its padded primary_model keys trimmed. migratePostgresRuleTable deliberately trims independently of the rename (see the comment at Lines 132-134) precisely to keep such rules reachable from the trim-normalizing Get/Delete lookups; the SQLite path leaves them orphaned.
Consider running an unconditional trim on SQLite when primary_model exists, before the legacy-shape check:
🔧 Sketch
if len(columns) == 0 {
return nil
}
+ if columns["primary_model"] {
+ if _, err := db.Exec(ctx, `UPDATE failover_rules SET primary_model = TRIM(primary_model) WHERE primary_model <> TRIM(primary_model)`); err != nil {
+ return fmt.Errorf("trim failover_rules primary_model: %w", err)
+ }
+ }
if !columns["source"] && !columns["targets"] && !columns["description"] {
return nil
}Note the INSERT OR REPLACE in the copy path can collide once keys are trimmed; that already applies to the existing copy and stays unchanged.
📝 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.
Suggested change
| if len(columns) == 0 { | |
| return nil | |
| } | |
| if !columns["source"] && !columns["targets"] && !columns["description"] { | |
| return nil | |
| } | |
| if len(columns) == 0 { | |
| return nil | |
| } | |
| if columns["primary_model"] { | |
| if _, err := db.Exec(ctx, `UPDATE failover_rules SET primary_model = TRIM(primary_model) WHERE primary_model <> TRIM(primary_model)`); err != nil { | |
| return fmt.Errorf("trim failover_rules primary_model: %w", err) | |
| } | |
| } | |
| if !columns["source"] && !columns["targets"] && !columns["description"] { | |
| return nil | |
| } |
🤖 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 `@internal/failover/store_sql_migrate.go` around lines 39 - 44, Update the
SQLite migration flow before its legacy-shape early return so it unconditionally
trims existing primary_model keys whenever the primary_model column is present.
Keep the existing source/targets/description check and copy behavior unchanged,
ensuring the trim-only case is handled like migratePostgresRuleTable.
Comment on lines
+72
to
+76
| { | ||
| name: "existing positional parameter is not mistaken for a dollar quote", | ||
| query: `SELECT $1, ?`, | ||
| want: `SELECT $1, $1`, | ||
| }, |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Mixed placeholder styles silently collide.
SELECT $1, ? → SELECT $1, $1 binds the pre-existing positional parameter and the rewritten ? to the same argument slot. Pinning this as expected behaviour hides a corruption mode; consider documenting in rebind that queries must not mix styles (or naming the case accordingly).
🤖 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 `@internal/storage/sqlx/rebind_test.go` around lines 72 - 76, Update the test
case describing `SELECT $1, ?` so it does not present the mixed-placeholder
collision as expected valid behavior. Rename the case to explicitly identify
mixed placeholder styles and adjust its expectation or coverage to match the
`rebind` contract; document in `rebind` that queries must use a single
placeholder style if that is the intended behavior.
Comment on lines
+99
to
+115
| const ddl = `CREATE TABLE t ( | ||
| id TEXT PRIMARY KEY, | ||
| n ` + TypeInt64 + ` NOT NULL, | ||
| flag ` + TypeBool + ` NOT NULL DEFAULT 1, | ||
| cost ` + TypeFloat + `, | ||
| doc ` + TypeJSONText + ` NOT NULL DEFAULT '[]', | ||
| blob ` + TypeJSON + `, | ||
| at ` + TypeTimestamp + ` | ||
| )` | ||
|
|
||
| tests := []struct { | ||
| dialect Dialect | ||
| want []string | ||
| }{ | ||
| {SQLite, []string{"INTEGER NOT NULL", "INTEGER NOT NULL DEFAULT 1", "REAL", "TEXT NOT NULL", "JSON", "DATETIME"}}, | ||
| {PostgreSQL, []string{"BIGINT NOT NULL", "BOOLEAN NOT NULL DEFAULT 1", "DOUBLE PRECISION", "JSONB NOT NULL", "JSONB", "TIMESTAMPTZ"}}, | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
DEFAULT 1 on a {bool} column is invalid PostgreSQL.
The fixture never reaches a database so the test passes, but it pins BOOLEAN NOT NULL DEFAULT 1, which PostgreSQL rejects. Every real store writes DEFAULT FALSE/TRUE; use the same here so the fixture doesn't advertise a portable-looking pattern that isn't.
♻️ Proposed fixture fix
- flag ` + TypeBool + ` NOT NULL DEFAULT 1,
+ flag ` + TypeBool + ` NOT NULL DEFAULT FALSE,- {SQLite, []string{"INTEGER NOT NULL", "INTEGER NOT NULL DEFAULT 1", "REAL", "TEXT NOT NULL", "JSON", "DATETIME"}},
- {PostgreSQL, []string{"BIGINT NOT NULL", "BOOLEAN NOT NULL DEFAULT 1", "DOUBLE PRECISION", "JSONB NOT NULL", "JSONB", "TIMESTAMPTZ"}},
+ {SQLite, []string{"INTEGER NOT NULL", "INTEGER NOT NULL DEFAULT FALSE", "REAL", "TEXT NOT NULL", "JSON", "DATETIME"}},
+ {PostgreSQL, []string{"BIGINT NOT NULL", "BOOLEAN NOT NULL DEFAULT FALSE", "DOUBLE PRECISION", "JSONB NOT NULL", "JSONB", "TIMESTAMPTZ"}},📝 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.
Suggested change
| const ddl = `CREATE TABLE t ( | |
| id TEXT PRIMARY KEY, | |
| n ` + TypeInt64 + ` NOT NULL, | |
| flag ` + TypeBool + ` NOT NULL DEFAULT 1, | |
| cost ` + TypeFloat + `, | |
| doc ` + TypeJSONText + ` NOT NULL DEFAULT '[]', | |
| blob ` + TypeJSON + `, | |
| at ` + TypeTimestamp + ` | |
| )` | |
| tests := []struct { | |
| dialect Dialect | |
| want []string | |
| }{ | |
| {SQLite, []string{"INTEGER NOT NULL", "INTEGER NOT NULL DEFAULT 1", "REAL", "TEXT NOT NULL", "JSON", "DATETIME"}}, | |
| {PostgreSQL, []string{"BIGINT NOT NULL", "BOOLEAN NOT NULL DEFAULT 1", "DOUBLE PRECISION", "JSONB NOT NULL", "JSONB", "TIMESTAMPTZ"}}, | |
| } | |
| const ddl = `CREATE TABLE t ( | |
| id TEXT PRIMARY KEY, | |
| n ` + TypeInt64 + ` NOT NULL, | |
| flag ` + TypeBool + ` NOT NULL DEFAULT FALSE, | |
| cost ` + TypeFloat + `, | |
| doc ` + TypeJSONText + ` NOT NULL DEFAULT '[]', | |
| blob ` + TypeJSON + `, | |
| at ` + TypeTimestamp + ` | |
| )` | |
| tests := []struct { | |
| dialect Dialect | |
| want []string | |
| }{ | |
| {SQLite, []string{"INTEGER NOT NULL", "INTEGER NOT NULL DEFAULT FALSE", "REAL", "TEXT NOT NULL", "JSON", "DATETIME"}}, | |
| {PostgreSQL, []string{"BIGINT NOT NULL", "BOOLEAN NOT NULL DEFAULT FALSE", "DOUBLE PRECISION", "JSONB NOT NULL", "JSONB", "TIMESTAMPTZ"}}, | |
| } |
🤖 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 `@internal/storage/sqlx/rebind_test.go` around lines 99 - 115, Update the
PostgreSQL expectation in the rebind test’s dialect table to use a valid boolean
default, matching the store’s FALSE/TRUE representation, while leaving the
SQLite expectation and other type mappings unchanged.
…t helper Twelve packages define a runSQLStoreTest wrapper. Eight carried the same sentence restating the function name, three already carried none, and the dialect fan-out it described is documented where it actually happens, on sqlxtest.Run. Removing them makes the helper consistent across packages and leaves a comment only where there is something to say — conversationstore notes why its cases matter most on PostgreSQL, auditlog notes what its tests replaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eight of eleven CodeRabbit findings; the other three are answered on the PR.
- auditlog.New dereferenced cfg with no guard, unlike every sibling factory
touched here. Added the nil check.
- The authkeys index-ordering comment claimed idx_auth_keys_enabled needed a
migrated column; `enabled` is in the base CREATE TABLE, so the rationale was
simply wrong. Rewritten to say what the ordering actually buys.
- Six stores redeclared the same anonymous `interface{ Scan(dest ...any) error }`
scanner. They now take sqlx.Row, which is that interface.
- budget, guardrails and workflows selected DDL by slice position
(sqlSchema[0], sqlSchema[1:]) to run migrations between the table and its
indexes, so reordering the literal would silently break the sequence. Split
into named table and index variables.
- filestore's Mongo tests built database names up to 91 bytes; MongoDB rejects
anything at or above 64, so they would have failed the moment MONGO_TEST_DSN
was set. The name is now bounded.
- rebind now documents that a query must use one placeholder style, and the
mixed-style test case is named as the unsupported input it is rather than
reading like a supported one.
- A test fixture pinned `BOOLEAN NOT NULL DEFAULT 1`, which PostgreSQL rejects,
while every real store correctly writes TRUE/FALSE.
- Replaced an `err != ErrNotFound` sentinel comparison with errors.Is.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Thanks — eight of these were right and are fixed in dbf52af. Three I'm declining, with reasoning:
Adding an unconditional trim on SQLite would also not be free: if two rows differ only by padding it collides on the primary key at startup. I kept both paths verbatim on purpose — they run against databases in the field where a subtle change would silently orphan rules.
The fresh-vs-migrated column type difference you describe predates this PR (same
|
Contributor
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)
internal/filestore/store_test.go (1)
129-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the missing-record checks table-driven.
These tests repeat the same backend harness and assertion shape. Represent
GetandDeleteas table cases so future not-found coverage remains consistent.As per coding guidelines, changed behavior tests should use table-driven coverage.
🤖 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 `@internal/filestore/store_test.go` around lines 129 - 145, Consolidate TestStoreGetMissingReturnsNotFound and TestStoreDeleteMissingReturnsNotFound into one table-driven test using cases that identify the operation and execute it against the shared runStoreSuite harness. Preserve the existing absent-key input and ErrNotFound assertion for both Get and Delete cases.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 `@internal/filestore/store_test.go`:
- Around line 70-81: Update mongoTestDatabaseName to replace all
MongoDB-forbidden characters (\, ., ", $, and null bytes, along with /) before
applying the length limit, and truncate sanitized names by rune boundaries
rather than bytes. Add table-driven coverage for long Unicode names and inputs
containing each forbidden character, preserving the existing prefix, suffix, and
63-byte maximum.
---
Outside diff comments:
In `@internal/filestore/store_test.go`:
- Around line 129-145: Consolidate TestStoreGetMissingReturnsNotFound and
TestStoreDeleteMissingReturnsNotFound into one table-driven test using cases
that identify the operation and execute it against the shared runStoreSuite
harness. Preserve the existing absent-key input and ErrNotFound assertion for
both Get and Delete cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1802fb0e-a570-4a82-bf32-830713d8e834
📒 Files selected for processing (14)
internal/auditlog/factory.gointernal/authkeys/store_sql.gointernal/budget/store_sql.gointernal/failover/store_sql.gointernal/filestore/store_test.gointernal/guardrails/store_sql.gointernal/mcpgateway/store_sql.gointernal/providers/credentials_store_sql.gointernal/ratelimit/store_sql.gointernal/storage/sqlx/rebind.gointernal/storage/sqlx/rebind_test.gointernal/virtualmodels/store_sql.gointernal/workflows/store_sql.gointernal/workflows/store_sql_test.go
Comment on lines
+70
to
+81
| // mongoTestDatabaseName builds a unique database name that stays inside | ||
| // MongoDB's 64-byte limit. | ||
| func mongoTestDatabaseName(testName string) string { | ||
| const prefix = "gomodel_filestore_test_" | ||
| suffix := "_" + time.Now().Format("20060102150405_000000000") | ||
|
|
||
| sanitized := strings.ReplaceAll(testName, "/", "_") | ||
| if budget := 63 - len(prefix) - len(suffix); len(sanitized) > budget { | ||
| sanitized = sanitized[:budget] | ||
| } | ||
| return prefix + sanitized + suffix | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)internal/filestore/(store_test\.go|store\.go|.*_test\.go)$|(^|/)go\.mod$|(^|/)go\.sum$'
echo "== testfile excerpt =="
if [ -f internal/filestore/store_test.go ]; then
nl -ba internal/filestore/store_test.go | sed -n '1,130p'
fi
echo "== relevant searches =="
rg -n "mongoTestDatabaseName|database|CreateDatabase|Database|Mongo|mongodb" internal go.mod 2>/dev/null || true
echo "== go.mod deps excerpt =="
if [ -f go.mod ]; then
sed -n '1,220p' go.mod | rg -n "module |require |mongodb|mongo|driver|v16|v5" -C 2 || true
fiRepository: ENTERPILOT/GoModel
Length of output: 370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== testfile excerpt =="
cat -n internal/filestore/store_test.go | sed -n '1,140p'
echo "== store excerpt around database creation =="
cat -n internal/filestore/store.go | sed -n '1,260p'
echo "== relevant searches =="
rg -n "mongoTestDatabaseName|mongo|Mongo|Database|Drop|Create" internal/filestore internal go.mod internal/filestore/store.go 2>/dev/null || true
echo "== go.mod deps excerpt =="
if [ -f go.mod ]; then
sed -n '1,220p' go.mod | rg -n "module |require |mongodb|mongo|driver|v16|v5" -C 2 || true
fi
echo "== read-only behavioral probe for current implementation =="
python3 - <<'PY'
import string
def current(test_name: str) -> str:
suffix = "_" + "20060102150405_000000000"
sanitized = test_name.replace("/", "_")
budget = 63 - len("gomodel_filestore_test_") - len(suffix)
if len(sanitized) > budget:
sanitized = sanitized[:budget]
return "gomodel_filestore_test_" + sanitized + suffix
cases = {
"slash": "a/b",
"forbidden_chars": r'ab\c.d"e$f',
"unicode_beyond_63": "x" * 63 + "\U0001F4AA",
"unicode_long": ("a/é" * 80),
}
for name, value in cases.items():
out = current(value)
print(f"{name}: len(value)={len(value)} value={value!r}")
print(f" out={out!r} len={len(out)} ok_63={len(out) <= 63}")
print(f" has_forbidden={bool(set('\\.$\"*>|?:') & set(out))}")
PYRepository: ENTERPILOT/GoModel
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== testfile excerpt =="
sed -n '1,140p' internal/filestore/store_test.go
echo "== store excerpt =="
sed -n '1,260p' internal/filestore/store.go
echo "== targeted searches from repository root =="
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' 'mongoTestDatabaseName|mongo|Mongo|Database|Drop|Create|client|mongo' . \
| grep -v internal/admin/dashboard/static/dist/assets/index-B4NYOd0m.js \
| head -n 200
echo "== go.mod deps excerpt =="
sed -n '1,220p' go.mod | rg -n 'module |require |mongodb|mongo|driver|v16|v5' -C 2 || true
echo "== read-only behavioral probe for current implementation =="
python3 - <<'PY'
import string
def current(test_name: str) -> str:
suffix = "_" + "20060102150405_000000000"
sanitized = test_name.replace("/", "_")
budget = 63 - len("gomodel_filestore_test_") - len(suffix)
if len(sanitized) > budget:
sanitized = sanitized[:budget]
return "gomodel_filestore_test_" + sanitized + suffix
cases = {
"slash": "a/b",
"forbidden_chars": r'ab\c.d"e$f',
"unicode_beyond_63": "x" * 63 + "\U0001F4AA",
"unicode_long": ("a/é" * 80),
}
for name, value in cases.items():
out = current(value)
print(f"{name}: len(value)={len(value)} value={value!r}")
print(f" out={out!r} len={len(out)} ok_63={len(out) <= 63}")
print(f" has_forbidden={bool(set('\\.$\"*>|?:') & set(out))}")
PYRepository: ENTERPILOT/GoModel
Length of output: 37734
🌐 Web query:
MongoDB reserved characters invalid in database names backslash dot dollar quote limit 64 bytes
💡 Result:
In MongoDB, database names must be less than 64 bytes [1][2][3] and cannot be empty [1][4][2]. The following restrictions apply to database names: General Restrictions: - Must be less than 64 bytes [1][4][2]. - Cannot be empty [1][4][2]. - Cannot contain the null character (\x00) [1][5]. - Cannot contain the following characters: forward slash (/), backslash (), dot (.), double quote ("), dollar sign ($), and space [1][6][7]. Platform-Specific Restrictions: - Windows: In addition to the above, database names on Windows cannot contain the following characters: * < >: |? [1][5]. - Reserved Names on Windows: On Windows systems, database names cannot be one of the reserved names: con, prn, aux, nul, com1, com2, com3, com4, com5, com6, com7, com8, com9, lpt1, lpt2, lpt3, lpt4, lpt5, lpt6, lpt7, lpt8, or lpt9 [4]. Important Considerations: - Atlas Shared Clusters: If you are using MongoDB Atlas shared clusters (M0, M2, M5), database names are further restricted to a maximum of 38 bytes [8]. - Case Sensitivity: While database names are case-sensitive, it is recommended to use consistent capitalization to avoid issues, as some filesystems are case-insensitive [1]. - Namespace Length: When naming databases, keep in mind that the full namespace (which includes the database name and collection name) is subject to its own length limits (e.g., 255 bytes for unsharded collections) [1][2].
Citations:
- 1: https://www.mongodb.com/docs/v8.2/reference/limits/
- 2: https://www.mongodb.com/docs/manual/reference/limits/
- 3: mongodb/mongo@0badd5a
- 4: https://github.com/mongodb/mongo/blob/66385e1723066f2c435c329a220c67220403d7ac/src/mongo/db/catalog/database_impl.cpp
- 5: https://www.geeksforgeeks.org/mongodb/mongodb-database-collection-and-document/
- 6: https://github.com/mongodb/mongo-python-driver/blob/ac61cf87a911d72b095ea00663e051f6ac148c7a/pymongo/database.py
- 7: https://mongodb.github.io/mongo-java-driver/5.9/apidocs/driver-core/com/mongodb/MongoNamespace.html
- 8: https://stackoverflow.com/questions/65774607/max-length-of-the-database-name-in-mongodb-4
Sanitize MongoDB database names before truncation.
mongoTestDatabaseName only replaces /, but the configured MongoDB tests use Mongo 7 and its database-name restrictions also disallow \ . " $ and null bytes. Also slice at rune boundaries when shrinking Unicode test names; otherwise a name can be truncated mid-rune. Add table-driven tests for long Unicode inputs and forbidden-character inputs.
🤖 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 `@internal/filestore/store_test.go` around lines 70 - 81, Update
mongoTestDatabaseName to replace all MongoDB-forbidden characters (\, ., ", $,
and null bytes, along with /) before applying the length limit, and truncate
sanitized names by rune boundaries rather than bytes. Add table-driven coverage
for long Unicode names and inputs containing each forbidden character,
preserving the existing prefix, suffix, and 63-byte maximum.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
The store suites build each case a fresh schema, so nothing exercised a database written by an earlier release — the blind spot that let the workflow_versions.created_at break reach a real PostgreSQL database. upgrade-compat.sh boots the baseline binary against an empty database, writes a row through every store domain, restarts it so the snapshot is a storage read rather than a cache read, then boots the working-tree binary on the same database and checks startup, reads and writes. Runs against SQLite, PostgreSQL and MongoDB. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ration Unifying workflow_versions.created_at on unix seconds converts the PostgreSQL column in place on first startup. Two consequences operators need before upgrading: the value keeps its instant but loses sub-second precision and renders in UTC, and an older binary cannot read the converted column, so rolling back means restoring a dump. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/advanced/configuration.mdx`:
- Around line 124-128: Update the upgrade guidance near the SQLite timestamp
migration to replace “no action needed” with “no manual migration is required,”
and recommend taking a database backup before startup when rollback may be
needed. Keep the existing one-way migration and restore-from-pre-upgrade-dump
details concise and user-focused.
- Around line 128-129: Update the documentation around the binary workflow
created_at conversion to state that fractional timestamps are converted to whole
Unix seconds, rather than claiming they preserve the instant. Add a migration
test covering fractional-second created_at values and verify the resulting
whole-second behavior.
In `@tests/e2e/upgrade-compat.sh`:
- Around line 60-73: Update build_binaries so the working-tree binary is always
rebuilt from the current repository state: remove the conditional existence
check around NEW_BIN and invoke make build unconditionally from REPO_ROOT. Leave
the baseline worktree setup and OLD_BIN build behavior unchanged.
- Around line 347-370: Add a post-upgrade filestore write check to the existing
write_check block, using the established filestore upload helper or command from
seed()/snapshot() and targeting the same filestore API domain. Keep the check
consistent with the other write checks so a failed upload reports its error and
verifies filestore writes after upgrade.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6e5b528d-d320-439f-94df-5e7facbaeea1
📒 Files selected for processing (4)
docs/advanced/configuration.mdxdocs/dev/2026-07-25_backend-refactor-survey.mdtests/e2e/release-e2e-scenarios.mdtests/e2e/upgrade-compat.sh
… rounding EXTRACT(EPOCH FROM created_at)::bigint rounds, so a timestamp carrying .6 seconds migrated to one second in the future — and disagreed with the truncation time.Unix performs on every row the store writes afterwards. FLOOR makes a migrated row indistinguishable from a freshly written one. The migration test now seeds fractional timestamps either side of the rounding boundary; it fails on the previous cast. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Reviewed the five findings on the last two commits. Four were right and are fixed in a06026b; one is declined. Fixed
Declined
|
SantiagoDePolonia
added a commit
that referenced
this pull request
Jul 25, 2026
Three of its deferred items were done in #587 and its MongoDB coverage numbers were stale, so the one section anyone reads was actively wrong. Removed what had a better home or no longer applied: a baseline snapshot of a commit two releases back, line-count tables git already holds, a paragraph on sqlxtest duplicating that package's own doc comment, the driver binding facts now stated on the type tokens, and a reconciliation against a file deleted in #586. 261 lines to 112, and renamed, since it stopped being a survey the moment the survey was acted on. Also fixes the dangling pointer to that deleted file in the 2026-07-04 architecture review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SantiagoDePolonia
added a commit
that referenced
this pull request
Jul 25, 2026
#587) * refactor(config): separate loading, decoding and validation of failover rules loadFailoverConfig did env expansion, file IO, strict JSON decoding, a three-way merge and validation in one body, with the decode-then-merge step written out once per source. Each source is now a named function that returns nil when unconfigured, so the loader reads as four steps. The hand-rolled JSON decoder is split into decodeStrictJSONObject, whose doc comment states what it rejects that a plain Unmarshal accepts and why that needs the token stream. Behaviour is unchanged, including that an unset FAILOVER_RULES_JSON is no rules while an empty rules *file* stays a parse error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(failover): compute a request's match keys once per resolve The resolver built the same key list twice on every request — once for the disabled check, once for the manual-rule lookup — and each build allocates a map and two slices. A requestIdentity value now carries the source model, its canonical key and the match keys, computed once. BenchmarkResolveFailovers: 330 -> 237 ns/op, 312 -> 216 B/op, 8 -> 6 allocs/op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(auditlog): serve both SQL backends from one reader reader_sqlite.go (497) and reader_postgresql.go (395) were the same reader written twice, down to a duplicated latency predicate and a scan body the SQLite half repeated inline as well as in its own helper. One SQLReader now serves both, with a readerDialect value holding the six spellings that genuinely differ — ILIKE, the id casts a pre-unification PostgreSQL database still needs, the JSON path operators, the date-range boundary and the hour-bucketing expression — each carrying why. sqlx.Timestamp is the read side of TimestampArg: PostgreSQL returns a time.Time, SQLite the RFC3339 text that was written. It leaves an unparseable value invalid rather than failing the scan, so one bad row does not fail a page of results, as before. Two PostgreSQL JSON lookups now use the `#>>` spelling the indexes in jsonPathIndexes are built on. `data->'response_body'->>'id'` is a different expression to the planner, so those indexes could not be used. Coverage: the store tests paired with the reader ran on SQLite only. They now run through sqlxtest on both engines — auditlog goes from 0 to 19 PostgreSQL subtests. reader_postgresql_test.go, 205 lines of reflection-based fake pgx rows asserting NULL handling, is deleted: the same behaviour is now asserted against a real database. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(usage): let GetCacheOverview own its cached-only scope The admin handler set params.CacheMode = CacheModeCached before calling GetCacheOverview, which every implementation then overrides anyway. The duplication made the guarantee look like the caller's job, and the only thing asserting it was three admin tests reading it back off a stub reader that cannot enforce anything. The handler no longer sets it, the interface documents that the method overrides rather than honours CacheMode, and a new usage test asserts the real behaviour: passing "all" or "uncached" still returns cached rows only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(storage): run store suites against MongoDB MongoDB stays hand-written rather than sharing an implementation with the SQL backends, which makes it the one place a behaviour can drift unnoticed — and 12 of its 16 stores had no test that touched a database. mongotest mirrors sqlxtest: it hands a suite an empty database and drops it afterwards, skipping unless MONGO_TEST_DSN names a reachable server. Names are bounded to MongoDB's 64-byte limit and stripped of the characters it rejects, which a nested subtest name would otherwise overrun. Six domains — virtualmodels, failover, pricingoverrides, mcpgateway, batch, responsestore — now run their store round-trips through a suite written against the domain's Store interface, so the same assertions cover SQLite, PostgreSQL and MongoDB. Tests that reach past the interface for a raw handle stay SQL-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(storage): address review findings on the MongoDB harness - mongotest database names now carry the pid. The counter only separates subtests inside one process, and `go test ./...` runs packages in parallel — batch and responsestore both define TestStoreDelete, so two processes could create *and drop* the same database. A new test pins the naming rules: under 64 bytes, no character MongoDB rejects, pid and counter present. - Every mongotest server call is bounded, so an unreachable DSN skips promptly instead of stalling every opted-in suite, and cleanup cannot hang the test binary. - runStoreSuite closes the store it built. responsestore's SQL store runs a retention goroutine that the SQL-only helper stopped and the shared one did not. - The cache-mode test asserts token totals, not just hit counts: only one fixture row is a cache hit either way, so a mode that wrongly widened to both rows still counted 1. Verified it now fails without the reader's override. - The request-stats hour parse error is carried to the caller instead of being dropped, so the failure names the reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(auditlog): pin that timestamps are stored in UTC on both engines sqlx.Timestamp deliberately does not normalise on read, so the guarantee has to hold on the way in. This asserts it against the column itself, not the reader's interpretation: a caller writing 14:30+02:00 must leave 12:30 with a UTC marker in the row, on SQLite text and PostgreSQL timestamptz alike, and the instant must survive the round trip for both the entry and its attempts. Without it, a write that bypassed Dialect.TimestampArg would store a local-offset string on SQLite that sorts wrongly against every other row, and the date-range filters would quietly return the wrong day. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(dev): record the per-backend timestamp rendering as deferred Writes are UTC on every backend and now have a test asserting it against the stored column. Reads are not normalised, so the same entry serialises with a Z on SQLite and a server-local offset on PostgreSQL. Pre-existing, same instant either way, and normalising would change the timestamp string in every PostgreSQL deployment's admin API responses — so it is a separate decision rather than part of a refactor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(dev): trim the storage refactor note to lessons and open work Three of its deferred items were done in #587 and its MongoDB coverage numbers were stale, so the one section anyone reads was actively wrong. Removed what had a better home or no longer applied: a baseline snapshot of a commit two releases back, line-count tables git already holds, a paragraph on sqlxtest duplicating that package's own doc comment, the driver binding facts now stated on the type tokens, and a reconciliation against a file deleted in #586. 261 lines to 112, and renamed, since it stopped being a survey the moment the survey was acted on. Also fixes the dangling pointer to that deleted file in the 2026-07-04 architecture review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(dev): point the metrics notes at where hooks are actually wired The implementation notes told contributors to look in cmd/gomodel/main.go for the SetHooks call, twice. It moved to run/providers.go (defaultProviderFactory). A link check would not catch it: the file still exists, it just no longer contains what the doc says it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(dev): drop two docs that had drifted past being useful api-examples.md cited five retired models, so a chunk of it failed on paste. tests/e2e/release-e2e-scenarios.md covers the same ground executably and is verified every release, so the hand-maintained copy could only fall further behind it. 2026-03-16_ARCHITECTURE_SNAPSHOT.md predated the MCP gateway, rate limiting, provider credentials and the storage refactor. The 2026-07-04 architecture review had already flagged both dated snapshots as looking authoritative while stale; this executes that for the one in docs/dev and records that its sibling in docs/ still has the problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(auditlog): make the UTC assertion independent of the server's time zone The check read timestamp::text, and PostgreSQL renders TIMESTAMPTZ in the *session* time zone — so it asserted the UTC wall clock only because this container happens to default to UTC. Against a server on Europe/Warsaw it failed on perfectly correct data: stored timestamp = "2026-07-25 14:30:00.123456+02", want the 12:30 UTC wall clock Both verified: the previous assertion fails under PGTZ=Europe/Warsaw, the projection through AT TIME ZONE 'UTC' passes under both. Also covers audit_log_attempts.started_at, which was only checked through the hydrated instant, and makes the cases table-driven across UTC, a summer +02:00 offset and a winter +01:00 one, so the contract is exercised either side of a DST change rather than at a single offset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Jul 26, 2026
SantiagoDePolonia
added a commit
that referenced
this pull request
Jul 29, 2026
…615) * fix(auditlog): cast audit thread key to text for legacy uuid schemas PostgreSQL databases created before the unified schema (#586) keep their original uuid audit_logs.id column - CREATE TABLE IF NOT EXISTS never retypes it. The session thread key added in #602 coalesces id with the text session_id column, so every GET /admin/audit/sessions call on such a database failed with SQLSTATE 42804 (COALESCE types text and uuid cannot be matched), leaving the Audit Logs page empty in its default group-by-session view. Cast the id to text inside the thread key; the cast is valid on both backends and a no-op on SQLite. Covered by a regression test that recreates the exact pre-unification PostgreSQL DDL and runs GetSessions against it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(auditlog): pin per-thread latest entry in legacy-schema test Table-drive the session summaries and assert Latest.ID for the grouped thread too, so a regression in the per-thread recency ranking on the uuid id column cannot slip past the count check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 16, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Collapses the triplicated SQL store layer onto one implementation per domain, and fixes the connection and shutdown wiring that duplication was hiding.
Full write-up, including what is deliberately left and why:
docs/dev/2026-07-25_backend-refactor-survey.md.Why
Every persisted domain shipped three near-identical stores plus a factory — 18,259 lines, 15.5% of the hand-written backend. Normalising placeholders and type names, the SQLite and PostgreSQL halves were 50–83% textually identical; the entire difference was six mechanical things (driver handle,
?vs$n, no-rows sentinel,RowsAffectedsignature,Execarity, DDL type names).The coverage asymmetry mattered more than the line count:
…and those four PostgreSQL "tests" only asserted generated SQL strings. No PostgreSQL store code executed against a database anywhere in the suite. It had drifted accordingly:
failovernormalised padded primary keys through two independently written migrations, only one of them ever verified.What changed
internal/storage/sqlxabsorbs exactly those six differences. 16 of 17 domains now have one store implementation.store_sql*.gointernal/storage/sqlxPostgreSQL subtests executing against a real database: 0 → 91, wired into CI with a
postgres:18-alpineservice.Two things the adapter deliberately does not abstract, both settled by probing the drivers rather than assuming:
boolbinds to SQLiteINTEGERand PGBOOLEAN, anINTEGERscans into*bool,TEXT/JSON/JSONBall scan into[]byte. TheboolToSQLitehelpers and thesql.NullString-vs-*stringsplit were accidental divergence, never dialect requirements.Dialect()check and labelled:conversationstore's JSON mutations (JSON1 functions vs jsonb operators — must be one server-sideUPDATEor concurrent Responses turns clobber each other), thefailover/ratelimitlegacy migrations,budget.SumUsageCost, andauditlog's timestamp binding.Also fixed: the app opened up to 15 connections to the same database when audit logging and usage tracking were both disabled — each subsystem had a
Newthat calledstorage.Newitself. Now one handle opened inapp.New; 14 duplicate constructors and 19Result.Storagefields deleted.Shutdown's 15 copies of the same close block become an ordered list with the connection closed last: 194 → 102 lines.Behaviour changes worth reviewing
workflow_versions.created_aton PostgreSQL wasTIMESTAMPTZwhile SQLite used INTEGER unix seconds — the only table where the backends disagreed on representation. It now converges on unix seconds (what every other table already used on both engines) via an in-placeEXTRACT(EPOCH FROM ...)migration inNewSQLStore. This is the one change that touches existing data.InTxon SQLite now usesBEGIN IMMEDIATE, taking the write lock up front instead of lazily.Verification
Every commit passed the full pre-commit gate (
make test-race,make lint,make fix-check) across all build tags. Beyond that, the whole suite ran against a live PostgreSQL 18, and the built binary was smoke-tested on both backends — boot,/health, an auth-gated/v1/models, graceful shutdown.That smoke test is what caught the
workflow_versionsbreak, and it points at the useful lesson: a conformance suite over fresh schemas proves the dialects agree but says nothing about upgrading a database written by an older release. Migration paths now have tests that start from the legacy table shape (guardrails,mcpgateway,failover,ratelimit,workflows,auditlog).Not done, deliberately
The
usagestore and both analytics readers. The readers are the one place where the divergence is real analytics SQL, not duplication. Theusagestore is coupled to its reader —RecalculatePricingbuilds rows from the reader's condition builders, and the two implementations differ semantically (SQLite paginates byid; PostgreSQL takes every matching row withFOR UPDATE), so unifying now would change behaviour on one engine. It should follow the reader rather than lead it.Also deferred with reasons in §7: the
CacheModeCachedfour-owner cleanup, the config-shadows-store precedence question (9 subsystems, 3 different semantics — a product decision), and MongoDB, which stays hand-written by decision.🤖 Generated with Claude Code
Summary by CodeRabbit
workflow_versions.created_at(unix seconds, UTC, floored).