Skip to content

feat: added migrator to log store #612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 14, 2025
Merged

feat: added migrator to log store #612

merged 1 commit into from
Oct 14, 2025

Conversation

@TejasGhatte
Copy link
Collaborator

@TejasGhatte TejasGhatte commented Oct 13, 2025 •
edited
Loading

Summary

Moved the migrator package to a more general location and added database migrations for the logstore to standardize object type values.

Changes

  • Moved the migrator package from framework/configstore/migrator to framework/migrator to make it more generally accessible
  • Updated import paths in configstore files to reference the new migrator location
  • Added migrations to the logstore package to properly initialize tables
  • Implemented a migration to standardize object_type values in the logs table (converting formats like 'chat.completion' to 'chat_completion')
  • Replaced the AutoMigrate call in sqlite.go with the new triggerMigrations function

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (Next.js)
  • Docs

How to test

Run the application with a database that contains logs with the old object_type format and verify they are properly migrated:

# Core/Transports
go version
go test ./framework/logstore/...
go test ./framework/migrator/...

Breaking changes

  • Yes
  • No

Related issues

Improves database schema consistency and prepares for future migrations.

Security considerations

No security implications as this is an internal refactoring of database migration code.

Checklist

  • I read docs/contributing/README.md and followed the guidelines
  • I added/updated tests where appropriate
  • I updated documentation where needed
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

@TejasGhatte TejasGhatte mentioned this pull request Oct 13, 2025
Merged
15 tasks
@TejasGhatte Graphite App
Copy link
Collaborator Author

TejasGhatte commented Oct 13, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@TejasGhatte TejasGhatte mentioned this pull request Oct 13, 2025
Merged
16 tasks
@TejasGhatte TejasGhatte marked this pull request as ready for review October 13, 2025 13:54
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 13, 2025 •
edited
Loading

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Added managed database migrations for log storage, including initialization and standardization of object type values.

  • Refactor

    • Consolidated migration utilities into a common location without changing behavior.

  • Chores

    • Bumped versions across core, framework, transports, and multiple plugins.

  • Documentation

    • Updated changelogs and prerelease notes to reflect version updates and recent changes.

Walkthrough

Repointed migrator imports to framework/migrator, added two-step logstore migrations (create Log table + canonicalize object_type with rollback), replaced Log AutoMigrate with triggerMigrations, and bumped multiple package versions and changelogs.

Changes

Cohort / File(s) Summary
Migrator import path update
framework/configstore/migrations.go, framework/configstore/rdb.go, framework/configstore/store.go		 Changed import from github.com/maximhq/bifrost/framework/configstore/migrator to github.com/maximhq/bifrost/framework/migrator. No exported API changes.
Logstore migrations feature
framework/logstore/migrations.go, framework/logstore/sqlite.go		 Added triggerMigrations(ctx, db) orchestrating migrationInit (create/drop logs table) and migrationUpdateObjectColumnValues (bulk UPDATE to normalize object_type with rollback). Replaced AutoMigrate(&Log{}) with triggerMigrations.
Version bumps & changelogs
core/version, framework/version, framework/changelog.md, transports/version, docs/changelogs/*, plugins/*/version, plugins/*/changelog.md		 Incremented version constants across core, framework, transports, and multiple plugins; updated changelogs to reflect dependency bumps and migration additions. Mostly metadata and docs edits.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor App
  participant LogInit as LogStore Init
  participant DB as GORM DB
  participant Migrator as Migrator

  App->>LogInit: initialize logstore(ctx)
  LogInit->>DB: open DB with ctx
  LogInit->>Migrator: triggerMigrations(ctx, db)
  activate Migrator

  rect rgba(200,230,255,0.12)
    note right of Migrator: migrationInit — ensure `logs` table exists
    Migrator->>DB: CREATE TABLE IF NOT EXISTS logs ...
    DB-->>Migrator: ok / error
  end

  alt init failed
    Migrator-->>LogInit: error
    LogInit-->>App: propagate error
  else init ok
    rect rgba(220,255,220,0.12)
      note right of Migrator: migrationUpdateObjectColumnValues — normalize `object_type`
      Migrator->>DB: UPDATE logs SET object_type = CASE ...
      DB-->>Migrator: ok / error
    end

    alt update failed
      Migrator-->>LogInit: error (rollback available)
      LogInit-->>App: propagate error
    else update ok
      Migrator-->>LogInit: success
      LogInit-->>App: ready
    end
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Suggested reviewers

  • akshaydeo
  • danpiths

Poem

I nibble at schema roots tonight,
Hop, hop — tables set just right.
Object names snug in tidy rows,
Rollbacks guard where wild wind blows.
A rabbit hops — migrations light! 🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title concisely describes the main enhancement of adding a migrator to the log store and follows a conventional commit style, making it clear and focused without extraneous details.
Description Check ✅ Passed The description adheres closely to the repository template by including a clear summary, detailed change list, type of change, affected areas, testing instructions, breaking change indicator, related issues, security considerations, and checklist; the only omitted section is optional screenshots for UI changes, which are not applicable here.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 10-13-feat_added_migrator_to_log_store

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@TejasGhatte TejasGhatte force-pushed the 10-13-feat_added_migrator_to_log_store branch from 50c4012 to 3a664da Compare October 13, 2025 13:55
coderabbitai[bot]
coderabbitai bot requested changes Oct 13, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2bf3a1d and 3a664da.

📒 Files selected for processing (5)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (6)
framework/configstore/migrations.go (1)

7-7: LGTM! Import path refactor aligns with the migrator package consolidation.

The update to use github.com/maximhq/bifrost/framework/migrator is consistent with the broader PR objective of moving the migrator to a shared location.

framework/configstore/rdb.go (1)

10-10: LGTM! Consistent with the migrator package consolidation.

framework/configstore/store.go (1)

9-9: LGTM! Import path update is consistent with the migrator refactor.

framework/logstore/sqlite.go (1)

25-28: LGTM! Migration to explicit migration framework follows GORM best practices.

Replacing AutoMigrate with triggerMigrations provides better control over schema versioning and rollback capabilities, which is recommended for production use.

Based on learnings.

framework/logstore/migrations.go (2)

11-21: LGTM! Clean orchestration of migrations with proper error propagation.

The function correctly sequences initialization and data migration steps.

23-53: LGTM! Initialization migration follows best practices.

The migration properly checks for table existence before creation and includes a rollback function.

@TejasGhatte TejasGhatte force-pushed the 10-13-feat_added_migrator_to_log_store branch 2 times, most recently from 7620085 to 09f41d5 Compare October 13, 2025 15:53
coderabbitai[bot]
coderabbitai bot reviewed Oct 13, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
framework/logstore/migrations.go (1)

62-84: Lossy rollback mappings; choose a reversible plan or document limitation.
Forward maps multiple originals to one target (e.g., 'completion' and 'text.completion' → 'text_completion'; 'responses.response' and 'response' → 'responses'). Rollback can’t restore originals.

Option A (reversible): remove ambiguous forward mappings to keep 1:1:

 				SET object_type = CASE object_type
 					WHEN 'chat.completion' THEN 'chat_completion'
 					WHEN 'text.completion' THEN 'text_completion'
-					WHEN 'completion' THEN 'text_completion'
 					WHEN 'list' THEN 'embedding'
@@
-					WHEN 'responses.response' THEN 'responses'
 				WHERE object_type IN (
-					'chat.completion', 'text.completion', 'completion', 'list',
+					'chat.completion', 'text.completion', 'list',
@@
-					'response', 'response.completion.chunk', 'responses.response'
+					'response', 'response.completion.chunk'
 				)

Option B (keep coalescing): add a clear comment that rollback is non‑lossless and acceptable.

Confirm which option you prefer and update accordingly.

Also applies to: 96-116

🧹 Nitpick comments (7)
framework/logstore/migrations.go (6)

11-21: Fix GoDoc to match function name.
Comment says “Migrate” but function is triggerMigrations. Align for clarity.

-// Migrate performs the necessary database migrations.
+// triggerMigrations performs the necessary database migrations.

29-37: Avoid shadowing the imported package name with a local variable.
Local migrator := tx.Migrator() shadows the migrator import. Rename for readability.

-            migrator := tx.Migrator()
-            if !migrator.HasTable(&Log{}) {
-                if err := migrator.CreateTable(&Log{}); err != nil {
+            gm := tx.Migrator()
+            if !gm.HasTable(&Log{}) {
+                if err := gm.CreateTable(&Log{}); err != nil {
                     return err
                 }
             }
-            migrator := tx.Migrator()
-            // Drop children first, then parents (adjust if your actual FKs differ)
-            if err := migrator.DropTable(&Log{}); err != nil {
+            gm := tx.Migrator()
+            // Drop children first, then parents (adjust if your actual FKs differ)
+            if err := gm.DropTable(&Log{}); err != nil {
                 return err
             }

Also applies to: 39-46

49-51: Wrap underlying errors with %w to preserve cause.
Use %w instead of %s.

-    return fmt.Errorf("error while running db migration: %s", err.Error())
+    return fmt.Errorf("error while running db migration: %w", err)
-    return fmt.Errorf("error while running object column migration: %s", err.Error())
+    return fmt.Errorf("error while running object column migration: %w", err)

Also applies to: 127-130

57-58: Enable transactions for migrations for stronger guarantees.
Current runs use DefaultOptions.UseTransaction=false. Prefer transactions to avoid partial state (especially if you add more steps later).

- m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{
+ opts := *migrator.DefaultOptions
+ opts.UseTransaction = true
+ m := migrator.New(db, &opts, []*migrator.Migration{{

Apply similarly to the init migration.

Also applies to: 25-27

41-46: Rollback note about children is misleading.
Comment says “Drop children first” but only drops Log. Either implement actual child drops or adjust comment.

12-21: Optional: consolidate migrations into one runner.
You can register both steps in a single migrator.New call to ensure ordering within one run and fewer writes to the migrations table.

If desired, I can draft a combined variant.

framework/changelog.md (1)

4-5: Changelog looks good; consider noting rollback nuance.
If you keep coalescing object_type values, add a note that rollback may not restore exact originals.

I can propose wording if helpful.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3a664da and 7620085.

📒 Files selected for processing (17)
  • core/version (1 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (10)
  • plugins/telemetry/version
  • plugins/governance/version
  • plugins/otel/version
  • plugins/mocker/version
  • core/version
  • framework/version
  • plugins/logging/version
  • plugins/jsonparser/version
  • plugins/maxim/version
  • transports/version
🚧 Files skipped from review as they are similar to previous changes (4)
  • framework/logstore/sqlite.go
  • framework/configstore/rdb.go
  • framework/configstore/migrations.go
  • framework/configstore/store.go
🧰 Additional context used
🧬 Code graph analysis (1)
framework/logstore/migrations.go (1)
framework/migrator/migrator.go (3)
  • New (131-149)
  • DefaultOptions (100-106)
  • Migration (62-69)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (2)
plugins/semanticcache/version (1)

1-1: Version bump OK.
No issues spotted.

framework/logstore/migrations.go (1)

25-27: Ensure migration ID uniqueness
Migration ID "init" is too generic and may collide if other components record the same ID in the shared migrations table.

  • Rename to a timestamped/namespaced ID, e.g.:
-   ID: "init",
+   ID: "20251013_logstore_init",
  • Or isolate this package’s migrations by customizing opts.TableName:
opts := *migrator.DefaultOptions
opts.TableName = "logstore_migrations"
m := migrator.New(db, &opts, migrations)

coderabbitai[bot]
coderabbitai bot reviewed Oct 13, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
framework/changelog.md (2)

5-5: Add ops note + mapping example for the new migrations.

Call out the data change and operational impact (queries/filters), and that migrations auto‑run.

-- Feat: Moved the migrator package to a more general location and added database migrations for the logstore to standardize object type values.
+- Feat: Move migrator to framework/migrator and add logstore migrations (init + data) to canonicalize logs.table.object_type.
+  - Example mapping: "chat.completion" -> "chat_completion".
+  - Ops note: Migrations run automatically; if you filter by object_type, update any references to the new canonical values.
+  - No breaking changes expected.

4-4: Clarify dependency upgrade entry in changelog

Specify the previous version and full module path. For example, in framework/changelog.md:

- Upgrade dependency: core to 1.2.6
+ Upgrade dependency: github.com/maximhq/bifrost/core from v1.2.5 to v1.2.6
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3a664da and 09f41d5.

📒 Files selected for processing (17)
  • core/version (1 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (11)
  • framework/version
  • plugins/jsonparser/version
  • plugins/otel/version
  • plugins/mocker/version
  • plugins/maxim/version
  • plugins/governance/version
  • core/version
  • transports/version
  • plugins/semanticcache/version
  • plugins/telemetry/version
  • plugins/logging/version
🚧 Files skipped from review as they are similar to previous changes (3)
  • framework/logstore/migrations.go
  • framework/configstore/migrations.go
  • framework/logstore/sqlite.go
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (2)
framework/configstore/store.go (1)

9-9: LGTM! Import path update is correct.

The migrator package relocation from framework/configstore/migrator to framework/migrator makes it reusable across the framework (e.g., in logstore). The import path is updated correctly and the usage remains consistent.

framework/configstore/rdb.go (1)

10-10: LGTM — no remaining references to framework/configstore/migrator.

@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 10-07-feat_responses_added_to_logs branch from 2bf3a1d to fff112b Compare October 13, 2025 17:02
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 10-13-feat_added_migrator_to_log_store branch from 09f41d5 to 3d59583 Compare October 13, 2025 17:02
coderabbitai[bot]
coderabbitai bot requested changes Oct 13, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
plugins/governance/changelog.md (1)

4-4: Consider aligning changelog wording for consistency.

Other changelog files in this PR use "Upgrade dependency: core to 1.2.6 and framework to 1.1.6" while this one uses "Chore: using core 1.2.6 and framework 1.1.6". Using consistent wording across changelogs improves maintainability.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 09f41d5 and 3d59583.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (10)
  • plugins/telemetry/version
  • plugins/jsonparser/changelog.md
  • core/changelog.md
  • plugins/mocker/changelog.md
  • plugins/jsonparser/version
  • plugins/semanticcache/changelog.md
  • plugins/otel/changelog.md
  • plugins/mocker/version
  • framework/configstore/store.go
  • plugins/maxim/changelog.md
🚧 Files skipped from review as they are similar to previous changes (11)
  • transports/version
  • plugins/semanticcache/version
  • plugins/logging/version
  • core/version
  • framework/configstore/migrations.go
  • framework/logstore/migrations.go
  • framework/logstore/sqlite.go
  • framework/version
  • framework/configstore/rdb.go
  • plugins/governance/version
  • plugins/maxim/version
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
plugins/logging/changelog.md (1)

4-4: LGTM!

The dependency version bumps are consistent with the broader version updates across the PR.

plugins/otel/version (1)

1-1: LGTM!

Version bump is consistent with the coordinated release across core and framework.

framework/changelog.md (1)

4-5: LGTM!

The changelog accurately documents the dependency upgrade and the key feature additions (migrator relocation and logstore migrations) described in the PR objectives.

transports/changelog.md (1)

4-8: LGTM!

The dependency upgrades are consistent with the PR-wide version bumps, and the additional changelog entries appropriately document the transport layer enhancements.

coderabbitai[bot]
coderabbitai bot requested changes Oct 13, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (6)
framework/logstore/migrations.go (6)

11-12: Update comment to match function name.

Comment says "Migrate" but function is triggerMigrations. Align for clarity.

-// Migrate performs the necessary database migrations.
+// triggerMigrations performs the necessary database migrations.

25-27: Avoid migration table/ID collisions; prefer unique table name and IDs.

Default migrator table is "migrations" with IDs "init"/"update_object_column_values". If this DB is shared with other packages, collisions are likely. Either ensure logstore uses a separate DB, or set a package-specific table name and more unique IDs (e.g., timestamped/prefixed).

Apply this pattern if the DB is shared:

- m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{
-   ID: "init",
+ opts := *migrator.DefaultOptions
+ opts.TableName = "log_migrations"
+ m := migrator.New(db, &opts, []*migrator.Migration{{
+   ID: "logstore_0001_init",
- m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{
-   ID: "update_object_column_values",
+ opts := *migrator.DefaultOptions
+ opts.TableName = "log_migrations"
+ m := migrator.New(db, &opts, []*migrator.Migration{{
+   ID: "logstore_0002_update_object_type",

As per migrator defaults in framework/migrator/migrator.go (DefaultOptions.TableName = "migrations"). [Based on relevant snippets]

Also applies to: 57-59

29-31: Rename local migrator var to avoid shadowing the imported package.

Prevents confusion and future edits needing the package in this scope.

- migrator := tx.Migrator()
- if !migrator.HasTable(&Log{}) {
-   if err := migrator.CreateTable(&Log{}); err != nil {
+ mg := tx.Migrator()
+ if !mg.HasTable(&Log{}) {
+   if err := mg.CreateTable(&Log{}); err != nil {
      return err
    }
  }
- migrator := tx.Migrator()
+ mg := tx.Migrator()
 // Drop children first, then parents (adjust if your actual FKs differ)
- if err := migrator.DropTable(&Log{}); err != nil {
+ if err := mg.DropTable(&Log{}); err != nil {
   return err
 }

Also applies to: 40-43

41-46: Guard DropTable with existence check (and fix misleading comment).

Unconditional drop can error on some drivers; comment mentions children but only Log is dropped.

- // Drop children first, then parents (adjust if your actual FKs differ)
- if err := migrator.DropTable(&Log{}); err != nil {
+ // Drop log table if present. If there are FKs, drop dependents first.
+ if mg.HasTable(&Log{}) {
+   if err := mg.DropTable(&Log{}); err != nil {
      return err
- }
+   }
+ }

48-51: Wrap errors with %w and include migration IDs.

Improves traceability and preserves original error.

- return fmt.Errorf("error while running db migration: %s", err.Error())
+ return fmt.Errorf("logstore migration 'init' failed: %w", err)
- return fmt.Errorf("error while running object column migration: %s", err.Error())
+ return fmt.Errorf("logstore migration 'update_object_type' failed: %w", err)

Also applies to: 125-128

57-61: Enable transactions for data migration.

Data rewrites should be atomic to avoid partial application on failure.

- m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{
+ opts := *migrator.DefaultOptions
+ opts.UseTransaction = true
+ m := migrator.New(db, &opts, []*migrator.Migration{{
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 09f41d5 and 3d59583.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (11)
  • plugins/semanticcache/changelog.md
  • plugins/logging/changelog.md
  • plugins/mocker/changelog.md
  • plugins/governance/changelog.md
  • plugins/governance/version
  • plugins/maxim/changelog.md
  • core/changelog.md
  • docs/changelogs/v1.3.0-prerelease6.mdx
  • transports/version
  • plugins/jsonparser/changelog.md
  • framework/version
🚧 Files skipped from review as they are similar to previous changes (8)
  • framework/logstore/sqlite.go
  • core/version
  • framework/configstore/rdb.go
  • plugins/maxim/version
  • plugins/semanticcache/version
  • framework/configstore/store.go
  • plugins/jsonparser/version
  • plugins/mocker/version
🧰 Additional context used
🧬 Code graph analysis (1)
framework/logstore/migrations.go (1)
framework/migrator/migrator.go (3)
  • New (131-149)
  • DefaultOptions (100-106)
  • Migration (62-69)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (9)
plugins/otel/changelog.md (1)

4-4: LGTM!

Dependency upgrade documented clearly and consistently with other modules.

plugins/logging/version (1)

1-1: LGTM!

Version bump is consistent with the dependency upgrades across the repository.

plugins/telemetry/version (1)

1-1: LGTM!

Version bump aligns with the framework and core upgrades.

plugins/telemetry/changelog.md (1)

4-5: LGTM!

Changelog accurately documents the dependency upgrades and the latency metric fix, consistent with related changes in transports.

plugins/otel/version (1)

1-1: LGTM!

Version bump is consistent with the core and framework upgrades.

framework/changelog.md (1)

4-5: LGTM!

Changelog clearly documents the migrator relocation and the new logstore migration features, matching the PR objectives.

transports/changelog.md (1)

4-8: LGTM!

Changelog comprehensively documents all changes including dependency upgrades, new streaming features, bug fixes, and the latency metric adjustment. All entries are clear and consistent with related changes in other modules.

framework/configstore/migrations.go (1)

7-7: All migrator import paths have been updated
Search for the old import path returned no matches; the change is complete.

framework/logstore/migrations.go (1)

62-82: Forward mapping looks correct and is scoped.

Mappings are one-to-one now and guarded by a WHERE IN set. Good.

Please confirm if any additional legacy values (e.g., 'responses.response' or plain 'completion') still exist in older datasets; if yes, add them explicitly or document intentional omission.

@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 10-13-feat_added_migrator_to_log_store branch from 3d59583 to 4ffdb58 Compare October 13, 2025 17:39
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 10-07-feat_responses_added_to_logs branch from fff112b to b9d12c7 Compare October 13, 2025 17:39
coderabbitai[bot]
coderabbitai bot requested changes Oct 13, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
docs/changelogs/v1.3.0-prerelease6.mdx (1)

7-7: Align dependency versions across sections

Top line says core 1.2.6/framework 1.1.6, but:

  • Framework section (Line 28) still shows core 1.2.5.
  • Governance section (Line 34) still shows core 1.2.5/framework 1.1.5.

Please update those to 1.2.6/1.1.6 (or note intentional differences).

Suggested replacements:

  • Framework: “- Upgrade dependency: core to 1.2.6 and framework to 1.1.6”
  • governance: “- Chore: using core 1.2.6 and framework 1.1.6”
🧹 Nitpick comments (2)
framework/logstore/sqlite.go (1)

25-27: Wrap migration error for context & confirm idempotency

  • Optional refactor: add context to migration failure:
-	if err := triggerMigrations(ctx, db); err != nil {
-		return nil, err
+	if err := triggerMigrations(ctx, db); err != nil {
+		return nil, fmt.Errorf("logstore: migrations failed: %w", err)
 	}
  • Verify that triggerMigrations (via migrator.New(..., DefaultOptions, ...)) is idempotent and safe for concurrent runs (uses locking/transactions).
transports/changelog.md (1)

4-8: Polish wording/capitalization; verify scope alignment with this PR

Minor nits for consistency and clarity. Also, confirm these streaming notes belong in this PR’s changelog (PR focuses on migrator/logstore).

- - Upgrade dependency: core to 1.2.6 and framework to 1.1.6
+ - Bump core to 1.2.6 and framework to 1.1.6.

- - Added Responses streaming across all providers.
+ - Added response streaming across all providers.
+   # If "Responses" is a proper noun in this project, keep it capitalized instead.

- - Fixed bedrock chat streaming decoding issues.
+ - Fixed Bedrock chat streaming decoding.

- - Added raw response support for all streaming requests.
+ - Added raw response support for all streaming requests.

- - Removed last token's accumulated latency from inter token latency metric.
+ - Removed last token's accumulated latency from inter-token latency metric.

Questions:

  • Is “Responses” a product/API name here? If yes, keep it capitalized.
  • Do these streaming changes ship in this PR/stack? If not, move them to the correct release note to avoid confusion.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3d59583 and 4ffdb58.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (7)
  • core/changelog.md
  • transports/version
  • plugins/jsonparser/changelog.md
  • plugins/telemetry/version
  • framework/version
  • plugins/jsonparser/version
  • plugins/otel/version
🚧 Files skipped from review as they are similar to previous changes (14)
  • plugins/semanticcache/version
  • plugins/logging/version
  • plugins/logging/changelog.md
  • plugins/semanticcache/changelog.md
  • plugins/mocker/changelog.md
  • plugins/otel/changelog.md
  • plugins/mocker/version
  • framework/changelog.md
  • framework/configstore/rdb.go
  • framework/configstore/store.go
  • framework/logstore/migrations.go
  • plugins/maxim/version
  • plugins/maxim/changelog.md
  • framework/configstore/migrations.go
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (3)
core/version (1)

1-1: LGTM

Version bump looks good.

plugins/governance/version (1)

1-1: LGTM

Version bump looks good.

plugins/telemetry/changelog.md (1)

4-5: LGTM

Changelog entries and dependency bump look correct.

coderabbitai[bot]
coderabbitai bot requested changes Oct 13, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
docs/changelogs/v1.3.0-prerelease6.mdx (1)

28-28: Align dependency versions across sections.

"Framework" and "governance" still reference older versions. Update to match 1.2.6/1.1.6 for consistency.

Apply this diff:

- - Upgrade dependency: core to 1.2.5
+ - Upgrade dependency: core to 1.2.6 and framework to 1.1.6
@@
-- Chore: using core 1.2.5 and framework 1.1.5
+- Chore: using core 1.2.6 and framework 1.1.6

Also applies to: 34-34

framework/logstore/migrations.go (1)

91-114: Rollback remains lossy and may change pre-existing canonical rows.

Mappings are many-to-one (e.g., response → responses), and rollback targets canonical values globally, not just rows changed by this migration. This cannot restore originals accurately.

Consider: track originals in a backup column during migrate and use it for rollback, or document that rollback is best-effort and acceptable operationally.

🧹 Nitpick comments (4)
plugins/governance/changelog.md (1)

4-4: Align entry with Conventional Commits and clearer wording

Prefer consistent lowercase type, scoped deps, and explicit version bumps.

- - Chore: using core 1.2.6 and framework 1.1.6
+ - chore(deps): bump core to v1.2.6, framework to v1.1.6
framework/logstore/migrations.go (3)

60-88: Guard updates if the logs table is absent.

If "init" didn’t run (e.g., due to ID collision or custom setup), this UPDATE will fail. Add a HasTable guard.

Apply this diff:

 		Migrate: func(tx *gorm.DB) error {
 			tx = tx.WithContext(ctx)
 
+			if !tx.Migrator().HasTable(&Log{}) {
+				// Nothing to update if logs table doesn't exist.
+				return nil
+			}
+
 			updateSQL := `
 				UPDATE logs 
 				SET object_type = CASE object_type

50-51: Wrap migration errors with %w, not %s.

Preserve error context for callers.

Apply this diff:

-		return fmt.Errorf("error while running db migration: %s", err.Error())
+		return fmt.Errorf("error while running db migration: %w", err)
@@
-		return fmt.Errorf("error while running object column migration: %s", err.Error())
+		return fmt.Errorf("error while running object column migration: %w", err)

Also applies to: 125-128

11-12: Fix comment to match function name.

Use an accurate doc comment for triggerMigrations.

Apply this diff:

-// Migrate performs the necessary database migrations.
+// triggerMigrations runs all logstore migrations.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3d59583 and 4ffdb58.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (4)
  • plugins/governance/version
  • plugins/otel/version
  • plugins/jsonparser/changelog.md
  • plugins/telemetry/version
🚧 Files skipped from review as they are similar to previous changes (17)
  • plugins/maxim/version
  • plugins/logging/version
  • framework/configstore/store.go
  • plugins/maxim/changelog.md
  • core/changelog.md
  • framework/version
  • plugins/jsonparser/version
  • framework/configstore/migrations.go
  • plugins/mocker/changelog.md
  • plugins/mocker/version
  • core/version
  • transports/version
  • framework/configstore/rdb.go
  • framework/changelog.md
  • plugins/logging/changelog.md
  • plugins/semanticcache/changelog.md
  • plugins/semanticcache/version
🧰 Additional context used
🧬 Code graph analysis (1)
framework/logstore/migrations.go (1)
framework/migrator/migrator.go (3)
  • New (131-149)
  • DefaultOptions (100-106)
  • Migration (62-69)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
plugins/telemetry/changelog.md (1)

4-5: Changelog entry looks good.

Version bumps and metric note read clearly.

transports/changelog.md (1)

4-8: LGTM.

Entries align with dependency bumps and streaming fixes/features.

framework/logstore/sqlite.go (1)

25-29: Good switch to explicit migrations.

Calling triggerMigrations keeps init deterministic and avoids implicit AutoMigrate.

framework/logstore/migrations.go (1)

41-45: Destructive rollback. Confirm policy.

Dropping logs in rollback is data-lossy. Ensure down-migrations are disabled in prod or gate this behind an explicit safety check.

coderabbitai[bot]
coderabbitai bot requested changes Oct 13, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (2)
docs/changelogs/v1.3.0-prerelease6.mdx (2)

28-28: Update Framework section to match dependency versions.

This line shows core 1.2.5, but the Bifrost (Line 7) and plugin sections (Lines 40, 45, 50, etc.) have been updated to core 1.2.6. This creates an inconsistency in the changelog.

-- Upgrade dependency: core to 1.2.5
+- Upgrade dependency: core to 1.2.6

34-34: Update governance section to match dependency versions.

This line shows core 1.2.5 and framework 1.1.5, but other sections have been updated to core 1.2.6 and framework 1.1.6. This creates an inconsistency in the changelog.

-- Chore: using core 1.2.5 and framework 1.1.5
+- Chore: using core 1.2.6 and framework 1.1.6
🧹 Nitpick comments (1)
transports/changelog.md (1)

4-8: Polish wording, tense, and capitalization for consistency.

Recommend small edits for clarity and consistency (past tense, AWS Bedrock capitalization, hyphenation):

-- Upgrade dependency: core to 1.2.6 and framework to 1.1.6
- Added Responses streaming across all providers.
- Fixed bedrock chat streaming decoding issues.
- Added raw response support for all streaming requests.
- Removed last token's accumulated latency from inter token latency metric.
+ - Bumped dependencies: core to 1.2.6, framework to 1.1.6.
+ - Added response streaming across all providers.
+ - Fixed AWS Bedrock chat streaming decoding issues.
+ - Added raw-response support for all streaming requests.
+ - Removed the last token’s latency from the inter‑token latency metric.
  • Please confirm whether “Responses” is a proper product name here; if so, keep it capitalized (“Responses streaming”).
  • Also verify the pinned versions (core 1.2.6, framework 1.1.6) align with go.mod/go.work and their respective changelogs.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3d59583 and 4ffdb58.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (5)
  • core/changelog.md
  • plugins/otel/version
  • plugins/otel/changelog.md
  • plugins/semanticcache/version
  • plugins/maxim/version
🚧 Files skipped from review as they are similar to previous changes (13)
  • plugins/mocker/version
  • plugins/semanticcache/changelog.md
  • framework/configstore/migrations.go
  • plugins/jsonparser/version
  • framework/version
  • framework/logstore/sqlite.go
  • plugins/mocker/changelog.md
  • transports/version
  • plugins/logging/changelog.md
  • framework/configstore/rdb.go
  • framework/logstore/migrations.go
  • plugins/governance/changelog.md
  • plugins/logging/version
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (1)
framework/configstore/store.go (1)

9-9: Import path update verified

Migrator package exists at framework/migrator and exports Migration; no remaining references to framework/configstore/migrator.

coderabbitai[bot]
coderabbitai bot requested changes Oct 13, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
framework/logstore/migrations.go (1)

91-122: Rollback may incorrectly modify rows with pre-existing canonical values.

The rollback WHERE clause (lines 110-114) targets all rows with canonical values like 'responses' or 'chat_completion', not just rows that were changed by the forward migration. If any rows already contained these canonical values before the migration ran, the rollback would incorrectly transform them (e.g., 'responses' → 'response').

While it's likely that no rows have canonical values before this migration (since this migration introduces the standardization), the risk remains if:

  • The migration runs multiple times
  • Canonical values were manually inserted
  • There's concurrent access during migration

Consider these solutions:

Solution 1 (recommended): Accept best-effort rollback and document the limitation with a clear comment:

 	Rollback: func(tx *gorm.DB) error {
 		tx = tx.WithContext(ctx)
 
+		// NOTE: This rollback is best-effort and may incorrectly modify rows
+		// that already had canonical values before this migration. It should
+		// only be used in non-production environments for testing.
 		// Use a single CASE statement for efficient bulk rollback
 		rollbackSQL := `

Solution 2: Track original values using a backup column before migration (as suggested in the previous review):

 	Migrate: func(tx *gorm.DB) error {
 		tx = tx.WithContext(ctx)
 
+		// Add backup column to track original values
+		backupSQL := `
+			ALTER TABLE logs ADD COLUMN IF NOT EXISTS object_type_backup TEXT;
+			UPDATE logs 
+			SET object_type_backup = object_type
+			WHERE object_type IN (
+				'chat.completion', 'text.completion', 'list',
+				'audio.speech', 'audio.transcription', 'chat.completion.chunk',
+				'audio.speech.chunk', 'audio.transcription.chunk', 
+				'response', 'response.completion.chunk'
+			);
+		`
+		if err := tx.Exec(backupSQL).Error; err != nil {
+			return fmt.Errorf("failed to backup object_type values: %w", err)
+		}
+
 		updateSQL := `
 			UPDATE logs 
 			SET object_type = CASE object_type

Then update rollback to restore from backup:

 	Rollback: func(tx *gorm.DB) error {
 		tx = tx.WithContext(ctx)
 
-		// Use a single CASE statement for efficient bulk rollback
 		rollbackSQL := `
 			UPDATE logs 
-			SET object_type = CASE object_type
-				WHEN 'chat_completion' THEN 'chat.completion'
-				...
-			END
-			WHERE object_type IN (...)
+			SET object_type = object_type_backup
+			WHERE object_type_backup IS NOT NULL;
+			
+			ALTER TABLE logs DROP COLUMN object_type_backup;
 		`

Solution 3: Remove the rollback if your operational policy disallows down-migrations in production.

🧹 Nitpick comments (1)
framework/logstore/migrations.go (1)

25-25: Consider enabling transactions for atomic migrations.

Both migrations use migrator.DefaultOptions, which has UseTransaction: false. This means:

  • If migrationInit succeeds but migrationUpdateObjectColumnValues fails, the database is left in a partially migrated state
  • Individual SQL statements within each migration are not wrapped in a transaction

For data integrity, consider enabling transactions:

+var logstoreMigratorOptions = &migrator.Options{
+	TableName:                 "migrations",
+	IDColumnName:              "id",
+	IDColumnSize:              255,
+	UseTransaction:            true,  // Enable transactions for atomicity
+	ValidateUnknownMigrations: false,
+}
+
 func migrationInit(ctx context.Context, db *gorm.DB) error {
-	m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{
+	m := migrator.New(db, logstoreMigratorOptions, []*migrator.Migration{{

Apply the same change to migrationUpdateObjectColumnValues.

Note: Ensure your SQLite configuration supports WAL mode (already enabled in sqlite.go line 20) for safe concurrent access with transactions.

Also applies to: 57-57

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3d59583 and 4ffdb58.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (6)
  • plugins/mocker/version
  • plugins/telemetry/version
  • plugins/maxim/changelog.md
  • plugins/otel/version
  • framework/version
  • plugins/jsonparser/version
🚧 Files skipped from review as they are similar to previous changes (12)
  • plugins/mocker/changelog.md
  • plugins/semanticcache/version
  • framework/configstore/store.go
  • plugins/governance/version
  • docs/changelogs/v1.3.0-prerelease6.mdx
  • transports/version
  • plugins/logging/changelog.md
  • plugins/governance/changelog.md
  • core/version
  • plugins/jsonparser/changelog.md
  • plugins/logging/version
  • plugins/otel/changelog.md
🧰 Additional context used
🧬 Code graph analysis (1)
framework/logstore/migrations.go (1)
framework/migrator/migrator.go (3)
  • New (131-149)
  • DefaultOptions (100-106)
  • Migration (62-69)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (10)
plugins/maxim/version (1)

1-1: LGTM!

Version bump from 1.4.5 to 1.4.6 is appropriate for this release.

plugins/semanticcache/changelog.md (1)

4-4: LGTM!

Dependency version updates are consistent with the broader changes in this PR.

plugins/telemetry/changelog.md (1)

4-5: LGTM!

Changelog entries accurately reflect the dependency upgrades and the telemetry-specific fix for inter-token latency metric calculation.

framework/configstore/migrations.go (1)

7-7: LGTM!

Import path correctly updated to reflect the migrator package relocation from framework/configstore/migrator to framework/migrator. This change enables the migrator utility to be shared across other framework packages like logstore.

framework/configstore/rdb.go (1)

10-10: LGTM!

Import path correctly updated to use the relocated migrator package, consistent with the broader refactoring in this PR.

transports/changelog.md (1)

4-8: LGTM!

Changelog entries accurately document the dependency upgrades and transport-layer enhancements for streaming functionality across providers.

core/changelog.md (1)

4-6: LGTM!

Changelog entries accurately reflect the core streaming enhancements and fixes in this release.

framework/changelog.md (1)

4-5: LGTM!

Changelog entries accurately document the core dependency upgrade and the key architectural changes in this PR: migrator package relocation and introduction of structured logstore migrations.

framework/logstore/sqlite.go (1)

25-27: LGTM! Migration orchestration properly centralized.

The replacement of AutoMigrate with triggerMigrations provides better control over the migration process and enables versioned, reversible migrations. Context and error handling are correctly preserved.

framework/logstore/migrations.go (1)

12-21: LGTM! Clear orchestration with proper error handling.

The sequential execution of migrations with early error return is appropriate for this use case.

@TejasGhatte TejasGhatte changed the base branch from 10-07-feat_responses_added_to_logs to graphite-base/612 October 14, 2025 08:56
@TejasGhatte TejasGhatte force-pushed the 10-13-feat_added_migrator_to_log_store branch from 4ffdb58 to c67e977 Compare October 14, 2025 09:09
@TejasGhatte TejasGhatte changed the base branch from graphite-base/612 to 10-07-feat_responses_added_to_logs October 14, 2025 09:09
coderabbitai[bot]
coderabbitai bot requested changes Oct 14, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
docs/changelogs/v1.3.0-prerelease6.mdx (1)

28-35: Align the remaining sections with the 1.2.6/1.1.6 bump
Framework and governance still mention core 1.2.5 / framework 1.1.5, which contradicts the headline entry and the rest of the stack. Please update those lines (or explain the intentional divergence) so the prerelease changelog reports a consistent version matrix.

♻️ Duplicate comments (3)
docs/changelogs/v1.3.0-prerelease6.mdx (1)

70-71: Sync the telemetry entry wording with the plugin changelog
This section still advertises the old “First Token…” feature, but plugins/telemetry/changelog.md now records the fix (“Removed last token's accumulated latency…”). Please update the wording here to match the shipped change.

framework/logstore/migrations.go (2)

38-46: Do not drop existing logs on rollback
The rollback always drops the logs table, even if it pre-existed this migration. A down-migration would wipe customer data. Remove the rollback handler (or at least make it conditional), so rolling back cannot destroy unrelated tables.

Apply this diff:

-		Rollback: func(tx *gorm.DB) error {
-			tx = tx.WithContext(ctx)
-			migrator := tx.Migrator()
-			// Drop children first, then parents (adjust if your actual FKs differ)
-			if err := migrator.DropTable(&Log{}); err != nil {
-				return err
-			}
-			return nil
-		},
+		Rollback: nil,

91-118: Rollback rewrites canonical object_type rows
This CASE statement rewinds every responses, embedding, etc. row to their legacy names, even if those canonical values were present long before this migration. Attempting a rollback will corrupt unrelated data. Either persist the original value per row before the forward UPDATE and restore from that backup, or mark this migration irreversible by dropping the rollback entirely.

Apply this diff to make the migration non-reversible until a safe strategy is implemented:

-		Rollback: func(tx *gorm.DB) error {
-			tx = tx.WithContext(ctx)
-
-			// Use a single CASE statement for efficient bulk rollback
-			rollbackSQL := `
-				UPDATE logs 
-				SET object_type = CASE object_type
-					WHEN 'chat_completion' THEN 'chat.completion'
-					WHEN 'text_completion' THEN 'text.completion'
-					WHEN 'embedding' THEN 'list'
-					WHEN 'speech' THEN 'audio.speech'
-					WHEN 'transcription' THEN 'audio.transcription'
-					WHEN 'chat_completion_stream' THEN 'chat.completion.chunk'
-					WHEN 'speech_stream' THEN 'audio.speech.chunk'
-					WHEN 'transcription_stream' THEN 'audio.transcription.chunk'
-					WHEN 'responses' THEN 'response'
-					WHEN 'responses_stream' THEN 'response.completion.chunk'
-					ELSE object_type
-				END
-				WHERE object_type IN (
-					'chat_completion', 'text_completion', 'embedding', 'speech',
-					'transcription', 'chat_completion_stream', 'speech_stream',
-					'transcription_stream', 'responses', 'responses_stream'
-				)`
-
-			result := tx.Exec(rollbackSQL)
-			if result.Error != nil {
-				return fmt.Errorf("failed to rollback object_type values: %w", result.Error)
-			}
-
-			return nil
-		},
+		Rollback: nil,
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4ffdb58 and c67e977.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (4)
  • plugins/mocker/changelog.md
  • plugins/otel/changelog.md
  • plugins/semanticcache/changelog.md
  • plugins/governance/version
🚧 Files skipped from review as they are similar to previous changes (17)
  • framework/changelog.md
  • framework/configstore/store.go
  • plugins/mocker/version
  • plugins/otel/version
  • plugins/telemetry/changelog.md
  • core/version
  • plugins/semanticcache/version
  • plugins/governance/changelog.md
  • plugins/jsonparser/version
  • framework/version
  • plugins/telemetry/version
  • plugins/jsonparser/changelog.md
  • transports/version
  • core/changelog.md
  • plugins/maxim/version
  • framework/configstore/rdb.go
  • plugins/logging/version
🧰 Additional context used
🧬 Code graph analysis (1)
framework/logstore/migrations.go (1)
framework/migrator/migrator.go (3)
  • New (131-149)
  • DefaultOptions (100-106)
  • Migration (62-69)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (1)
framework/logstore/sqlite.go (1)

25-28: Migrations use context and are idempotent; confirm rollback on failure.

  • All migration steps call tx.WithContext(ctx), so cancellations and deadlines propagate.
  • migrationInit checks HasTable before CreateTable and migrationUpdateObjectColumnValues only updates matching rows—both safe to re-run.
  • Verify that m.Migrate() (from migrator.New) invokes the provided Rollback closures on failure to avoid partial schema state.

@TejasGhatte TejasGhatte changed the base branch from 10-07-feat_responses_added_to_logs to graphite-base/612 October 14, 2025 09:42
@TejasGhatte TejasGhatte force-pushed the 10-13-feat_added_migrator_to_log_store branch from c67e977 to 1fb2e03 Compare October 14, 2025 10:01
@TejasGhatte TejasGhatte changed the base branch from graphite-base/612 to 10-07-feat_responses_added_to_logs October 14, 2025 10:01
coderabbitai[bot]
coderabbitai bot requested changes Oct 14, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (5)
docs/changelogs/v1.3.0-prerelease6.mdx (3)

28-28: Update Framework dependency version to match other sections.

This line lists "core to 1.2.5" but line 7 and other sections show "core to 1.2.6". All sections document the same release (v1.3.0-prerelease6) and should have consistent dependency versions.

Apply this diff to fix the inconsistency:

-- Upgrade dependency: core to 1.2.5
+- Upgrade dependency: core to 1.2.6 and framework to 1.1.6

34-34: Update governance dependency versions to match other sections.

This line lists "core 1.2.5 and framework 1.1.5" but line 7 and other sections show "core 1.2.6 and framework 1.1.6". All sections document the same release (v1.3.0-prerelease6) and should have consistent dependency versions.

Apply this diff to fix the inconsistency:

-- Chore: using core 1.2.5 and framework 1.1.5
+- Chore: using core 1.2.6 and framework 1.1.6

71-71: Update telemetry changelog entry to reflect actual changes.

The current text mentions "Feat: Added First Token and Inter Token latency metrics" but according to the telemetry plugin changelog and the AI summary, the actual change was to remove last token's accumulated latency from the inter-token latency metric.

Apply this diff to correct the entry:

-- Feat: Added First Token and Inter Token latency metrics for streaming requests.
+- Fix: Removed last token's accumulated latency from inter token latency metric.
framework/configstore/rdb.go (1)

10-10: LGTM!

Import path update is consistent with the migrator package relocation.

framework/configstore/migrations.go (1)

7-7: LGTM!

Import path update is consistent with the migrator package relocation.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c67e977 and 1fb2e03.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (2 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (3)
  • plugins/maxim/version
  • plugins/otel/version
  • plugins/governance/changelog.md
🚧 Files skipped from review as they are similar to previous changes (15)
  • plugins/jsonparser/changelog.md
  • plugins/mocker/changelog.md
  • framework/changelog.md
  • framework/version
  • framework/logstore/sqlite.go
  • plugins/jsonparser/version
  • core/version
  • plugins/semanticcache/version
  • plugins/logging/version
  • plugins/semanticcache/changelog.md
  • transports/changelog.md
  • framework/logstore/migrations.go
  • transports/version
  • plugins/telemetry/changelog.md
  • plugins/mocker/version
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
plugins/logging/changelog.md (1)

4-4: Changelog entry aligns with dependency bumps.

Matches the core/framework version updates referenced elsewhere—good to go.

plugins/telemetry/version (1)

1-1: LGTM!

The version bump to 1.3.6 is appropriate and consistent with the release process.

framework/configstore/store.go (1)

9-9: No obsolete migrator imports detected
All references to github.com/maximhq/bifrost/framework/configstore/migrator have been removed.

framework/configstore/migrations.go (1)

46-46: Ensure migration ID change does not trigger a re-run on existing databases
The migrator stores IDs verbatim in its history table with no alias support—renaming "init" to "configstore_init" will be treated as a new migration. Revert to the original ID, introduce an alias for backward compatibility, or update existing schema_migrations records before rolling out.

coderabbitai[bot]
coderabbitai bot requested changes Oct 14, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
docs/changelogs/v1.3.0-prerelease6.mdx (1)

28-35: Make the version matrix consistent

Top-level Bifrost entry claims we shipped core 1.2.6/framework 1.1.6, but the Framework and governance sections still advertise 1.2.5/1.1.5. That contradiction makes the changelog misleading; please align these bullets (or explicitly call out intentional deltas) so every section reflects the same shipped versions.

- - Upgrade dependency: core to 1.2.5
+ - Upgrade dependency: core to 1.2.6 and framework to 1.1.6
…
- - Chore: using core 1.2.5 and framework 1.1.5
+ - Chore: using core 1.2.6 and framework 1.1.6
🧹 Nitpick comments (1)
framework/logstore/migrations.go (1)

63-137: Well-structured migration with accurate one-to-one mappings.

The many-to-one mapping issues flagged in previous reviews have been resolved—all forward and rollback mappings are now exact one-to-one inverses. The WHERE clauses correctly filter only the values being transformed, and the CASE statements match their corresponding filters.

Optional enhancement: Consider adding a comment above the rollback function documenting that it transforms all rows with canonical values (including any pre-existing canonical rows), which is acceptable for your deployment scenario but worth noting for future maintainers:

// Rollback reverts object_type canonicalization. Note: this transforms all
// rows with canonical values (e.g., 'responses'), including any that may have
// had canonical values prior to this migration. This is acceptable given that
// old formats were only used during development.
Rollback: func(tx *gorm.DB) error {
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c67e977 and 1fb2e03.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (2 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (6)
  • transports/version
  • plugins/governance/version
  • plugins/otel/changelog.md
  • plugins/jsonparser/version
  • plugins/mocker/version
  • plugins/mocker/changelog.md
🚧 Files skipped from review as they are similar to previous changes (15)
  • plugins/jsonparser/changelog.md
  • plugins/semanticcache/changelog.md
  • framework/configstore/rdb.go
  • plugins/semanticcache/version
  • plugins/otel/version
  • plugins/logging/changelog.md
  • framework/changelog.md
  • plugins/governance/changelog.md
  • core/version
  • framework/version
  • core/changelog.md
  • plugins/maxim/version
  • framework/logstore/sqlite.go
  • plugins/maxim/changelog.md
  • framework/configstore/store.go
🧰 Additional context used
🧬 Code graph analysis (1)
framework/logstore/migrations.go (1)
framework/migrator/migrator.go (4)
  • Options (46-59)
  • DefaultOptions (100-106)
  • New (131-149)
  • Migration (62-69)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (7)
plugins/telemetry/version (1)

1-1: Version bump looks good.

Telemetry plugin metadata cleanly reflects the 1.3.6 release; no further action needed.

plugins/telemetry/changelog.md (1)

4-5: LGTM!

The changelog accurately documents the dependency upgrades and the latency metric fix.

framework/configstore/migrations.go (2)

7-7: LGTM!

The migrator import path update reflects the package relocation to a shared location, improving code organization.

46-46: Good migration ID rename to prevent collisions.

Changing the ID from "init" to "configstore_init" prevents potential migration ID collisions with other modules (e.g., logstore) that might also use a generic "init" identifier.

transports/changelog.md (1)

4-8: LGTM!

The changelog comprehensively documents dependency upgrades, new features (responses streaming, raw response support), and fixes (Bedrock streaming, latency metric).

framework/logstore/migrations.go (2)

12-16: Excellent fix for the migration ID collision issue.

Using a dedicated logstore_migrations table name prevents collisions with configstore's migration tracking. This directly addresses the critical issue from previous reviews.

31-60: Migration follows established pattern.

The migration ID "logstore_init" prevents collision with configstore's "configstore_init". The rollback implementation (unconditional table drop) follows the same pattern used in framework/configstore/migrations.go and is appropriate for an init migration given your team's operational practices.

akshaydeo
akshaydeo reviewed Oct 14, 2025
View reviewed changes
akshaydeo
akshaydeo reviewed Oct 14, 2025
View reviewed changes
@TejasGhatte TejasGhatte force-pushed the 10-13-feat_added_migrator_to_log_store branch from 1fb2e03 to 03e32c2 Compare October 14, 2025 10:46
coderabbitai[bot]
coderabbitai bot requested changes Oct 14, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (2)
docs/changelogs/v1.3.0-prerelease6.mdx (2)

28-28: Update Framework section to core 1.2.6.

This section still shows "core to 1.2.5" while other sections reference core 1.2.6. Please update to maintain version consistency across the changelog.

Apply this diff:

-- Upgrade dependency: core to 1.2.5
+- Upgrade dependency: core to 1.2.6

34-34: Update governance section to core 1.2.6 and framework 1.1.6.

This section still shows "core 1.2.5 and framework 1.1.5" while other sections reference core 1.2.6 and framework 1.1.6. Please update to maintain version consistency.

Apply this diff:

-- Chore: using core 1.2.5 and framework 1.1.5
+- Chore: using core 1.2.6 and framework 1.1.6
♻️ Duplicate comments (2)
plugins/logging/version (1)

1-1: Version bump rationale unclear.

As flagged in the previous review, please verify whether the logging plugin actually depends on the migrator changes described in this PR, and ensure the corresponding changelog entry and go.mod dependencies are synchronized.

plugins/governance/changelog.md (1)

4-4: Ensure go.mod dependencies are synchronized with changelog.

As noted in the previous review, multiple go.mod files still reference core v1.2.5 and framework v1.1.5. Please update all go.mod files to align with the changelog's declared versions (core v1.2.6 and framework v1.1.6).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1fb2e03 and 03e32c2.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (5)
  • framework/version
  • plugins/maxim/version
  • plugins/otel/changelog.md
  • plugins/mocker/version
  • plugins/logging/changelog.md
🚧 Files skipped from review as they are similar to previous changes (13)
  • core/version
  • framework/configstore/rdb.go
  • plugins/jsonparser/changelog.md
  • plugins/governance/version
  • core/changelog.md
  • plugins/semanticcache/version
  • framework/configstore/store.go
  • plugins/telemetry/changelog.md
  • plugins/mocker/changelog.md
  • framework/configstore/migrations.go
  • framework/changelog.md
  • plugins/telemetry/version
  • transports/version
🧰 Additional context used
🧬 Code graph analysis (1)
framework/logstore/migrations.go (1)
framework/migrator/migrator.go (3)
  • New (131-149)
  • DefaultOptions (100-106)
  • Migration (62-69)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (9)
transports/changelog.md (1)

4-8: LGTM! Comprehensive changelog entries.

The changelog clearly documents both the dependency upgrade and the functional changes (streaming features, fixes). The entries are well-structured and informative.

docs/changelogs/v1.3.0-prerelease6.mdx (1)

7-7: Version references are consistent across these sections.

The dependency upgrade entries correctly reflect core 1.2.6 and framework 1.1.6 across Bifrost, jsonparser, logging, maxim, mocker, otel, semanticcache, and telemetry sections.

Also applies to: 40-40, 45-45, 50-50, 55-55, 60-60, 65-65, 70-70

plugins/jsonparser/version (1)

1-1: JSONParser plugin unaffected by migrator/logstore changes; version bump to 1.3.6 is safe.

plugins/otel/version (1)

1-1: Verify plugins/otel version bump
No references to migrator or logstore in plugins/otel. Confirm this bump is purely a transitive dependency upgrade.

plugins/maxim/changelog.md (1)

4-4: Confirm transitive dependency bump only. plugins/maxim has no imports of migrator or logstore; this changelog entry is purely bumping core to v1.2.6 and framework to v1.1.6.

framework/logstore/sqlite.go (1)

25-26: LGTM! Clean migration refactor.

The change correctly delegates schema migrations to the new triggerMigrations function while preserving error handling and context propagation.

framework/logstore/migrations.go (3)

12-21: LGTM! Proper migration orchestration.

The function correctly sequences table creation before data migration and properly propagates errors.

24-53: LGTM! Init migration correctly implemented.

The migration properly:

  • Uses a unique ID (logs_init) to avoid collisions with configstore migrations
  • Checks table existence before creation for idempotency
  • Propagates context through the migration function

Note: The unconditional table drop in rollback (lines 38-46) was previously flagged but explicitly accepted by the maintainers as appropriate for an init migration.

56-130: LGTM! Object type canonicalization correctly implemented.

The migration properly:

  • Converts dot notation to underscore notation (e.g., chat.completion → chat_completion)
  • Uses 1-to-1 mappings ensuring accurate rollback (previous lossy mapping concerns have been resolved)
  • Filters updates via WHERE clause for efficiency
  • Wraps errors with helpful context

The previous concerns about rollback affecting pre-existing canonical values were acknowledged by the maintainers as not applicable to their deployment context.

@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 10-07-feat_responses_added_to_logs branch from a6bdc87 to e3b13ec Compare October 14, 2025 12:40
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 10-13-feat_added_migrator_to_log_store branch from 03e32c2 to 31e5b83 Compare October 14, 2025 12:40
coderabbitai[bot]
coderabbitai bot reviewed Oct 14, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
docs/changelogs/v1.3.0-prerelease6.mdx (1)

28-35: Fix inconsistent dependency versions.
Framework/governance still cite core 1.2.5 / framework 1.1.5 while Bifrost + other sections call out 1.2.6 / 1.1.6. Please align the remaining entries (or add an explicit note if the older versions are intentional) so the prerelease changelog tells a single story.

- - Upgrade dependency: core to 1.2.5
+ - Upgrade dependency: core to 1.2.6
...
- - Chore: using core 1.2.5 and framework 1.1.5
+ - Chore: using core 1.2.6 and framework 1.1.6
plugins/jsonparser/changelog.md (1)

4-4: Verify dependency versions match across module files.

The past review comment indicated that plugins/jsonparser/go.mod still required core v1.2.5 and needed updating to v1.2.6. Please confirm this update was completed so the changelog entry matches the actual dependency declarations.

🧹 Nitpick comments (1)
framework/logstore/migrations.go (1)

55-130: LGTM! Object type canonicalization is correctly implemented.

The migration properly converts dotted object types to underscore format with one-to-one invertible mappings. Rollback accurately reverses the forward migration. The WHERE clauses correctly scope updates to only affected rows, and error handling properly wraps failures with context.

Optional: Consider extracting the object type list to reduce duplication.

The WHERE clause values are duplicated between the forward migration (lines 77-82) and rollback (lines 110-114). If you anticipate adding more object type transformations, consider defining the list once as a constant or deriving it programmatically to reduce maintenance burden.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 03e32c2 and 31e5b83.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (4)
  • plugins/mocker/version
  • plugins/otel/changelog.md
  • plugins/otel/version
  • plugins/telemetry/version
🚧 Files skipped from review as they are similar to previous changes (15)
  • plugins/maxim/version
  • framework/configstore/migrations.go
  • plugins/governance/version
  • plugins/telemetry/changelog.md
  • framework/configstore/store.go
  • plugins/logging/version
  • plugins/semanticcache/version
  • transports/version
  • framework/configstore/rdb.go
  • plugins/maxim/changelog.md
  • plugins/mocker/changelog.md
  • plugins/semanticcache/changelog.md
  • framework/version
  • plugins/jsonparser/version
  • plugins/governance/changelog.md
🔇 Additional comments (8)
core/version (1)

1-1: LGTM!

Version bump is consistent with the PR objectives and changelog entries across the repository.

plugins/logging/changelog.md (1)

4-4: LGTM!

Dependency version updates are consistent with the broader version bump across the repository.

framework/changelog.md (1)

4-5: LGTM!

The changelog entries accurately describe the dependency upgrade and the migrator package relocation with logstore migrations feature addition.

transports/changelog.md (1)

4-8: LGTM!

The changelog entries clearly document the dependency upgrade and the new streaming features and fixes.

core/changelog.md (1)

4-6: LGTM!

The changelog entries properly document the streaming features and fixes.

framework/logstore/sqlite.go (1)

25-27: LGTM! Migration to explicit migration workflow.

Replacing AutoMigrate with triggerMigrations is a good practice. AutoMigrate only adds missing columns/tables and won't modify existing column types or drop columns, whereas explicit migrations provide full schema control and rollback capability.

Based on learnings.

framework/logstore/migrations.go (2)

11-21: LGTM! Clean migration orchestration.

The function correctly sequences the initialization and data migration steps with proper error propagation.

23-53: LGTM! Init migration correctly implemented.

The migration ID "logs_init" avoids collisions with configstore migrations. The unconditional table drop in Rollback was discussed and accepted by the team for init migrations, as documented in past review comments.

coderabbitai[bot]
coderabbitai bot requested changes Oct 14, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/changelogs/v1.3.0-prerelease6.mdx (1)

28-31: Fix version inconsistencies and align telemetry entry

  • Update Framework and governance sections to core 1.2.6/framework 1.1.6.
  • Align telemetry line with plugin changelog (it’s a Fix), and hyphenate “inter‑token”.

Suggested edits:

- - Upgrade dependency: core to 1.2.5
+ - Upgrade dependency: core to 1.2.6

- - Chore: using core 1.2.5 and framework 1.1.5
+ - Chore: using core 1.2.6 and framework 1.1.6

- - Feat: Added First Token and Inter Token latency metrics for streaming requests.
+ - Fix: Removed last token's accumulated latency from inter-token latency metric.

Also applies to: 34-37, 70-72

♻️ Duplicate comments (4)
plugins/otel/changelog.md (1)

4-4: Add version header and release links.

This entry still lacks the version/date header and links to the core/framework release notes that were requested earlier.

plugins/logging/version (1)

1-1: Verify that dependency updates and changelog were addressed per past review.

The patch version bump (1.3.5 → 1.3.6) is semantically appropriate for dependency updates. However, the previous review correctly identified that plugins/logging/go.mod should reference core v1.2.6 and framework v1.1.6, and a changelog entry should document this bump.

Please confirm:

  1. plugins/logging/go.mod has been updated to require the new dependency versions
  2. A changelog entry explains this version bump
  3. If the logging plugin doesn't directly use migrator functionality, clarify whether this bump is purely for transitive dependency alignment
#!/bin/bash
# Verify go.mod dependencies and changelog updates

echo "=== Checking plugins/logging/go.mod for updated dependencies ==="
grep -E "(github.com/maximhq/bifrost/core|github.com/maximhq/bifrost/framework)" plugins/logging/go.mod || echo "Dependencies not found in go.mod"

echo -e "\n=== Checking for changelog entry mentioning 1.3.6 ==="
fd -t f -e md "CHANGELOG|changelog" plugins/logging/ --exec grep -l "1.3.6" {} || echo "No changelog entry found for 1.3.6"
plugins/semanticcache/changelog.md (1)

4-4: Changelog/go.mod alignment check

This bump implies core 1.2.6/framework 1.1.6. Ensure plugins/semanticcache/go.mod matches; otherwise update go.mod or correct this entry.

Run:

#!/bin/bash
rg -nP -C2 --glob 'plugins/semanticcache/go.mod' 'core|framework'
plugins/governance/changelog.md (1)

4-4: LGTM; verify go.mod matches

Changelog looks good. Please confirm all go.mod files reference core 1.2.6/framework 1.1.6 to avoid drift.

Script:

#!/bin/bash
rg -nP -g 'go.mod' 'core\s+v1\.2\.[0-6]|framework\s+v1\.1\.[0-6]' -C2
🧹 Nitpick comments (5)
plugins/telemetry/changelog.md (1)

5-5: Hyphenate “inter‑token”

Use “inter-token latency” for clarity.

Apply:

-- Fix: Removed last token's accumulated latency from inter token latency metric.
+- Fix: Removed last token's accumulated latency from inter-token latency metric.
framework/configstore/migrations.go (1)

49-50: Avoid name shadowing with imported package “migrator”

Local variable named “migrator := tx.Migrator()” shadows the imported package name, harming readability. Consider renaming to gormMigrator or gm.

Example:

- migrator := tx.Migrator()
- if !migrator.HasTable(&TableConfigHash{}) {
-   if err := migrator.CreateTable(&TableConfigHash{}); err != nil {
+ gm := tx.Migrator()
+ if !gm.HasTable(&TableConfigHash{}) {
+   if err := gm.CreateTable(&TableConfigHash{}); err != nil {
     return err
   }
 }

Also applies to: 79-83, 88-99

transports/changelog.md (1)

8-8: Hyphenate “inter‑token”

Use “inter-token latency”.

- - Removed last token's accumulated latency from inter token latency metric.
+ - Removed last token's accumulated latency from inter-token latency metric.
framework/logstore/migrations.go (2)

11-13: Comment/function name mismatch

Comment says “Migrate…” but function is triggerMigrations. Align the comment.

- // Migrate performs the necessary database migrations.
+ // triggerMigrations performs the necessary database migrations.

25-53: Init rollback drops table; consider making it non‑rollbackable

Dropping logs on rollback can destroy pre‑existing data. If acceptable per policy, keep; else set Rollback to nil to avoid accidental data loss.

-   Rollback: func(tx *gorm.DB) error {
-     tx = tx.WithContext(ctx)
-     migrator := tx.Migrator()
-     if err := migrator.DropTable(&Log{}); err != nil {
-       return err
-     }
-     return nil
-   },
+   Rollback: nil,

Confirm whether production environments ever perform down-migrations; if yes, prefer the safer approach above.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 03e32c2 and 31e5b83.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (8)
  • plugins/otel/version
  • plugins/telemetry/version
  • framework/version
  • core/version
  • transports/version
  • plugins/jsonparser/changelog.md
  • plugins/semanticcache/version
  • plugins/maxim/version
🚧 Files skipped from review as they are similar to previous changes (9)
  • framework/configstore/store.go
  • plugins/governance/version
  • plugins/logging/changelog.md
  • plugins/jsonparser/version
  • plugins/mocker/version
  • framework/configstore/rdb.go
  • framework/changelog.md
  • plugins/mocker/changelog.md
  • plugins/maxim/changelog.md
🧰 Additional context used
🧬 Code graph analysis (1)
framework/logstore/migrations.go (1)
framework/migrator/migrator.go (3)
  • New (131-149)
  • DefaultOptions (100-106)
  • Migration (62-69)
🪛 LanguageTool
plugins/telemetry/changelog.md

[grammar] ~5-~5: Use a hyphen to join words.
Context: ...t token's accumulated latency from inter token latency metric.

(QB_NEW_EN_HYPHEN)

[grammar] ~5-~5: There might be a mistake here.
Context: ...latency from inter token latency metric.

(QB_NEW_EN)

transports/changelog.md

[grammar] ~5-~5: There might be a mistake here.
Context: ...esponses streaming across all providers. - Fixed bedrock chat streaming decoding is...

(QB_NEW_EN)

[grammar] ~8-~8: Use a hyphen to join words.
Context: ...t token's accumulated latency from inter token latency metric.

(QB_NEW_EN_HYPHEN)

[grammar] ~8-~8: There might be a mistake here.
Context: ...latency from inter token latency metric.

(QB_NEW_EN)

⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (3)
framework/configstore/migrations.go (1)

7-7: Import path update looks correct

Migrator moved to framework/migrator; import is accurate.

core/changelog.md (1)

4-6: LGTM

Entries are clear and scoped to streaming improvements.

framework/logstore/migrations.go (1)

62-83: Mapping set looks sane and idempotent

Coalesced/ambiguous cases removed; UPDATE is scoped and idempotent. Good.

Optionally add a unit test covering each mapping pair to prevent regressions.

@TejasGhatte TejasGhatte force-pushed the 10-13-feat_added_migrator_to_log_store branch from 31e5b83 to 3f250ff Compare October 14, 2025 14:32
coderabbitai[bot]
coderabbitai bot reviewed Oct 14, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (3)
plugins/semanticcache/changelog.md (1)

4-4: Ensure go.mod matches the changelog versions.

The changelog entry references core v1.2.6 and framework v1.1.6, but this concern has already been flagged in a previous review regarding the mismatch with go.mod versions.

plugins/governance/changelog.md (1)

4-4: Verify go.mod alignment with changelog versions.

This changelog references core v1.2.6 and framework v1.1.6, but the same concern about go.mod mismatches has already been raised in a previous review.

framework/logstore/sqlite.go (1)

25-28: Integration of migration system looks correct.

The replacement of AutoMigrate with triggerMigrations provides better control over schema evolution. The context is properly passed and error handling is preserved.

However, the previous review comment regarding the need for tests for the migration logic (triggerMigrations, migrationInit, migrationUpdateObjectColumnValues) remains valid and should be addressed.

🧹 Nitpick comments (1)
plugins/telemetry/changelog.md (1)

5-5: Consider hyphenating compound adjective.

"inter token latency" should be "inter-token latency" for correct English grammar when used as a compound adjective.

Apply this diff:

-- Fix: Removed last token's accumulated latency from inter token latency metric.
+- Fix: Removed last token's accumulated latency from inter-token latency metric.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 31e5b83 and 3f250ff.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (4)
  • plugins/otel/version
  • core/version
  • plugins/semanticcache/version
  • plugins/mocker/version
🚧 Files skipped from review as they are similar to previous changes (15)
  • transports/version
  • plugins/jsonparser/changelog.md
  • framework/configstore/migrations.go
  • framework/configstore/store.go
  • framework/configstore/rdb.go
  • framework/logstore/migrations.go
  • plugins/otel/changelog.md
  • framework/version
  • plugins/maxim/version
  • docs/changelogs/v1.3.0-prerelease6.mdx
  • plugins/logging/version
  • plugins/maxim/changelog.md
  • plugins/governance/version
  • plugins/jsonparser/version
  • plugins/telemetry/version
🧰 Additional context used
🪛 LanguageTool
plugins/governance/changelog.md

[grammar] ~4-~4: There might be a mistake here.
Context: ...re: using core 1.2.6 and framework 1.1.6

(QB_NEW_EN)

plugins/telemetry/changelog.md

[grammar] ~5-~5: Use a hyphen to join words.
Context: ...t token's accumulated latency from inter token latency metric.

(QB_NEW_EN_HYPHEN)

[grammar] ~5-~5: There might be a mistake here.
Context: ...latency from inter token latency metric.

(QB_NEW_EN)

transports/changelog.md

[grammar] ~5-~5: There might be a mistake here.
Context: ...esponses streaming across all providers. - Fixed bedrock chat streaming decoding is...

(QB_NEW_EN)

[grammar] ~8-~8: There might be a mistake here.
Context: ...e support for all streaming requests. - Removed last token's accumulated latency from i...

(QB_NEW_EN)

[grammar] ~8-~8: There might be a mistake here.
Context: ...latency from inter token latency metric.

(QB_NEW_EN)

⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (5)
plugins/logging/changelog.md (1)

4-4: Changelog entry aligns with PR objectives.

The dependency version bumps to core 1.2.6 and framework 1.1.6 are consistent with the migrator relocation and logstore migration features introduced in this PR.

plugins/telemetry/changelog.md (1)

4-4: Version bump looks good.

The dependency upgrade to core 1.2.6 and framework 1.1.6 is consistent with the broader PR changes.

plugins/mocker/changelog.md (1)

4-4: Changelog entry is consistent with PR changes.

The dependency version bump aligns with the framework and core upgrades introduced in this PR.

framework/changelog.md (1)

4-5: Changelog accurately reflects the PR changes.

The entries correctly document the core version upgrade and the migration infrastructure improvements (relocating the migrator package and introducing logstore migrations to standardize object_type values).

core/changelog.md (1)

4-6: Changelog entries clearly document new features.

The three entries appropriately describe the streaming enhancements and fixes: response streaming across all providers, Bedrock decoding fixes, and raw response support for streaming requests.

@TejasGhatte TejasGhatte changed the base branch from 10-07-feat_responses_added_to_logs to graphite-base/612 October 14, 2025 14:54
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 10-13-feat_added_migrator_to_log_store branch from 3f250ff to fe71707 Compare October 14, 2025 16:09
@Pratham-Mishra04 Pratham-Mishra04 changed the base branch from graphite-base/612 to main October 14, 2025 16:09
@Pratham-Mishra04 Graphite App
Copy link
Collaborator

Pratham-Mishra04 commented Oct 14, 2025 •
edited
Loading

Merge activity

  • Oct 14, 4:11 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Oct 14, 4:12 PM UTC: @Pratham-Mishra04 merged this pull request with Graphite.

@Pratham-Mishra04 Pratham-Mishra04 merged commit 61944ba into main Oct 14, 2025
3 of 4 checks passed
@Pratham-Mishra04 Pratham-Mishra04 deleted the 10-13-feat_added_migrator_to_log_store branch October 14, 2025 16:12
coderabbitai[bot]
coderabbitai bot requested changes Oct 14, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
plugins/semanticcache/changelog.md (1)

4-4: Confirm changelog aligns with plugins/semanticcache/go.mod.

Earlier review flagged a mismatch. Please verify the go.mod requires actually point to core 1.2.6 and framework 1.1.6; adjust either the changelog or go.mod accordingly.

#!/usr/bin/env bash
set -euo pipefail
modfile="$(fd -a '^go\.mod$' plugins/semanticcache | head -n1 || true)"
if [[ -z "${modfile:-}" ]]; then
  echo "plugins/semanticcache/go.mod not found"; exit 0
fi
echo "Scanning $modfile"
echo "Requires:"
rg -nP '^\s*require\s+(github\.com/maximhq/bifrost/(core|framework))\s+v?([\d\.]+)' "$modfile" -C0 || true
echo
echo "Replaces (if any):"
rg -nP '^\s*replace\s+(github\.com/maximhq/bifrost/(core|framework))\s+=>\s+.+\s(v?[\d\.]+)?' "$modfile" -C0 || true
🧹 Nitpick comments (5)
plugins/governance/changelog.md (1)

4-4: Polish the changelog wording

Minor wording tweak for consistency with our usual “Chore: bump …” phrasing.

- Chore: using core 1.2.6 and framework 1.1.6
+ Chore: bump core to 1.2.6 and framework to 1.1.6
plugins/telemetry/changelog.md (1)

5-5: Hyphenate “inter-token” and add article for clarity.

-- Fix: Removed last token's accumulated latency from inter token latency metric.
+- Fix: Removed last token's accumulated latency from the inter-token latency metric.
transports/changelog.md (1)

5-8: Polish wording and hyphenation.

-- Added Responses streaming across all providers.
+- Added response streaming across all providers.
-- Fixed bedrock chat streaming decoding issues.
+- Fixed Bedrock chat streaming decoding issues.
-- Added raw response support for all streaming requests.
+- Added raw-response support for all streaming requests.
-- Removed last token's accumulated latency from inter token latency metric.
+- Removed last token's accumulated latency from the inter-token latency metric.
docs/changelogs/v1.3.0-prerelease6.mdx (1)

28-28: Align versions and telemetry note with plugin changelog.

Make these consistent with the top-level bump and plugins/telemetry:

- - Upgrade dependency: core to 1.2.5
+ - Upgrade dependency: core to 1.2.6 and framework to 1.1.6
- - Chore: using core 1.2.5 and framework 1.1.5
+ - Chore: using core 1.2.6 and framework 1.1.6
- - Feat: Added First Token and Inter Token latency metrics for streaming requests.
+ - Fix: Removed last token's accumulated latency from the inter-token latency metric.

Also applies to: 34-34, 71-71

framework/logstore/migrations.go (1)

11-12: Tidy-ups: avoid shadowing, wrap errors with %w, and fix comment wording.

  • Rename local variable to avoid shadowing the imported package name.
  • Use %w for error wrapping (consistent with later code).
  • Update comment to reflect function name.
-// Migrate performs the necessary database migrations.
+// triggerMigrations runs the necessary database migrations.
 func triggerMigrations(ctx context.Context, db *gorm.DB) error {
-			migrator := tx.Migrator()
-			if !migrator.HasTable(&Log{}) {
-				if err := migrator.CreateTable(&Log{}); err != nil {
+			gm := tx.Migrator()
+			if !gm.HasTable(&Log{}) {
+				if err := gm.CreateTable(&Log{}); err != nil {
 					return err
 				}
 			}
-			migrator := tx.Migrator()
+			gm := tx.Migrator()
 			// Drop children first, then parents (adjust if your actual FKs differ)
-			if err := migrator.DropTable(&Log{}); err != nil {
+			if err := gm.DropTable(&Log{}); err != nil {
 				return err
 			}
-	return fmt.Errorf("error while running db migration: %s", err.Error())
+	return fmt.Errorf("error while running db migration: %w", err)
-	return fmt.Errorf("error while running object column migration: %s", err.Error())
+	return fmt.Errorf("error while running object column migration: %w", err)

Also applies to: 29-34, 41-44, 50-50, 129-129

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3f250ff and fe71707.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (3)
  • plugins/maxim/version
  • plugins/logging/version
  • plugins/otel/changelog.md
🚧 Files skipped from review as they are similar to previous changes (15)
  • transports/version
  • framework/logstore/sqlite.go
  • framework/configstore/migrations.go
  • plugins/governance/version
  • plugins/telemetry/version
  • plugins/mocker/changelog.md
  • plugins/semanticcache/version
  • core/changelog.md
  • plugins/jsonparser/changelog.md
  • framework/configstore/rdb.go
  • framework/version
  • plugins/maxim/changelog.md
  • framework/changelog.md
  • plugins/mocker/version
  • core/version
🧰 Additional context used
🧬 Code graph analysis (1)
framework/logstore/migrations.go (1)
framework/migrator/migrator.go (3)
  • New (131-149)
  • DefaultOptions (100-106)
  • Migration (62-69)
🪛 LanguageTool
transports/changelog.md

[grammar] ~5-~5: There might be a mistake here.
Context: ...esponses streaming across all providers. - Fixed bedrock chat streaming decoding is...

(QB_NEW_EN)

[grammar] ~8-~8: There might be a mistake here.
Context: ...e support for all streaming requests. - Removed last token's accumulated latency from i...

(QB_NEW_EN)

[grammar] ~8-~8: There might be a mistake here.
Context: ...latency from inter token latency metric.

(QB_NEW_EN)

plugins/governance/changelog.md

[grammar] ~4-~4: There might be a mistake here.
Context: ...re: using core 1.2.6 and framework 1.1.6

(QB_NEW_EN)

plugins/telemetry/changelog.md

[grammar] ~5-~5: Use a hyphen to join words.
Context: ...t token's accumulated latency from inter token latency metric.

(QB_NEW_EN_HYPHEN)

[grammar] ~5-~5: There might be a mistake here.
Context: ...latency from inter token latency metric.

(QB_NEW_EN)

🔇 Additional comments (2)
plugins/otel/version (1)

1-1: Version bump looks good.

framework/configstore/store.go (1)

9-9: Import path update to framework/migrator looks correct.

plugins/jsonparser/version
@@ -1 +1 @@
1.3.5 No newline at end of file
1.3.6 No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify the necessity of this version bump.

The patch version bump from 1.3.5 to 1.3.6 appears unrelated to the main PR changes (migrator refactoring and logstore migrations). While the AI summary mentions this is part of broader version bumps across modules, it's unclear whether the jsonparser plugin has any actual changes that warrant a version bump.

Please confirm:

  1. Are there actual changes to the jsonparser plugin that require this version bump?
  2. If this is a coordinated release, is there a changelog or release notes documenting what changed in version 1.3.6?
  3. Have all dependent modules been updated to reference the new version if needed?

🏁 Script executed:

#!/bin/bash
# Description: Check for actual code changes in the jsonparser plugin and look for changelog updates

# Search for jsonparser plugin files to see if there are code changes
fd -e go -p 'plugins/jsonparser' --exec echo "File: {}"

# Look for changelog or version-related documentation
fd -e md -p 'plugins/jsonparser' | head -20

# Check if other files reference the jsonparser version
rg -n 'jsonparser.*1\.3\.[56]'

Length of output: 108

🏁 Script executed:

#!/bin/bash
# Verify changelog entry for version 1.3.6
grep -nE '^##\s*1\.3\.6' plugins/jsonparser/changelog.md || echo "No entry for 1.3.6"

# Show diff for plugin source files to see actual changes
git diff origin/main...HEAD -- plugins/jsonparser/main.go plugins/jsonparser/plugin_test.go

Length of output: 18

Document or revert jsonparser patch bump The patch bump to 1.3.6 in plugins/jsonparser/version has no corresponding code changes and no entry in plugins/jsonparser/changelog.md. Add release notes for v1.3.6 or revert this bump.

🤖 Prompt for AI Agents 
In plugins/jsonparser/version around lines 1 to 1, the version was bumped to
1.3.6 but there are no corresponding code changes or an entry in
plugins/jsonparser/changelog.md; either add a changelog entry describing the
1.3.6 changes and any relevant release notes to plugins/jsonparser/changelog.md
(including date and concise bullet points) and keep the version bump, or revert
plugins/jsonparser/version back to the previous version number so the repository
state matches existing changes; commit the chosen fix with a clear message
(e.g., "chore(jsonparser): add changelog for v1.3.6" or "chore(jsonparser):
revert accidental v1.3.6 bump").

coderabbitai[bot]
coderabbitai bot requested changes Oct 14, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (4)
plugins/logging/version (1)

1-1: Verify go.mod dependencies match the bumped core and framework versions.

The patch version increment (1.3.5 → 1.3.6) aligns with coordinated dependency updates across the project. However, as flagged in the previous review, please confirm that plugins/logging/go.mod has been updated to require:

  • github.com/maximhq/bifrost/core v1.2.6
  • github.com/maximhq/bifrost/framework v1.1.6

Also verify that a changelog entry for v1.3.6 exists in plugins/logging/CHANGELOG.md.

Run the following script to verify the dependency versions and changelog:

#!/bin/bash
# Description: Verify go.mod dependencies and changelog for logging plugin v1.3.6

# Check go.mod for core and framework versions
echo "=== plugins/logging/go.mod dependencies ==="
grep -E "github.com/maximhq/bifrost/(core|framework)" plugins/logging/go.mod || echo "Dependencies not found in go.mod"

# Check for changelog entry for v1.3.6
echo -e "\n=== Changelog entry for v1.3.6 ==="
rg -n "1\.3\.6" plugins/logging/CHANGELOG.md -A 5 || echo "No changelog entry found for v1.3.6"

# Verify core and framework version files
echo -e "\n=== Core version ==="
cat core/version 2>/dev/null || echo "core/version not found"

echo -e "\n=== Framework version ==="
cat framework/version 2>/dev/null || echo "framework/version not found"
plugins/telemetry/changelog.md (1)

4-4: Confirm go.mod bumps align with this changelog

Verify all modules require core v1.2.6 and framework v1.1.6; flag any stale refs.

#!/bin/bash
set -euo pipefail

echo "Listing go.mod files:"
fd -a -t f go.mod

echo -e "\nExpect core v1.2.6 and framework v1.1.6 occurrences:"
rg -nP -C1 -g 'go.mod' 'github.com/maximhq/bifrost/(core|framework)\s+v(1\.2\.6|1\.1\.6)\b' || true

echo -e "\nStale core refs (v1.2.0–v1.2.5):"
rg -nP -g 'go.mod' 'github.com/maximhq/bifrost/core\s+v1\.2\.[0-5]\b' || echo "→ None"

echo -e "\nStale framework refs (v1.1.0–v1.1.5):"
rg -nP -g 'go.mod' 'github.com/maximhq/bifrost/framework\s+v1\.1\.[0-5]\b' || echo "→ None"
framework/logstore/migrations.go (2)

38-46: Unconditional table drop on rollback is risky; acknowledged in prior discussion.

This was previously flagged and discussed; leaving as-is per maintainer decision for initial migration rollback.

If policy changes later, consider removing rollback or making it conditional on having created the table.

96-117: Rollback mapping may touch pre-existing canonical rows; acknowledged earlier.

Best-effort rollback could convert rows that were already canonical before migration. Not re-litigating since this was discussed.

🧹 Nitpick comments (8)
plugins/telemetry/changelog.md (1)

5-5: Hyphenation and wording nit

Prefer “the” and “inter-token”.

-- Fix: Removed last token's accumulated latency from inter token latency metric.
+- Fix: Removed the last token's accumulated latency from the inter-token latency metric.
plugins/governance/changelog.md (1)

4-4: Wording nit

Prefer “bump dependencies … to …” for clarity.

-- Chore: using core 1.2.6 and framework 1.1.6
+- Chore: bump dependencies: core to 1.2.6 and framework to 1.1.6
transports/changelog.md (1)

5-8: Tighten phrasing and capitalization.

Minor grammar/casing fixes for clarity.

Apply this diff:

- - Added Responses streaming across all providers.
- - Fixed bedrock chat streaming decoding issues.
+ - Added response streaming across all providers.
+ - Fixed Bedrock chat streaming decoding issues.
  - Added raw response support for all streaming requests.
- - Removed last token's accumulated latency from inter token latency metric.
+ - Removed last token's accumulated latency from the inter-token latency metric.
framework/logstore/migrations.go (5)

11-12: Align comment with function name.

-// Migrate performs the necessary database migrations.
+// triggerMigrations performs the necessary database migrations for the log store.

25-37: Enable a transaction for the init migration and avoid shadowing the package name.

Wrap the init migration in a transaction for safety and rename the local variable to avoid shadowing the migrator import.

-func migrationInit(ctx context.Context, db *gorm.DB) error {
-	m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{
+func migrationInit(ctx context.Context, db *gorm.DB) error {
+	opts := *migrator.DefaultOptions
+	opts.UseTransaction = true
+	m := migrator.New(db, &opts, []*migrator.Migration{{
 		ID: "logs_init",
 		Migrate: func(tx *gorm.DB) error {
 			tx = tx.WithContext(ctx)
-			migrator := tx.Migrator()
-			if !migrator.HasTable(&Log{}) {
-				if err := migrator.CreateTable(&Log{}); err != nil {
+			dbMigrator := tx.Migrator()
+			if !dbMigrator.HasTable(&Log{}) {
+				if err := dbMigrator.CreateTable(&Log{}); err != nil {
 					return err
 				}
 			}
 
 			return nil
 		},

48-52: Wrap errors with %w for better propagation.

-	err := m.Migrate()
-	if err != nil {
-		return fmt.Errorf("error while running db migration: %s", err.Error())
-	}
+	if err := m.Migrate(); err != nil {
+		return fmt.Errorf("error while running db migration: %w", err)
+	}

64-86: Mapping looks idempotent and scoped.

The WHERE predicate ensures no-op on re-run. Consider indexing logs.object_type separately if queries rely on it heavily; not required for this migration.

127-130: Use %w here as well for consistency.

-	err := m.Migrate()
-	if err != nil {
-		return fmt.Errorf("error while running object column migration: %s", err.Error())
-	}
+	if err := m.Migrate(); err != nil {
+		return fmt.Errorf("error while running object column migration: %w", err)
+	}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3f250ff and fe71707.

📒 Files selected for processing (28)
  • core/changelog.md (1 hunks)
  • core/version (1 hunks)
  • docs/changelogs/v1.3.0-prerelease6.mdx (2 hunks)
  • framework/changelog.md (1 hunks)
  • framework/configstore/migrations.go (1 hunks)
  • framework/configstore/rdb.go (1 hunks)
  • framework/configstore/store.go (1 hunks)
  • framework/logstore/migrations.go (1 hunks)
  • framework/logstore/sqlite.go (1 hunks)
  • framework/version (1 hunks)
  • plugins/governance/changelog.md (1 hunks)
  • plugins/governance/version (1 hunks)
  • plugins/jsonparser/changelog.md (1 hunks)
  • plugins/jsonparser/version (1 hunks)
  • plugins/logging/changelog.md (1 hunks)
  • plugins/logging/version (1 hunks)
  • plugins/maxim/changelog.md (1 hunks)
  • plugins/maxim/version (1 hunks)
  • plugins/mocker/changelog.md (1 hunks)
  • plugins/mocker/version (1 hunks)
  • plugins/otel/changelog.md (1 hunks)
  • plugins/otel/version (1 hunks)
  • plugins/semanticcache/changelog.md (1 hunks)
  • plugins/semanticcache/version (1 hunks)
  • plugins/telemetry/changelog.md (1 hunks)
  • plugins/telemetry/version (1 hunks)
  • transports/changelog.md (1 hunks)
  • transports/version (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • plugins/telemetry/version
  • core/changelog.md
🚧 Files skipped from review as they are similar to previous changes (14)
  • plugins/governance/version
  • plugins/otel/version
  • framework/version
  • plugins/semanticcache/changelog.md
  • plugins/semanticcache/version
  • plugins/otel/changelog.md
  • framework/configstore/rdb.go
  • framework/configstore/migrations.go
  • plugins/jsonparser/changelog.md
  • plugins/maxim/changelog.md
  • plugins/mocker/changelog.md
  • plugins/maxim/version
  • core/version
  • docs/changelogs/v1.3.0-prerelease6.mdx
🧰 Additional context used
🧬 Code graph analysis (1)
framework/logstore/migrations.go (1)
framework/migrator/migrator.go (3)
  • New (131-149)
  • DefaultOptions (100-106)
  • Migration (62-69)
🪛 LanguageTool
transports/changelog.md

[grammar] ~5-~5: There might be a mistake here.
Context: ...esponses streaming across all providers. - Fixed bedrock chat streaming decoding is...

(QB_NEW_EN)

[grammar] ~8-~8: There might be a mistake here.
Context: ...e support for all streaming requests. - Removed last token's accumulated latency from i...

(QB_NEW_EN)

[grammar] ~8-~8: There might be a mistake here.
Context: ...latency from inter token latency metric.

(QB_NEW_EN)

plugins/governance/changelog.md

[grammar] ~4-~4: There might be a mistake here.
Context: ...re: using core 1.2.6 and framework 1.1.6

(QB_NEW_EN)

plugins/telemetry/changelog.md

[grammar] ~5-~5: Use a hyphen to join words.
Context: ...t token's accumulated latency from inter token latency metric.

(QB_NEW_EN_HYPHEN)

[grammar] ~5-~5: There might be a mistake here.
Context: ...latency from inter token latency metric.

(QB_NEW_EN)

🔇 Additional comments (7)
transports/version (1)

1-1: Version bump looks good

No functional impact here.

plugins/mocker/version (1)

1-1: Version bump looks good

No functional impact here.

framework/configstore/store.go (1)

9-9: Import path updated; no stale references
All instances of the old framework/configstore/migrator import have been removed.

plugins/logging/changelog.md (1)

4-4: Changelog entry looks good.

Version bumps are clear and scoped. No further edits needed.

framework/changelog.md (1)

4-5: Changelog matches the code changes.

Dependency bump + migration note accurately reflect the PR.

framework/logstore/sqlite.go (1)

25-28: Switch to explicit migrations is a solid improvement.

Replacing AutoMigrate with triggerMigrations centralizes schema/data evolution. LGTM.

framework/logstore/migrations.go (1)

57-60: Good call enabling a transaction for the data migration.

This prevents partial updates on interruption. LGTM.

plugins/jsonparser/version
@@ -1 +1 @@
1.3.5 No newline at end of file
1.3.6 No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify the necessity of this version bump.

The jsonparser plugin version has been incremented to 1.3.6, but the PR objectives and summary indicate no functional changes to the jsonparser plugin itself. Since this PR focuses on migrator and logstore changes, please confirm:

  1. Does the jsonparser plugin depend on core or framework versions that were updated in this PR (core 1.2.5→1.2.6, framework 1.1.5→1.1.6)?
  2. Is there a corresponding changelog entry documenting why this version was bumped?
  3. Does this follow the project's versioning policy for coordinated releases?

Run the following script to check for dependency updates and changelog entries:

🏁 Script executed:

#!/bin/bash
# Description: Verify jsonparser dependencies and changelog for version 1.3.6

# Check go.mod for core/framework dependencies
if [ -f "plugins/jsonparser/go.mod" ]; then
  echo "=== jsonparser dependencies ==="
  rg -A 2 'github.com/maximhq/bifrost/(core|framework)' plugins/jsonparser/go.mod
fi

# Check for changelog entries
echo "=== Checking for changelog ==="
fd -t f 'CHANGELOG|changelog|HISTORY|history' plugins/jsonparser/

# Check for version references in plugin files
echo "=== Version references in jsonparser ==="
rg -n '1\.3\.[56]' plugins/jsonparser/

Length of output: 228

🏁 Script executed:

#!/bin/bash
# Verify changelog entry for version 1.3.6 in jsonparser plugin

echo "=== Changelog entries around 1.3.6 ==="
rg -n '1\.3\.6' -C2 plugins/jsonparser/changelog.md || echo "No entries found for 1.3.6"

Length of output: 65

Add missing changelog entry and update dependency

  • Document the 1.3.6 bump in plugins/jsonparser/changelog.md explaining its purpose.
  • Update plugins/jsonparser/go.mod to require github.com/maximhq/bifrost/core v1.2.6 (or revert the version bump if unintended).
🤖 Prompt for AI Agents 
In plugins/jsonparser/version around line 1, the version was bumped to 1.3.6 but
there is no changelog entry and the dependency requirement may be inconsistent;
add an entry to plugins/jsonparser/changelog.md documenting the 1.3.6 bump and
its purpose (what changed and why), and update plugins/jsonparser/go.mod to
require github.com/maximhq/bifrost/core v1.2.6 if the dependency change is
intended (or revert the version in plugins/jsonparser/version back to the
previous version if the bump was accidental).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] requested changes

@Pratham-Mishra04 Pratham-Mishra04 Awaiting requested review from Pratham-Mishra04

@danpiths danpiths Awaiting requested review from danpiths

@akshaydeo akshaydeo Awaiting requested review from akshaydeo

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@TejasGhatte @Pratham-Mishra04 @akshaydeo