feat: add config-based skills registry - #4232
Merged
Merged
Conversation
This was referenced Jun 9, 2026
|
|
18 tasks
Collaborator
Author
This was referenced Jun 9, 2026
Contributor
|
Warning Review limit reached
More reviews will be available in 5 minutes and 59 seconds. Learn how PR review limits work. To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a declarative skills_registry to transport config: schema and structs, object-store wiring and lifecycle, startup reconciliation that creates/updates/skips skills by deterministic config-hash, MIME inference for declared files, and test mock plumbing. ChangesSkills Registry Configuration and Startup Reconciliation
Sequence Diagram(s): sequenceDiagram
participant Client
participant LoadConfig
participant PluginLoader
participant loadSkillsRegistry
participant reconcileOneSkill
participant ConfigStore
participant ObjectStore
Client->>LoadConfig: start boot
LoadConfig->>PluginLoader: load plugins
PluginLoader->>loadSkillsRegistry: trigger skills reconciliation
loadSkillsRegistry->>reconcileOneSkill: per-entry reconcile
reconcileOneSkill->>ConfigStore: GetSkillByName / CreateSkill / UpdateSkill
reconcileOneSkill->>ObjectStore: CreateSkillFileBlob / read/write blobs
loadSkillsRegistry->>LoadConfig: finish reconciliation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Confidence Score: 5/5Safe to merge; the reconciliation logic is correct, the object-store threading is properly wired, and hash-before-network ordering prevents unnecessary startup latency. The change is self-contained startup reconciliation code with no hot-path effects. Object store is correctly threaded through from initStores, hash comparison gates all network I/O, and validation reuses the existing management-API pipeline. The only observation is a substring-match pattern that is fragile to future refactors but does not affect current correctness. config_skills.go — the error-string matching on lines 128-129 is worth watching if validateSkillVersionIncrement error messages are ever reworded. Important Files Changed
Reviews (40): Last reviewed commit: "feat: add config-based skills registry" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/lib/config_skills.go`:
- Line 31: The call to reconcileSkillsRegistry is passing nil for the objStore,
dropping the object-store lifecycle; update the call in loadSkillsRegistry to
pass the initialized object store instead of nil (e.g., pass the local objStore
or the config-provided store) so reconcileSkillsRegistry(ctx,
config.ConfigStore, configData.SkillsRegistry.Skills, objStore) uses the real
object store; ensure any helper that constructs or returns the object store (the
variable named objStore in surrounding code) is used here so persistence and
create/update reconciliation paths are not bypassed.
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 1481-1483: The MockConfigStore.GetSkillByName method currently
returns configstore.ErrNotFound which is inconsistent with other mock getters
(GetSkill, GetSkillLean); update MockConfigStore.GetSkillByName to return the
zero value and nil error (i.e., return nil, nil) so it matches the other mock
methods and keeps test mocks simple and consistent.
In `@transports/bifrost-http/lib/config.go`:
- Around line 4562-4569: The objStore.Ping call can block indefinitely; wrap the
ping in a short derived context using context.WithTimeout (e.g., 3–10s), call
objStore.Ping with that derived ctx, and defer cancel() so the ping enforces a
timeout; keep the existing error handling (close objStore on failure) but return
a timeout-wrapped error if the derived context expires. Reference:
objectstore.NewObjectStore and objStore.Ping.
- Around line 931-933: Gate the call to initSkillsObjectStore (and the similar
call around the other mentioned range) behind the skills registry startup toggle
(check configData.SkillsRegistry.Enabled or the equivalent
skills_registry.enabled flag) so the object-store initialization only runs when
reconciliation is enabled; if initSkillsObjectStore returns an error after any
stores (e.g., config.LogsStore or other stores on config) have already been
opened, ensure you explicitly Close those initialized stores (call the
config.Close or the store.Close methods on the already-created objects) before
returning the error to avoid resource leaks.
In `@transports/config.schema.json`:
- Around line 1313-1325: The files[] entry in transports/config.schema.json
should enforce that when skills_registry.skills[].files[].source_type is
"url"/"filepath"/"text"/"dataurl" the corresponding payload field
(url/filepath/content/dataurl) is required and other payload fields are
disallowed; update the schema for files (the object with "required":
["path","source_type"] and properties path, source_type, url, filepath, content,
dataurl) to express this contract using JSON Schema conditionals (if/then/else)
or a oneOf with four variants that each require the correct field and set
additionalProperties: false for that variant so mismatched fields fail
validation at schema time. Ensure the updated rules refer to the same files[]
object so skills_registry.skills[].files[] is validated correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f53e36be-54c2-4465-8c4a-506d487745e5
📒 Files selected for processing (4)
transports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_skills.gotransports/bifrost-http/lib/config_test.gotransports/config.schema.json
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
from
June 10, 2026 05:57
a673be3 to
e874137
Compare
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
from
June 10, 2026 05:57
0ad867e to
f90a9af
Compare
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
from
June 10, 2026 06:27
e874137 to
fa938fb
Compare
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
from
June 10, 2026 06:27
f90a9af to
50e21f2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
transports/bifrost-http/lib/config_test.go (1)
1481-1483:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent error return in GetSkillByName mock.
This method returns
configstore.ErrNotFoundwhile the other skill getter mocks (GetSkill,GetSkillLean) returnnil, nil. Based on learnings, MockConfigStore methods should keep things simple by returning zero/nil values without special error cases. Consider aligning with the other getters for consistency.Suggested fix
func (m *MockConfigStore) GetSkillByName(ctx context.Context, name string) (*tables.TableSkill, error) { - return nil, configstore.ErrNotFound + return nil, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config_test.go` around lines 1481 - 1483, The MockConfigStore.GetSkillByName implementation returns configstore.ErrNotFound which is inconsistent with the other mock getters; update MockConfigStore.GetSkillByName to return the zero value and nil error (i.e., return nil, nil) to match the behavior of GetSkill and GetSkillLean so all mock getters are consistent and simple.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/lib/config_skills.go`:
- Around line 36-42: The reconcileSkillsRegistry loop currently mutates the
store without checking for duplicate SkillsRegistryEntry.Name values, so two
entries with the same name can partially apply; add a preflight duplicate-name
check at the start of reconcileSkillsRegistry that iterates entries, builds a
set (map[string]struct{}), and if any duplicate Name is found (referring to
SkillsRegistryEntry.Name and the skills_registry.skills logical key) log an
error and abort the function before calling reconcileOneSkill or performing any
writes; return early to ensure no mutations occur when duplicates exist.
- Around line 53-71: Compute the config hash and fetch the existing registry row
before calling configEntryToTableFiles to avoid needless URL HEADs and file
reads; call generateSkillRegistryEntryHash(entry) first, look up the current row
for this skill, and if the hash matches (no create/update needed) return early.
Only when the hash differs proceed to build the TableSkill/TableFiles (invoke
configEntryToTableSkill and configEntryToTableFiles), then run
configstore.ValidateSkill and configstore.ValidateSkillFile for each file and
continue with create/update logic.
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 1509-1511: The mock method MockConfigStore.GetAllSkillsVersion
currently returns the hardcoded non-zero value "1.0.0"; change it to return the
zero value for strings ("" with nil error) for consistency with other mocks
unless a test explicitly depends on a non-empty version; update the return in
GetAllSkillsVersion accordingly and run tests to ensure no tests rely on
"1.0.0".
---
Duplicate comments:
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 1481-1483: The MockConfigStore.GetSkillByName implementation
returns configstore.ErrNotFound which is inconsistent with the other mock
getters; update MockConfigStore.GetSkillByName to return the zero value and nil
error (i.e., return nil, nil) to match the behavior of GetSkill and GetSkillLean
so all mock getters are consistent and simple.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3eef08d2-143e-4e10-8f7c-5067f25f435b
📒 Files selected for processing (4)
transports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_skills.gotransports/bifrost-http/lib/config_test.gotransports/config.schema.json
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
from
June 10, 2026 11:09
fa938fb to
a7277f0
Compare
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
from
June 10, 2026 11:09
50e21f2 to
a469d0b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
transports/bifrost-http/lib/config_test.go (2)
1481-1483:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGetSkillByName should return zero values for consistency.
This method returns
configstore.ErrNotFoundwhile other skill getter stubs (GetSkill,GetSkillLean) returnnil, nil. Per learnings, MockConfigStore methods should be kept simple by returning zero/nil values. Align with the other getters for consistency.🔧 Suggested fix
func (m *MockConfigStore) GetSkillByName(ctx context.Context, name string) (*tables.TableSkill, error) { - return nil, configstore.ErrNotFound + return nil, nil }Based on learnings: "In tests under transports/bifrost-http/lib/config_test.go, keep MockConfigStore methods (e.g., GetVirtualKeysPaginated) as simple, returning zero/nil values."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config_test.go` around lines 1481 - 1483, The MockConfigStore.GetSkillByName stub currently returns (nil, configstore.ErrNotFound) while other getters like GetSkill and GetSkillLean return (nil, nil); change GetSkillByName to return zero values (nil, nil) for consistency with other mock methods in MockConfigStore so tests use simple zero/nil returns.Source: Learnings
1509-1511: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueGetAllSkillsVersion should return zero value for consistency.
This method returns
"1.0.0"instead of the zero value""for strings. Per learnings, keep mock methods simple with zero/nil values unless tests require a specific non-zero value. Consider returning""for consistency with other mocks unless there's a test dependency on the version format.🔧 Suggested change for consistency
func (m *MockConfigStore) GetAllSkillsVersion(ctx context.Context) (string, error) { - return "1.0.0", nil + return "", nil }Based on learnings: "In tests under transports/bifrost-http/lib/config_test.go, keep MockConfigStore methods simple, returning zero/nil values."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config_test.go` around lines 1509 - 1511, The MockConfigStore.GetAllSkillsVersion method returns a non-zero value ("1.0.0"); change it to return the string zero value and nil error for consistency with other mocks by updating MockConfigStore.GetAllSkillsVersion(ctx context.Context) to return "" , nil so tests use the default zero value unless a specific version is required.Source: Learnings
transports/bifrost-http/lib/config.go (2)
4590-4592:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound the object-store ping with a short timeout.
Line 4590 uses the parent startup context directly. If that context is long-lived or lacks a deadline, an unreachable object store can hang process startup indefinitely.
Suggested fix
func initSkillsObjectStore(ctx context.Context, config *Config, logStoreConfig *logstore.Config) error { if config == nil || config.ObjectStore != nil || logStoreConfig == nil || logStoreConfig.ObjectStorage == nil { return nil } objStore, err := objectstore.NewObjectStore(ctx, logStoreConfig.ObjectStorage, logger) if err != nil { return fmt.Errorf("failed to create skills object store: %w", err) } - if err := objStore.Ping(ctx); err != nil { + pingCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + if err := objStore.Ping(pingCtx); err != nil { _ = objStore.Close() return fmt.Errorf("failed to ping skills object store: %w", err) } config.ObjectStore = objStore logger.Info("skills object store initialized") return nil }As per coding guidelines, external calls in Go startup paths should enforce timeouts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 4590 - 4592, The objStore.Ping call uses the long-lived ctx and can hang startup; replace it by creating a short timeout context (e.g., pingCtx, cancel := context.WithTimeout(ctx, time.Second*5); defer cancel()) and use pingCtx when calling objStore.Ping; ensure you still call objStore.Close() on error and return fmt.Errorf(...) after canceling so the timeout is enforced and resources are cleaned up (refer to objStore.Ping, objStore.Close, ctx, and the fmt.Errorf return).Source: Coding guidelines
931-933:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate skills object-store init behind the feature and clean up on failure.
These branches now initialize the skills object store whenever the logs-store config has
object_storage, even ifskills_registryis absent or disabled. That makes an optional feature block process boot, and a failure here returns fromLoadConfigafterConfigStore/LogsStoremay already be open, leaking resources on the failed startup path.Gate this behind the effective
skills_registry.enabledtoggle and close already-initialized stores before returning the error.Also applies to: 983-985
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 931 - 933, The initSkillsObjectStore call is currently run whenever logs-store has object_storage, leaking resources if it fails; modify LoadConfig to only call initSkillsObjectStore when the effective skills_registry.enabled is true (respecting the configured default/overrides) and, if initSkillsObjectStore returns an error, ensure any previously-initialized stores (e.g., ConfigStore and LogsStore created earlier in LoadConfig) are closed/cleaned up before returning the error; apply the same gating and cleanup fix for the other occurrence around the 983-985 block so both paths respect skills_registry.enabled and don’t leak open stores on init failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/lib/config_skills.go`:
- Around line 140-141: The log call in the MIME inference failure currently
prints cf.URL verbatim and may leak secrets; update the error path around
inferConfigLiveURLMimeType to redact sensitive parts of cf.URL before logging
(e.g., strip userinfo and/or query string or mask query values) and use the
redacted value in the logger.Warn call that includes entry.Name and cf.Path;
ensure the redaction logic is applied where the logger.Warn is invoked so any
transient HEAD failures never emit raw URLs.
---
Duplicate comments:
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 1481-1483: The MockConfigStore.GetSkillByName stub currently
returns (nil, configstore.ErrNotFound) while other getters like GetSkill and
GetSkillLean return (nil, nil); change GetSkillByName to return zero values
(nil, nil) for consistency with other mock methods in MockConfigStore so tests
use simple zero/nil returns.
- Around line 1509-1511: The MockConfigStore.GetAllSkillsVersion method returns
a non-zero value ("1.0.0"); change it to return the string zero value and nil
error for consistency with other mocks by updating
MockConfigStore.GetAllSkillsVersion(ctx context.Context) to return "" , nil so
tests use the default zero value unless a specific version is required.
In `@transports/bifrost-http/lib/config.go`:
- Around line 4590-4592: The objStore.Ping call uses the long-lived ctx and can
hang startup; replace it by creating a short timeout context (e.g., pingCtx,
cancel := context.WithTimeout(ctx, time.Second*5); defer cancel()) and use
pingCtx when calling objStore.Ping; ensure you still call objStore.Close() on
error and return fmt.Errorf(...) after canceling so the timeout is enforced and
resources are cleaned up (refer to objStore.Ping, objStore.Close, ctx, and the
fmt.Errorf return).
- Around line 931-933: The initSkillsObjectStore call is currently run whenever
logs-store has object_storage, leaking resources if it fails; modify LoadConfig
to only call initSkillsObjectStore when the effective skills_registry.enabled is
true (respecting the configured default/overrides) and, if initSkillsObjectStore
returns an error, ensure any previously-initialized stores (e.g., ConfigStore
and LogsStore created earlier in LoadConfig) are closed/cleaned up before
returning the error; apply the same gating and cleanup fix for the other
occurrence around the 983-985 block so both paths respect
skills_registry.enabled and don’t leak open stores on init failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f16812da-0aa7-4622-91c4-1a00afba958b
📒 Files selected for processing (4)
transports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_skills.gotransports/bifrost-http/lib/config_test.gotransports/config.schema.json
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
from
June 10, 2026 11:29
a7277f0 to
c47e03e
Compare
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
from
June 10, 2026 11:29
a469d0b to
5ba3a5a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
transports/bifrost-http/lib/config_test.go (2)
1481-1483:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent error return in GetSkillByName mock.
This method returns
configstore.ErrNotFoundwhile the other skill getter mocks (GetSkill,GetSkillLean) returnnil, nil. Based on learnings, MockConfigStore methods should keep things simple by returning zero/nil values without special error cases. Consider aligning with the other getters for consistency.Suggested fix
func (m *MockConfigStore) GetSkillByName(ctx context.Context, name string) (*tables.TableSkill, error) { - return nil, configstore.ErrNotFound + return nil, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config_test.go` around lines 1481 - 1483, The MockConfigStore.GetSkillByName implementation is returning configstore.ErrNotFound while other mock getters (GetSkill, GetSkillLean) return nil, nil; change GetSkillByName to return the zero value and no error (i.e., return nil, nil) to match the other mock getters and keep the mock behavior consistent; update the MockConfigStore.GetSkillByName function accordingly.Source: Learnings
1509-1511: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider returning zero value for consistency.
GetAllSkillsVersionreturns"1.0.0"instead of the zero value""for strings. While this might be intentional if tests require a valid version format, the learning recommends keeping mock methods simple with zero/nil values. Consider returning""unless there's a specific test requirement for a non-empty version.Suggested change for consistency
func (m *MockConfigStore) GetAllSkillsVersion(ctx context.Context) (string, error) { - return "1.0.0", nil + return "", nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config_test.go` around lines 1509 - 1511, The MockConfigStore.GetAllSkillsVersion mock currently returns a hard-coded "1.0.0"; change it to return the zero value for strings ("" , nil) for consistency with other mocks unless a specific test asserts a non-empty version, in which case document that requirement or create a dedicated test-specific override.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/lib/config_skills.go`:
- Around line 132-159: The loop over entry.Files takes addresses of the loop
variable fields (e.g. &cf.URL, &cf.Filepath, &cf.Content, &cf.DataURL) which
causes pointer aliasing; change configEntryToTableFiles to avoid taking
addresses of the reused range variable by creating new local string variables
(or using a helper stringPtr) for each field before assigning to
configstoreTables.TableSkillFile.SourceURL, SourceFilepath, InlineContent, and
DataURL so each TableSkillFile gets its own distinct *string rather than
pointing to the final cf value.
---
Duplicate comments:
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 1481-1483: The MockConfigStore.GetSkillByName implementation is
returning configstore.ErrNotFound while other mock getters (GetSkill,
GetSkillLean) return nil, nil; change GetSkillByName to return the zero value
and no error (i.e., return nil, nil) to match the other mock getters and keep
the mock behavior consistent; update the MockConfigStore.GetSkillByName function
accordingly.
- Around line 1509-1511: The MockConfigStore.GetAllSkillsVersion mock currently
returns a hard-coded "1.0.0"; change it to return the zero value for strings (""
, nil) for consistency with other mocks unless a specific test asserts a
non-empty version, in which case document that requirement or create a dedicated
test-specific override.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f22470d-33b9-43c3-b4e0-ce93b573923d
📒 Files selected for processing (4)
transports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_skills.gotransports/bifrost-http/lib/config_test.gotransports/config.schema.json
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
from
June 10, 2026 12:07
5ba3a5a to
ff1a6ce
Compare
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
from
June 10, 2026 12:07
c47e03e to
24bbff1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
transports/bifrost-http/lib/config.go (2)
4586-4593:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound the startup object-store ping with a timeout.
objStore.Ping(ctx)inherits the caller's context directly. IfLoadConfigis invoked with a background or long-lived context, a stalled object-store endpoint can hang process startup indefinitely.Suggested fix
func initSkillsObjectStore(ctx context.Context, config *Config, logStoreConfig *logstore.Config) error { if config == nil || config.ObjectStore != nil || logStoreConfig == nil || logStoreConfig.ObjectStorage == nil { return nil } objStore, err := objectstore.NewObjectStore(ctx, logStoreConfig.ObjectStorage, logger) if err != nil { return fmt.Errorf("failed to create skills object store: %w", err) } - if err := objStore.Ping(ctx); err != nil { + pingCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + if err := objStore.Ping(pingCtx); err != nil { _ = objStore.Close() return fmt.Errorf("failed to ping skills object store: %w", err) } config.ObjectStore = objStore logger.Info("skills object store initialized") return nil }As per coding guidelines, "enforce timeouts and size limits" for Go external calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 4586 - 4593, The objStore.Ping call currently uses the incoming ctx and can hang startup; wrap the ping in a bounded context (use context.WithTimeout) before calling objStore.Ping and defer the cancel, then use that timed context for Ping and keep the existing error handling and objStore.Close() on failure; update the code around objectstore.NewObjectStore / objStore.Ping in the LoadConfig flow to create a short timeout (e.g., a few seconds) so external calls are bounded.Source: Coding guidelines
931-933:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate skills object-store init behind the registry toggle and unwind opened stores on failure.
This runs even when
skills_registryis absent or disabled, so an optional feature can now fail the whole boot on an otherwise unused object-store config. Also, Line 932 and Line 984 return afterLogsStore(and sometimesConfigStore) are already open, so those resources leak on the failed startup path becauseLoadConfignever reachesConfig.Close.Also applies to: 983-985
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 931 - 933, initSkillsObjectStore is being called unconditionally during LoadConfig causing startup failures when the optional skills_registry feature is absent or disabled and leaking open resources (LogsStore/ConfigStore) on error; gate the call to initSkillsObjectStore behind the skills_registry feature toggle (check the config flag or presence) so it only runs when enabled, and modify the error path in LoadConfig (and the branches around the existing returns at the calls near initSkillsObjectStore and the LogsStore/ConfigStore opens) to close any stores that were successfully opened before returning an error (invoke their Close methods or a cleanup helper) to ensure no resources leak on failed startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/lib/config_skills.go`:
- Around line 61-65: The code validates each entry with
configstore.ValidateSkillFile but doesn't preflight-check for duplicate files[]
Path values, which later causes unique constraint failures in
CreateSkill/UpdateSkill; add a duplicate-path guard before the validation loop
by scanning the files slice (e.g., use a map[string]struct{} or map[string]int
keyed by files[i].Path) and return a clear error like fmt.Errorf("duplicate file
path %q in request", path) if a duplicate is found; keep this check above the
loop that calls ValidateSkillFile so MIME probing/DB ops are avoided on bad
input.
---
Duplicate comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 4586-4593: The objStore.Ping call currently uses the incoming ctx
and can hang startup; wrap the ping in a bounded context (use
context.WithTimeout) before calling objStore.Ping and defer the cancel, then use
that timed context for Ping and keep the existing error handling and
objStore.Close() on failure; update the code around objectstore.NewObjectStore /
objStore.Ping in the LoadConfig flow to create a short timeout (e.g., a few
seconds) so external calls are bounded.
- Around line 931-933: initSkillsObjectStore is being called unconditionally
during LoadConfig causing startup failures when the optional skills_registry
feature is absent or disabled and leaking open resources (LogsStore/ConfigStore)
on error; gate the call to initSkillsObjectStore behind the skills_registry
feature toggle (check the config flag or presence) so it only runs when enabled,
and modify the error path in LoadConfig (and the branches around the existing
returns at the calls near initSkillsObjectStore and the LogsStore/ConfigStore
opens) to close any stores that were successfully opened before returning an
error (invoke their Close methods or a cleanup helper) to ensure no resources
leak on failed startup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 93ebae89-5c82-480b-b5d5-d71085579f50
📒 Files selected for processing (4)
transports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_skills.gotransports/bifrost-http/lib/config_test.gotransports/config.schema.json
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
from
June 10, 2026 12:34
ff1a6ce to
97839a2
Compare
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
2 times, most recently
from
June 10, 2026 12:50
54c7601 to
a6b71ca
Compare
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
2 times, most recently
from
June 11, 2026 18:56
50d16b9 to
48f00e5
Compare
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
2 times, most recently
from
June 11, 2026 18:58
4be44d8 to
cd028b2
Compare
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
from
June 11, 2026 18:58
48f00e5 to
2e539a8
Compare
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
from
June 11, 2026 19:20
2e539a8 to
b33b38c
Compare
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
2 times, most recently
from
June 12, 2026 07:09
0a0750f to
bc5992d
Compare
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
from
June 12, 2026 07:10
b33b38c to
6aeee4c
Compare
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
from
June 12, 2026 07:14
bc5992d to
b4d7bcb
Compare
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
2 times, most recently
from
June 12, 2026 07:40
4651cbf to
a8a79fc
Compare
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
from
June 12, 2026 07:40
b4d7bcb to
6250aa0
Compare
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
from
June 12, 2026 09:34
a8a79fc to
af6ad8e
Compare
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
from
June 12, 2026 09:34
6250aa0 to
fd3c2c5
Compare
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
from
June 12, 2026 09:55
fd3c2c5 to
86f29e7
Compare
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
from
June 12, 2026 09:55
af6ad8e to
13781f4
Compare
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
from
June 12, 2026 11:10
86f29e7 to
4fd957d
Compare
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
from
June 12, 2026 13:42
13781f4 to
4b2af22
Compare
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
from
June 12, 2026 13:42
4fd957d to
10012b5
Compare
danpiths
force-pushed
the
06-10-feat_add_config-based_skills_registry
branch
from
June 12, 2026 14:35
10012b5 to
136faca
Compare
danpiths
force-pushed
the
06-10-feat_add_skills_management_and_serving_apis
branch
from
June 12, 2026 14:35
4b2af22 to
0884897
Compare
Contributor
Merge activity
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds declarative Skills Repository support through Bifrost configuration. Skills
can now be defined in config, reconciled on startup, and kept in sync through
config-hash change detection.
Changes
skills_registryconfig structures.and frontmatter.
uploadsource type from config; supported config filesource types are
text,url,filepath, anddataurl.Type of change
Affected areas
How to test
Validate this change with the following command:
Expected result: config loading tests pass, including config-store mock coverage
for the expanded Skills Repository interface. The
skills_registryschema iscovered by the checked-in JSON schema.
New config surface:
skills_registryin Bifrost config.Screenshots/Recordings
N/A — backend config support only.
Breaking changes
Related issues
N/A
Security considerations
Config-defined skills are reconciled through the same backend validation and
persistence path as API-created skills. URL and filepath sources are
operator-controlled live references; filepath sources can read local files
during startup and serving, so they should only be used with trusted
configuration. The interactive
uploadsource type is not accepted from config.Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit