Skip to content

Conversation

@wenerme
Copy link

@wenerme wenerme commented Dec 12, 2025

Summary

config.json is long, has alot same parts, use yaml can make this easier to write.

Changes

  • What was changed and why
  • Any notable design decisions or trade-offs

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

Describe the steps to validate this change. Include commands and expected outcomes.

# Core/Transports
go version
go test ./...

# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build

If adding new configs or environment variables, document them here.

Screenshots/Recordings

If UI changes, add before/after screenshots or short clips.

Breaking changes

  • Yes
  • No

If yes, describe impact and migration instructions.

Related issues

Link related issues and discussions. Example: Closes #123

Security considerations

Note any security implications (auth, secrets, PII, sandboxing, etc.).

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 12, 2025

📝 Walkthrough

Summary by CodeRabbit

  • New Features
    • Configuration files now support YAML format (.yaml and .yml files) in addition to JSON, providing greater flexibility.
    • System automatically searches for and loads the first available configuration file in order of precedence, with automatic fallback to default settings if no config file is found.

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

The config loading system now supports YAML format alongside JSON, searching for config.json, config.yaml, and config.yml in order and loading the first found. YAML files are parsed and converted to JSON before processing. A YAML dependency is added and tests validate precedence and YAML anchor behavior.

Changes

Cohort / File(s) Summary
Config Loading Enhancement
transports/bifrost-http/lib/config.go
Modified LoadConfig to search for and load config files in priority order: config.json, config.yaml, config.yml. Adds YAML parsing with yaml.v3, converts YAML content to JSON for internal processing, and implements fallback to defaults if no config file is found.
Config YAML Tests
transports/bifrost-http/lib/config_yaml_test.go
New test file with TestLoadConfig_YAML and TestLoadConfig_YAML_Precedence. Validates YAML parsing, YAML anchors/merges, and ensures JSON config takes precedence over YAML when both exist.
Dependency Management
transports/go.mod
Converts gopkg.in/yaml.v3 v3.0.1 from indirect to direct dependency.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • config.go logic: Review YAML parsing, JSON conversion, file precedence handling, and error cases for robustness
  • config_yaml_test.go: Verify test coverage for YAML anchors/merge behavior and precedence scenarios
  • go.mod: Confirm yaml.v3 version compatibility with existing dependencies

Poem

🐰 YAML whispers join JSON's song,
Config files find where they belong,
Precedence clear, anchors align,
The config dance is now sublime! ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR addresses YAML config loading (#1073 objective) but the linked issue #123 concerns File APIs for providers, which is unrelated to this changeset. Either unlink issue #123 or clarify its relationship to YAML config loading, or link the correct issues related to configuration management.
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive The description is incomplete. While it explains the motivation and marks the type of change, it lacks detailed explanation of what was changed and design decisions under the 'Changes' section. Expand the 'Changes' section to describe the implementation details, file format precedence rules, and how YAML anchors/references are handled.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding support for YAML config file formats (config.yaml/config.yml) with reference support.
Out of Scope Changes check ✅ Passed All changes are scoped to YAML configuration loading: config.go updates, new test file, and dependency management for yaml.v3. No out-of-scope changes detected.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

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 (1)
transports/bifrost-http/lib/config.go (1)

253-266: Update function documentation to reflect YAML support.

The function comment references "JSON config file parsing" but the function now supports YAML as well. Consider updating the documentation:

-// LoadConfig loads initial configuration from a JSON config file into memory
+// LoadConfig loads initial configuration from a config file (JSON or YAML) into memory
 // with full preprocessing including environment variable resolution and key config parsing.
 // All processing is done upfront to ensure zero latency when retrieving data.
 //
-// If the config file doesn't exist, the system starts with default configuration
+// If no config file exists, the system starts with default configuration
 // and users can add providers dynamically via the HTTP API.
 //
 // This method handles:
-//   - JSON config file parsing
+//   - Config file parsing (config.json, config.yaml, or config.yml in that order)
+//   - YAML anchor/alias resolution via conversion to JSON
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 675561f and 15d37ec.

📒 Files selected for processing (3)
  • transports/bifrost-http/lib/config.go (2 hunks)
  • transports/bifrost-http/lib/config_yaml_test.go (1 hunks)
  • transports/go.mod (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)

Files:

  • transports/bifrost-http/lib/config.go
  • transports/go.mod
  • transports/bifrost-http/lib/config_yaml_test.go
🧠 Learnings (1)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.

Applied to files:

  • transports/bifrost-http/lib/config.go
  • transports/bifrost-http/lib/config_yaml_test.go
🧬 Code graph analysis (1)
transports/bifrost-http/lib/config_yaml_test.go (2)
transports/bifrost-http/lib/config.go (1)
  • LoadConfig (267-377)
framework/configstore/clientconfig.go (1)
  • ClientConfig (36-50)
🔇 Additional comments (5)
transports/go.mod (1)

21-21: LGTM!

The promotion of gopkg.in/yaml.v3 from indirect to direct dependency is appropriate since it's now explicitly used in config.go for YAML configuration parsing.

transports/bifrost-http/lib/config.go (2)

30-30: LGTM!

The YAML library import is correctly added to support the new configuration format.


287-323: LGTM - Clean implementation of multi-format config loading.

The approach of converting YAML to JSON before processing is well-designed:

  • Properly handles YAML anchors/aliases via yaml.v3
  • Maintains compatibility with existing JSON unmarshaling logic
  • Clear error messages for parse/conversion failures
  • Correct precedence: config.json → config.yaml → config.yml
transports/bifrost-http/lib/config_yaml_test.go (2)

71-111: LGTM - Comprehensive precedence test.

The test validates both critical scenarios:

  1. JSON takes precedence when both files exist (InitialPoolSize = 10)
  2. YAML is used as fallback when JSON is removed (InitialPoolSize = 20)

This aligns well with the documented load order in LoadConfig.


10-69: LGTM - Good coverage of YAML-specific features.

The test effectively validates:

  • Basic YAML parsing and conversion to JSON
  • YAML anchor (&common) and merge key (<<: *common) functionality
  • Inherited values from anchors (key 1 inherits value from anchor)
  • Provider and key structure preservation

The createTempDir helper is properly defined in the test package (config_test.go).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Files API Support

1 participant