Add five new fields to MCPServiceConfig and mcpKnownKeys to support#384
Open
moizpgedge wants to merge 4 commits into
Open
Add five new fields to MCPServiceConfig and mcpKnownKeys to support#384moizpgedge wants to merge 4 commits into
MCPServiceConfig and mcpKnownKeys to support#384moizpgedge wants to merge 4 commits into
Conversation
knowledgebase search configuration for the MCP service:
- `kb_enabled` — opt-in bool; when false or absent all other KB fields
are ignored and no validation runs
- `kb_embedding_provider` — required when KB is enabled; `voyage` and
`openai` are the only accepted values for V1
- `kb_embedding_model` — required when KB is enabled
- `kb_embedding_api_key` — required for `voyage` and `openai`; scrubbed
from API output automatically via the existing `api_key` sensitive-field
policy
- `kb_database_host_path` — optional override for the KB file path on the
host; defaults to `{DataDir}/kb/nla-kb.db` in downstream tickets
Validation rules enforced at `ParseMCPServiceConfig` call time:
- `ollama` is rejected with an explicit "not yet supported in V1" error;
full ollama support is blocked on the `embedding_ollama_url` field
mapping question (see PLAT-607)
- Setting `kb_enabled: true` together with `disable_search_knowledgebase:
true` is rejected — the combination would silently produce a running
container with no `search_knowledgebase` tool registered
- All KB fields are silently ignored when `kb_enabled` is false or absent,
matching the same opt-in pattern used by `llm_enabled`
PLAT-590
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMCPServiceConfig adds knowledgebase (KB) configuration support, with parsing and validation. The change emits a knowledgebase section in generated YAML, requires staged KB files during resource Refresh, propagates KB host/dir paths through orchestrator and service specs to add a read-only /app/kb mount for RAG containers, and adds tests and docs. ChangesKnowledgebase Configuration
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 5 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
…-608) Enables the search_knowledgebase tool in MCP service deployments via a host-placed SQLite KB file bind-mounted read-only into the container. **PLAT-590 — KB fields on MCPServiceConfig (already committed)** Five new config fields: kb_enabled, kb_embedding_provider, kb_embedding_model, kb_embedding_api_key, kb_database_host_path. Parse-time validation rejects unknown providers, missing credentials, and the kb_enabled + disable_search_knowledgebase conflict (PLAT-607). kb_embedding_api_key is scrubbed from API output. **PLAT-591 — knowledgebase: section in config.yaml** GenerateMCPConfig now emits a knowledgebase: block when kb_enabled is true. The database_path uses the container-side path (/app/kb/<file>), derived from the host path basename. The API key field name varies by provider: embedding_voyage_api_key or embedding_openai_api_key. **PLAT-592 — read-only /app/kb bind mount** ServiceContainerSpec adds a read-only bind mount from the host KB directory to /app/kb when KBDirPath is non-empty. The mount is on the directory, not the file, so atomic renames of the KB file are visible to the container immediately. **PLAT-593 — deployment blocked when KB file is missing** MCPConfigResource.Refresh checks for the KB file before checking for config.yaml. This ordering is intentional: a new service has no config.yaml yet, so checking config.yaml first would return ErrNotFound (triggering Create) before the KB check runs. By checking KB first, a missing file returns a plain error that blocks both initial creates and updates. ErrNotFound is never returned for a missing KB file. **PLAT-608 — documentation** docs/services/mcp.md gains a Knowledgebase section under Configuration Reference (5-field table, warning admonition for the staging requirement) and a Knowledgebase Search (Voyage AI) example under Examples. - 19 parse-time validation cases (PLAT-590 + PLAT-607) - 7 config generation cases coveproviders and custom path (PLAT-591) - 2 container spec cases confirming mount presence/absence (PLAT-592) - 9 Refresh cases covering KB disabled, file present, file missing with and without existing config.yaml (PLAT-593)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/internal/orchestrator/swarm/mcp_config_resource_test.go (1)
24-24: ⚡ Quick winHandle the error from
MkdirAllfor consistency.The error from
fs.MkdirAllis silently discarded, while line 39 usest.Fatalffor errors, and the test cases userequire.NoErrorfor file operations (lines 85, 105-107, 128). For consistency and fail-fast behavior on setup issues, userequire.NoErrorhere as well.🛡️ Proposed fix
- _ = fs.MkdirAll(dirPath, 0o700) + require.NoError(t, fs.MkdirAll(dirPath, 0o700))🤖 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 `@server/internal/orchestrator/swarm/mcp_config_resource_test.go` at line 24, The call to fs.MkdirAll currently discards its error; change it to check and fail the test on error (consistent with other setup code) by capturing the error from fs.MkdirAll(dirPath, 0o700) and calling require.NoError(t, err) (or t.Fatalf on error) so the test setup fails fast; refer to the fs.MkdirAll call and dirPath in mcp_config_resource_test.go and use the existing testing imports (require) used elsewhere in the file.
🤖 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.
Nitpick comments:
In `@server/internal/orchestrator/swarm/mcp_config_resource_test.go`:
- Line 24: The call to fs.MkdirAll currently discards its error; change it to
check and fail the test on error (consistent with other setup code) by capturing
the error from fs.MkdirAll(dirPath, 0o700) and calling require.NoError(t, err)
(or t.Fatalf on error) so the test setup fails fast; refer to the fs.MkdirAll
call and dirPath in mcp_config_resource_test.go and use the existing testing
imports (require) used elsewhere in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd5e577a-1ffb-40f0-bce1-b442d8283c6e
📒 Files selected for processing (9)
docs/services/mcp.mdserver/internal/orchestrator/swarm/mcp_config.goserver/internal/orchestrator/swarm/mcp_config_resource.goserver/internal/orchestrator/swarm/mcp_config_resource_test.goserver/internal/orchestrator/swarm/mcp_config_test.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/orchestrator/swarm/service_spec_test.go
Address security and correctness issues raised in code review: - Reject `kb_database_host_path` values that are not absolute or contain `..` components. An unsanitized host path could be used to bind-mount arbitrary host directories into the container via the CP API. - Reject stale `kb_*` fields (provider, model, api_key, host_path) when `kb_enabled` is false or absent, matching the existing LLM pattern. Previously these were silently ignored, hiding typos and stale config. - Make `ollama` provider check case-insensitive (`strings.ToLower`) so `"Ollama"` gets the explicit "not yet supported" error instead of the generic "must be one of" message. - Replace defensive nil checks in `GenerateMCPConfig` with an invariant assertion. If KB fields are nil despite `kb_enabled=true`, the function now returns an error immediately rather than silently emitting empty strings into `config.yaml`. Fields are dereferenced unconditionally below the assertion. - Fix discarded `MkdirAll` error in `mcpRCAndFs` test helper — use `require.NoError` instead of `_ =`. - Add `!!! note` to `docs/services/mcp.md` stating that any `kb_*` config change requires a container restart, not just a SIGHUP reload. - Add test cases: relative path, `..` path, case-insensitive ollama, and four cases for stale KB fields rejected when disabled. PLAT-590
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/services/mcp.md`:
- Around line 87-90: The docs currently claim kb_* fields are ignored when
kb_enabled is false, but implementation rejects those fields; update the
phrasing around kb_enabled and kb_* (lines referencing "kb_enabled", "kb_*", and
the embedding providers "voyage" and "openai" and note about "Ollama") to state
that kb_* fields are invalid/rejected unless kb_enabled is true (also apply the
same change to the other occurrence mentioned around lines 109-110), and make
clear the validator will fail the config rather than silently ignoring those
fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42cf1446-aff5-4f7d-bf4f-ccb485abcc5f
📒 Files selected for processing (5)
docs/services/mcp.mdserver/internal/database/mcp_service_config.goserver/internal/database/mcp_service_config_test.goserver/internal/orchestrator/swarm/mcp_config.goserver/internal/orchestrator/swarm/mcp_config_resource_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/internal/orchestrator/swarm/mcp_config.go
- server/internal/orchestrator/swarm/mcp_config_resource_test.go
- server/internal/database/mcp_service_config_test.go
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
1 participant
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.
feat: add knowledgebase support to MCP service
Enables the
search_knowledgebasetool in MCP service deployments.Operators place a SQLite KB file on the host once; the Control Plane
validates it, bind-mounts it read-only into every MCP container on
that host, and wires up the
knowledgebase:section inconfig.yaml.Summary
MCPServiceConfigwithparse-time validation
kb_enabled: true+disable_search_knowledgebase: truetogetherknowledgebase:section emitted inconfig.yaml/app/kbbind mount in the MCP containerdocs/services/mcp.md)Changes
PLAT-590 — KB fields on
MCPServiceConfigFive new config fields added to
MCPServiceConfigandmcpKnownKeys:kb_enabledfalseor absent, stalekb_*fields are rejected (matching thellm_enabledpattern).kb_embedding_providerkb_enabledistrue. Accepted:voyage,openai.kb_embedding_modelkb_enabledistrue. Must match the model used to build the KB file.kb_embedding_api_keyvoyageandopenai. Scrubbed from API responses automatically via the existingapi_keysensitive-field policy.kb_database_host_path{DataDir}/kb/nla-kb.db.Validation rules enforced in
ParseMCPServiceConfig:kb_embedding_providerandkb_embedding_modelare required whenkb_enabled: truekb_embedding_api_keyis required forvoyageandopenaiollamais explicitly rejected — the MCP KB config requires aseparate
embedding_ollama_urlfield and there is no matching CPfield yet; accepting it would silently write an empty URL
kb_embedding_provider,kb_embedding_model,kb_embedding_api_key,kb_database_host_path) are rejectedwhen
kb_enabledisfalseor absent, matching the same patternused by
llm_enabledPLAT-607 —
kb_enabled+disable_search_knowledgebaseconflictSetting
kb_enabled: truetogether withdisable_search_knowledgebase: trueis rejected at parse time.The combination would deploy a running container with no
search_knowledgebasetool registered — a silently broken setup.This validation lives in the same block as the KB field validation and
is included here to close the gap between merging PLAT-590 and a
hypothetical standalone PLAT-607 ticket.
PLAT-591 —
knowledgebase:section inconfig.yamlGenerateMCPConfignow emits aknowledgebase:block whenkb_enabledistrue. Thedatabase_pathuses the container-sidepath (
/app/kb/<basename>), derived from the host path basename.The API key field name varies by provider:
embedding_voyage_api_keyorembedding_openai_api_key.Example output for OpenAI:
When
kb_enabledis absent orfalse, noknowledgebase:key isemitted and behavior is identical to before this change.
PLAT-592 — Read-only
/app/kbbind mountServiceContainerSpecadds a read-only bind mount from the host KBdirectory to
/app/kbwhenKBDirPathis non-empty. The mount is onthe directory, not the file. A file bind mount ties the container
to the original inode — an atomic rename (write to temp file, rename
into place) creates a new inode, so the container would keep reading
the old file. A directory mount tracks by name, so the renamed file is
visible immediately. When
kb_enabledis absent orfalse, no extramount is added.
PLAT-593 — Deployment blocked when KB file is missing
MCPConfigResource.Refreshchecks for the KB file before checkingfor
config.yaml. This ordering is intentional: a new service has noconfig.yamlyet, so checkingconfig.yamlfirst would returnErrNotFound(triggering Create) before the KB check ran, silentlyallowing a container to start with a broken bind mount.
By checking KB first, a missing file returns a plain
fmt.Errorf,which blocks both initial creates and subsequent updates.
ErrNotFoundis never returned for a missing KB file.
PLAT-608 — Documentation
docs/services/mcp.mdgains:!!! warningadmonition (staging requirement and default path), 5-field table
block, complete
curlrequest body, custom path noteTests
Unit tests
TestParseMCPServiceConfig/knowledgebase_configTestGenerateMCPConfig_KBTestServiceContainerSpec_MCPTestMCPConfigResourceKey PLAT-593 cases:
Refresh_KBDisabledKBHostPath=""nilRefresh_KBFilePresentnilRefresh_KBFileMissingconfig.yamlpresentErrNotFoundRefresh_KBFileMissing_NoConfigYetconfig.yamlErrNotFoundManual — validation errors (all return HTTP 400)
Manual — successful deployment
Stage the KB file first:
mkdir -p docker/control-plane-dev/data/host-1/kb gh release download kb-2026-04-27 --repo pgEdge/pgedge-postgres-mcp \ --pattern "kb.db" mv kb.db docker/control-plane-dev/data/host-1/kb/nla-kb.dbDeploy:
Verify (PLAT-591):
config.yamlcontains aknowledgebase:block withdatabase_path: /app/kb/nla-kb.db.Verify (PLAT-592):
docker inspectshows a read-only bind mount at/app/kb.Verify (PLAT-590):
restish control-plane-local-1 get-database kb-ok—
kb_embedding_api_keyis absent fromservices[0].config.Verify (PLAT-593): Remove the KB file and redeploy — task fails with
KB database file not found at .../nla-kb.db.Notes for Reviewers
PLAT-607 is included in this PR — it lives in the same validation
block as the KB field checks. Splitting it out would leave a window
where a user could configure a silently broken setup.
kb_embedding_api_keyscrubbing requires no code change — theexisting
isSensitiveConfigKeyfunction matches any key containing thesubstring
api_key, sokb_embedding_api_keyis covered automatically.The KB file check ordering in
Refreshis load-bearing — thecomment in the code explains why it must come before the
config.yamlcheck. Do not reorder.
SIGHUP does not reload KB — SIGHUP triggers
clientManager.UpdateDatabaseConfigs()only. KB state is initializedonce at startup. Any KB config change (provider, model, credentials,
or path) requires a container restart via a Control Plane service
update.
Checklist
docs/services/mcp.md)Issues