Conversation
WalkthroughThis pull request introduces MCP (Model Context Protocol) server integration to both the Horizon and Online Feature Store projects. New server entrypoints are added for each module with tool handlers enabling entity/feature discovery and retrieval capabilities via the MCP protocol. Dependencies on the mcp-go library are introduced. Changes
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@horizon/internal/mcp/tools.go`:
- Around line 69-77: The summaries slice is left nil when featureGroups is
nil/empty which causes json.Marshal(summaries) to emit "null" instead of "[]";
initialize summaries as an empty slice (e.g., make([]fgSummary, 0) or
[]fgSummary{}) before appending so that the variable used later (summaries)
always marshals to an empty JSON array; update the block around the summaries
declaration and the loop that iterates over featureGroups (referencing the
summaries variable and fgSummary type) to ensure summaries is non-nil even when
no featureGroups are present.
In `@online-feature-store/cmd/mcp-server/main.go`:
- Around line 7-8: Remove the duplicate import of the same package
(github.com/Meesho/BharatMLStack/online-feature-store/internal/config) so only
one alias is used (keep featureConfig and remove config), then update any usages
that reference config (e.g., in main where config.SomeVar or config.NewX is
used) to use featureConfig instead so all references consistently use
featureConfig.
In `@online-feature-store/internal/mcp/tools.go`:
- Around line 170-182: injectAuthContext silently returns the original context
when callerIDEnvVar or authTokenEnvVar are empty, allowing auth bypass; update
injectAuthContext to emit a warning via the existing logger when either env var
is missing (include which one) and continue, or better: add a startup validation
in main() that checks callerIDEnvVar and authTokenEnvVar and fails fast with a
clear error if they are required; reference callerIDEnvVar, authTokenEnvVar,
callerIDHeader, authTokenHeader and metadata.NewIncomingContext to locate the
code and either add logging inside injectAuthContext or perform validation in
main() and exit on missing credentials.
🧹 Nitpick comments (3)
online-feature-store/cmd/mcp-server/main.go (1)
38-48: Consider whether failed watch-path registrations should be fatal.If etcd watch callbacks fail to register, the server will run with stale configuration and no mechanism to detect it. For circuit breaker and client registration updates, this could be a silent reliability risk. Consider making these fatal or at minimum emitting a metric for alerting.
online-feature-store/internal/mcp/tools.go (1)
163-166:HealthCheckdoesn't actually verify service health.This always returns
{"status":"ok"}regardless of the actual state of the feature store. Consider at minimum pinging the underlying handler or a dependency to provide meaningful health status.horizon/cmd/mcp-server/main.go (1)
42-42: Replace magic number1with a named constant.
ofsHandler.NewConfigHandler(1)uses a magic number. Extract this to a descriptive constant (e.g.,configHandlerVersion) for clarity.Proposed fix
+const configHandlerVersion = 1 + func main() { var appConfig AppConfig ... - configHandler := ofsHandler.NewConfigHandler(1) + configHandler := ofsHandler.NewConfigHandler(configHandlerVersion)
| var summaries []fgSummary | ||
| if featureGroups != nil { | ||
| for _, fg := range *featureGroups { | ||
| summaries = append(summaries, fgSummary{ | ||
| Label: fg.FeatureGroupLabel, | ||
| DataType: string(fg.DataType), | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
json.Marshal(nil) produces "null", not "[]" — initialize the slice.
When featureGroups is nil or empty, summaries stays nil, and json.Marshal(nil) outputs null. MCP tool consumers likely expect an empty JSON array.
Proposed fix
- var summaries []fgSummary
- if featureGroups != nil {
+ summaries := make([]fgSummary, 0)
+ if featureGroups != nil {🤖 Prompt for AI Agents
In `@horizon/internal/mcp/tools.go` around lines 69 - 77, The summaries slice is
left nil when featureGroups is nil/empty which causes json.Marshal(summaries) to
emit "null" instead of "[]"; initialize summaries as an empty slice (e.g.,
make([]fgSummary, 0) or []fgSummary{}) before appending so that the variable
used later (summaries) always marshals to an empty JSON array; update the block
around the summaries declaration and the loop that iterates over featureGroups
(referencing the summaries variable and fgSummary type) to ensure summaries is
non-nil even when no featureGroups are present.
| "github.com/Meesho/BharatMLStack/online-feature-store/internal/config" | ||
| featureConfig "github.com/Meesho/BharatMLStack/online-feature-store/internal/config" |
There was a problem hiding this comment.
Fix duplicate import — CI is failing (ST1019).
Both config and featureConfig alias the same package github.com/Meesho/BharatMLStack/online-feature-store/internal/config. Remove one and use a single alias throughout.
Proposed fix
- "github.com/Meesho/BharatMLStack/online-feature-store/internal/config"
- featureConfig "github.com/Meesho/BharatMLStack/online-feature-store/internal/config"
+ featureConfig "github.com/Meesho/BharatMLStack/online-feature-store/internal/config"Then update line 30 to use featureConfig instead of config:
- etcd.Init(configManagerVersion, &config.FeatureRegistry{})
+ etcd.Init(configManagerVersion, &featureConfig.FeatureRegistry{})🧰 Tools
🪛 GitHub Actions: Online Feature Store CI
[error] 7-7: staticcheck: package "github.com/Meesho/BharatMLStack/online-feature-store/internal/config" is being imported more than once (ST1019).
🤖 Prompt for AI Agents
In `@online-feature-store/cmd/mcp-server/main.go` around lines 7 - 8, Remove the
duplicate import of the same package
(github.com/Meesho/BharatMLStack/online-feature-store/internal/config) so only
one alias is used (keep featureConfig and remove config), then update any usages
that reference config (e.g., in main where config.SomeVar or config.NewX is
used) to use featureConfig instead so all references consistently use
featureConfig.
| func injectAuthContext(ctx context.Context) context.Context { | ||
| callerID := os.Getenv(callerIDEnvVar) | ||
| authToken := os.Getenv(authTokenEnvVar) | ||
|
|
||
| if callerID == "" || authToken == "" { | ||
| return ctx | ||
| } | ||
|
|
||
| md := metadata.New(map[string]string{ | ||
| callerIDHeader: callerID, | ||
| authTokenHeader: authToken, | ||
| }) | ||
| return metadata.NewIncomingContext(ctx, md) |
There was a problem hiding this comment.
Silent auth bypass when env vars are unset.
If OFS_CALLER_ID or OFS_AUTH_TOKEN are not configured, injectAuthContext silently returns the original context without auth metadata. The downstream gRPC handler may or may not reject unauthenticated requests. Consider logging a warning or failing at startup if these are expected to be set.
Option: warn on missing credentials
func injectAuthContext(ctx context.Context) context.Context {
callerID := os.Getenv(callerIDEnvVar)
authToken := os.Getenv(authTokenEnvVar)
if callerID == "" || authToken == "" {
+ // Consider logging a warning here or validating at startup
return ctx
}Alternatively, validate these env vars in main() at startup and fail fast.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func injectAuthContext(ctx context.Context) context.Context { | |
| callerID := os.Getenv(callerIDEnvVar) | |
| authToken := os.Getenv(authTokenEnvVar) | |
| if callerID == "" || authToken == "" { | |
| return ctx | |
| } | |
| md := metadata.New(map[string]string{ | |
| callerIDHeader: callerID, | |
| authTokenHeader: authToken, | |
| }) | |
| return metadata.NewIncomingContext(ctx, md) | |
| func injectAuthContext(ctx context.Context) context.Context { | |
| callerID := os.Getenv(callerIDEnvVar) | |
| authToken := os.Getenv(authTokenEnvVar) | |
| if callerID == "" || authToken == "" { | |
| // Consider logging a warning here or validating at startup | |
| return ctx | |
| } | |
| md := metadata.New(map[string]string{ | |
| callerIDHeader: callerID, | |
| authTokenHeader: authToken, | |
| }) | |
| return metadata.NewIncomingContext(ctx, md) | |
| } |
🤖 Prompt for AI Agents
In `@online-feature-store/internal/mcp/tools.go` around lines 170 - 182,
injectAuthContext silently returns the original context when callerIDEnvVar or
authTokenEnvVar are empty, allowing auth bypass; update injectAuthContext to
emit a warning via the existing logger when either env var is missing (include
which one) and continue, or better: add a startup validation in main() that
checks callerIDEnvVar and authTokenEnvVar and fails fast with a clear error if
they are required; reference callerIDEnvVar, authTokenEnvVar, callerIDHeader,
authTokenHeader and metadata.NewIncomingContext to locate the code and either
add logging inside injectAuthContext or perform validation in main() and exit on
missing credentials.
1 participant
🔁 Pull Request Template – BharatMLStack
Context:
Give a brief overview of the motivation behind this change. Include any relevant discussion links (Slack, documents, tickets, etc.) that help reviewers understand the background and the issue being addressed.
Describe your changes:
Mention the changes made in the codebase.
Testing:
Please describe how you tested the code. If manual tests were performed - please explain how. If automatic tests were added or existing ones cover the change - please explain how did you run them.
Monitoring:
Explain how this change will be tracked after deployment. Indicate whether current dashboards, alerts, and logs are enough, or if additional instrumentation is required.
Rollback plan
Explain rollback plan in case of issues.
Checklist before requesting a review
📂 Modules Affected
horizon(Real-time systems / networking)online-feature-store(Feature serving infra)trufflebox-ui(Admin panel / UI)infra(Docker, CI/CD, GCP/AWS setup)docs(Documentation updates)___________✅ Type of Change
___________📊 Benchmark / Metrics (if applicable)
Summary by CodeRabbit