Add five new fields to MCPServiceConfig and mcpKnownKeys to support#384
Merged
moizpgedge merged 5 commits intoMay 22, 2026
Merged
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (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 Create/Update, 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 docstrings
🧪 Generate unit tests (beta)
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 | -3 |
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
jason-lynch
reviewed
May 19, 2026
Member
jason-lynch
left a comment
jason-lynch
left a comment
There was a problem hiding this comment.
This is looking good! Left a few minor comments, but the core functionality looks great.
| // service (no config.yaml yet) would return ErrNotFound before reaching | ||
| // here, skipping the check and allowing the container to be deployed with | ||
| // a broken bind mount. | ||
| if r.KBHostPath != "" { |
Member
There was a problem hiding this comment.
This check would be better in the Create and Update methods. Refresh is invoked for existing resources, so this won't be executed the first time you deploy the service.
| // service spec → BuildServiceHostList → GenerateMCPConfig → YAML output. | ||
| // It verifies that the generated YAML contains the correct ordered hosts | ||
| // array and target_session_attrs for various topologies. | ||
| func TestGenerateMCPConfig_KBDisabled_SectionOmitted(t *testing.T) { |
Member
There was a problem hiding this comment.
The comment above this line is for a different test: TestGenerateMCPConfig_MultiHostTopology. Could you please move the comment to the correct line?
|
|
||
| ### Knowledgebase | ||
|
|
||
| Knowledgebase support enables the `search_knowledgebase` tool to query |
Member
There was a problem hiding this comment.
Could you please add a sentence in this section to tell people that they can get the knowledge base file from the pgEdge Postgres MCP release artifacts? It's unfortunate that the KB releases are separate from the normal tagged releases, so I think the best we can do is tell them to the download the kb.db file from the latest Knowledge Base release on the releases page: https://github.com/pgEdge/pgedge-postgres-mcp/releases.
| !!! note | ||
|
|
||
| Changing any `kb_*` field (provider, model, credentials, or path) | ||
| requires a service redeploy — not just a config reload. SIGHUP only |
Member
There was a problem hiding this comment.
Is it necessary to describe this distinction about service redeploys vs config reload? Users shouldn't modify the config file outside of the Control Plane API, so they should only ever use the update database endpoint.
| Changing any `kb_*` field (provider, model, credentials, or path) | ||
| requires a service redeploy — not just a config reload. SIGHUP only | ||
| reloads database connection settings and does not reinitialize the | ||
| knowledgebase. Use `update-database` to apply KB config changes; the |
Member
There was a problem hiding this comment.
Suggested change
| knowledgebase. Use `update-database` to apply KB config changes; the | |
| knowledgebase. Use the `update-database` endpoint to apply KB config changes; the |
Just want to make it clear to users that we're talking about an API endpoint.
- Move KB file existence check from Refresh into a new checkKBFileExists helper called from Create and Update. Refresh is only invoked by the planner for resources already in state, so a check there would not fire on first deploy — silently allowing a container to start with a broken bind mount. The check now blocks both initial creates and updates. - Rewrite the 4 Refresh-based KB tests as Create/Update tests: Create_KBDisabled, Create_KBFilePresent, Create_KBFileMissing, Update_KBFileMissing. All pass. - Move the misplaced TestGenerateMCPConfig_MultiHostTopology doc comment from above TestGenerateMCPConfig_KBDisabled_ SectionOmitted to its correct location above the actual MultiHostTopology test. - Add a sentence to docs/services/mcp.md pointing users to the pgEdge Postgres MCP releases page to download the kb.db file, noting that Knowledge Base releases are tagged separately. - Remove the SIGHUP vs config-reload note fr the knowledgebase docs section. Users only interact with the update-database endpoint; how the Control Plane applies the change is an internal mechanic and shouldn't leak into the user-facing docs.
jason-lynch
approved these changes
May 22, 2026
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
2 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.
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