feat: Get catalog from OCI registry - #30
Conversation
|
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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR migrates the MCP server's extension definitions from hardcoded Rust code to an externally-sourced JSON catalog loadable from OCI registries, with trust validation, size enforcement, and digest verification. ChangesExtension Catalog System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
extensions/control-plane/README.md (1)
364-364:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocumented MCP persistence size default is inconsistent with
values.yaml.The README states
supernodeMcp.persistence.sizedefaults to1Gi, butextensions/control-plane/values.yaml(line 191) sets it to10Gi. Update one of them so operators relying on the docs get accurate sizing.📝 Proposed fix
-| `supernodeMcp.persistence.size` | MCP session store PVC size | `1Gi` | +| `supernodeMcp.persistence.size` | MCP session store PVC size | `10Gi` |🤖 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 `@extensions/control-plane/README.md` at line 364, The README entry for supernodeMcp.persistence.size must match the actual default in values.yaml; update either the README table row or the default in extensions/control-plane/values.yaml so both use the same value (choose 10Gi if the PVC should be larger or change the values.yaml default to 1Gi if the README is correct). Specifically, reconcile the symbol supernodeMcp.persistence.size by editing the README row that currently shows `1Gi` or the values.yaml setting at the supernodeMcp.persistence.size key so they are identical and reflect the intended operator default.
🧹 Nitpick comments (2)
mcp-server/src/config.rs (1)
97-114: 💤 Low valueConsider adding tests for the env-var helper validations.
The new
parses_catalog_sources/rejects_unknown_catalog_sourcecoverFromStr, but the validation logic inextension_catalog_config(missing OCI ref when source is OCI),extension_catalog_max_bytes(parse error / default), andextension_catalog_allow_untrusted(true/false/empty/invalid) is currently uncovered. Since these helpers read process-wide env vars, you'd likely want to factor them to accept inputs or guard tests with a mutex — happy to deferto a follow-up.Also applies to: 130-153
🤖 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 `@mcp-server/src/config.rs` around lines 97 - 114, Add unit tests that cover the validation branches in extension_catalog_config, extension_catalog_max_bytes, and extension_catalog_allow_untrusted by either refactoring those helpers to accept inputs (e.g., pass source/oci_ref/max_bytes/allow_untrusted strings into parsing functions) or by serializing tests that mutate process env vars using a test-wide mutex/guard; specifically exercise the case where source == ExtensionCatalogSource::Oci with missing MCP_EXTENSION_CATALOG_OCI_REF to trigger ConfigError::MissingExtensionCatalogOciRef, invalid and empty MCP_EXTENSION_CATALOG_MAX_BYTES to exercise parse/default behavior, and true/false/empty/invalid MCP_EXTENSION_CATALOG_ALLOW_UNTRUSTED values to verify boolean parsing and defaults.mcp-server/src/server.rs (1)
56-57: Heads-up: OCI catalog fetch is on the startup critical path.
load_catalogis awaited synchronously before the listener binds, so a transiently unreachable OCI registry (or a slow pull near themax_bytesceiling) will block startup and surface as a hard process failure. This is reasonable fail-fast behavior, but in Kubernetes you'll likely want:
- A startup/liveness probe budget large enough to accommodate registry latency.
- Clear log lines (registry host, ref, attempt) emitted from
load_catalogso failed pulls are diagnosable from pod logs.- Possibly a local cache of the last-known-good catalog as a fallback, since today an OCI outage = MCP server outage.
No change requested here — flagging for operational awareness as you wire this into the control-plane chart.
🤖 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 `@mcp-server/src/server.rs` around lines 56 - 57, The startup currently awaits load_catalog(&config.extension_catalog).await which places OCI catalog fetch on the critical startup path; to harden this, make load_catalog emit clear structured logs (registry host, ref, attempt count and durations) and retry/backoff info, move the fetch off the hard bind path by returning a placeholder/default catalog immediately and performing the OCI pull asynchronously (or implement a startup grace/retry loop) and add a local last-known-good cache fallback used when the OCI pull fails; update references to load_catalog, config.extension_catalog and session_store so the server binds/listens before long OCI pulls complete and ensure failures are logged with the host/ref/attempt details for diagnosability.
🤖 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 `@mcp-server/src/catalog/mod.rs`:
- Around line 262-269: chart_basename() returns names with OCI tags or digests
(e.g., "dolos:1.2.3" or "dolos@sha256:...") causing validate_extensions() to
reject valid references; update chart_basename to strip any trailing tag
(":tag") or digest ("`@sha256`:...") after removing the "oci://" prefix and
trailing slashes before taking the last path segment so it returns the pure
basename. Locate the chart_basename function and: after strip_prefix("oci://")
and trim_end_matches('/'), remove a trailing '@...' digest or ':...' tag (handle
the first occurrence of '@' or ':' in the final segment), then rsplit('/') and
return the filtered non-empty name; add regression tests covering both ":tag"
and "`@sha256`:" forms to ensure validate_extensions() accepts them.
In `@mcp-server/src/catalog/oci.rs`:
- Around line 19-20: fetch_catalog_json currently creates the reqwest client
with Client::new(), leaving timeouts to defaults; change it to build a client
via reqwest::Client::builder() and set explicit connect_timeout and overall
timeout using std::time::Duration (e.g., Duration::from_secs for both) before
calling .build(), then use that client variable where fetch_manifest(&client,
&reference).await? is invoked; ensure you add use std::time::Duration to the
imports and keep the variable name client so fetch_manifest and other callers
continue to work.
In `@mcp-server/src/config.rs`:
- Around line 101-106: The env var value for MCP_EXTENSION_CATALOG_OCI_REF is
filtered with !value.trim().is_empty() but the untrimmed string is stored in
oci_ref; change the env::var handling to trim and store the trimmed string (e.g.
map the Ok value to value.trim().to_string() or .to_owned() before applying
.ok().filter(...)) so oci_ref contains the trimmed Option<String>, leaving the
existing check that returns ConfigError::MissingExtensionCatalogOciRef when
source == ExtensionCatalogSource::Oci unchanged.
In `@mcp-server/src/tools/workloads/registry.rs`:
- Around line 18-31: installed_extension_ids is using release.chart.name but
then calls catalog.get(extension_id) which expects an extension ID, causing
misses when chart names differ from IDs; update installed_extension_ids to
resolve by chart reference instead of ID—e.g., replace the
catalog.get(extension_id) call with a lookup that finds a catalog entry by its
chart name (use an existing method like catalog.get_by_chart_name or iterate
catalog entries and match their chart/name field against release.chart.name),
keep filtering by deployed status and return the matched extension IDs via their
canonical ID field.
---
Outside diff comments:
In `@extensions/control-plane/README.md`:
- Line 364: The README entry for supernodeMcp.persistence.size must match the
actual default in values.yaml; update either the README table row or the default
in extensions/control-plane/values.yaml so both use the same value (choose 10Gi
if the PVC should be larger or change the values.yaml default to 1Gi if the
README is correct). Specifically, reconcile the symbol
supernodeMcp.persistence.size by editing the README row that currently shows
`1Gi` or the values.yaml setting at the supernodeMcp.persistence.size key so
they are identical and reflect the intended operator default.
---
Nitpick comments:
In `@mcp-server/src/config.rs`:
- Around line 97-114: Add unit tests that cover the validation branches in
extension_catalog_config, extension_catalog_max_bytes, and
extension_catalog_allow_untrusted by either refactoring those helpers to accept
inputs (e.g., pass source/oci_ref/max_bytes/allow_untrusted strings into parsing
functions) or by serializing tests that mutate process env vars using a
test-wide mutex/guard; specifically exercise the case where source ==
ExtensionCatalogSource::Oci with missing MCP_EXTENSION_CATALOG_OCI_REF to
trigger ConfigError::MissingExtensionCatalogOciRef, invalid and empty
MCP_EXTENSION_CATALOG_MAX_BYTES to exercise parse/default behavior, and
true/false/empty/invalid MCP_EXTENSION_CATALOG_ALLOW_UNTRUSTED values to verify
boolean parsing and defaults.
In `@mcp-server/src/server.rs`:
- Around line 56-57: The startup currently awaits
load_catalog(&config.extension_catalog).await which places OCI catalog fetch on
the critical startup path; to harden this, make load_catalog emit clear
structured logs (registry host, ref, attempt count and durations) and
retry/backoff info, move the fetch off the hard bind path by returning a
placeholder/default catalog immediately and performing the OCI pull
asynchronously (or implement a startup grace/retry loop) and add a local
last-known-good cache fallback used when the OCI pull fails; update references
to load_catalog, config.extension_catalog and session_store so the server
binds/listens before long OCI pulls complete and ensure failures are logged with
the host/ref/attempt details for diagnosability.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64e1c245-85db-413f-ac42-cf5bee00e570
📒 Files selected for processing (30)
catalog/README.mdcatalog/extension-catalog.jsonextensions/control-plane/README.mdextensions/control-plane/templates/stage4-04-statefulset-supernode-mcp.yamlextensions/control-plane/values.yamlmcp-server/src/catalog/apex_fusion_block_producer.rsmcp-server/src/catalog/apex_fusion_relay.rsmcp-server/src/catalog/cardano_block_producer.rsmcp-server/src/catalog/cardano_relay.rsmcp-server/src/catalog/dolos.rsmcp-server/src/catalog/extension.rsmcp-server/src/catalog/hydra_node.rsmcp-server/src/catalog/mod.rsmcp-server/src/catalog/oci.rsmcp-server/src/catalog/schema.rsmcp-server/src/catalog/source.rsmcp-server/src/config.rsmcp-server/src/errors.rsmcp-server/src/mcp.rsmcp-server/src/prompts/catalog.rsmcp-server/src/resources/router.rsmcp-server/src/server.rsmcp-server/src/tools/dynamic.rsmcp-server/src/tools/k8s_summaries.rsmcp-server/src/tools/router.rsmcp-server/src/tools/supernode.rsmcp-server/src/tools/workloads/dolos.rsmcp-server/src/tools/workloads/mod.rsmcp-server/src/tools/workloads/outputs.rsmcp-server/src/tools/workloads/registry.rs
💤 Files with no reviewable changes (8)
- mcp-server/src/catalog/apex_fusion_relay.rs
- mcp-server/src/catalog/hydra_node.rs
- mcp-server/src/catalog/schema.rs
- mcp-server/src/catalog/apex_fusion_block_producer.rs
- mcp-server/src/catalog/dolos.rs
- mcp-server/src/catalog/cardano_block_producer.rs
- mcp-server/src/tools/k8s_summaries.rs
- mcp-server/src/catalog/cardano_relay.rs
| let oci_ref = env::var("MCP_EXTENSION_CATALOG_OCI_REF") | ||
| .ok() | ||
| .filter(|value| !value.trim().is_empty()); | ||
| if source == ExtensionCatalogSource::Oci && oci_ref.is_none() { | ||
| return Err(ConfigError::MissingExtensionCatalogOciRef); | ||
| } |
There was a problem hiding this comment.
Store the trimmed oci_ref value for consistency.
The filter checks !value.trim().is_empty() but the original (untrimmed) string is preserved in oci_ref. If a user sets MCP_EXTENSION_CATALOG_OCI_REF=" oci://example.com/catalog:v1 " (e.g., from a quoted env file), the leading/trailing whitespace is kept and will likely cause confusing failures when the OCI client parses the reference.
🛠️ Proposed fix
let oci_ref = env::var("MCP_EXTENSION_CATALOG_OCI_REF")
.ok()
- .filter(|value| !value.trim().is_empty());
+ .map(|value| value.trim().to_string())
+ .filter(|value| !value.is_empty());📝 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.
| let oci_ref = env::var("MCP_EXTENSION_CATALOG_OCI_REF") | |
| .ok() | |
| .filter(|value| !value.trim().is_empty()); | |
| if source == ExtensionCatalogSource::Oci && oci_ref.is_none() { | |
| return Err(ConfigError::MissingExtensionCatalogOciRef); | |
| } | |
| let oci_ref = env::var("MCP_EXTENSION_CATALOG_OCI_REF") | |
| .ok() | |
| .map(|value| value.trim().to_string()) | |
| .filter(|value| !value.is_empty()); | |
| if source == ExtensionCatalogSource::Oci && oci_ref.is_none() { | |
| return Err(ConfigError::MissingExtensionCatalogOciRef); | |
| } |
🤖 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 `@mcp-server/src/config.rs` around lines 101 - 106, The env var value for
MCP_EXTENSION_CATALOG_OCI_REF is filtered with !value.trim().is_empty() but the
untrimmed string is stored in oci_ref; change the env::var handling to trim and
store the trimmed string (e.g. map the Ok value to value.trim().to_string() or
.to_owned() before applying .ok().filter(...)) so oci_ref contains the trimmed
Option<String>, leaving the existing check that returns
ConfigError::MissingExtensionCatalogOciRef when source ==
ExtensionCatalogSource::Oci unchanged.
| catalog.get(release.chart.name.as_deref()?) | ||
| } | ||
|
|
||
| pub(crate) fn extension_id_for_chart(chart_name: Option<&str>) -> Option<&'static str> { | ||
| match chart_name { | ||
| Some(APEX_FUSION_RELAY_CHART_NAME) => Some(APEX_FUSION_RELAY_EXTENSION_ID), | ||
| Some(APEX_FUSION_BLOCK_PRODUCER_CHART_NAME) => { | ||
| Some(APEX_FUSION_BLOCK_PRODUCER_EXTENSION_ID) | ||
| } | ||
| Some(CARDANO_RELAY_CHART_NAME) => Some(CARDANO_RELAY_EXTENSION_ID), | ||
| Some(CARDANO_BLOCK_PRODUCER_CHART_NAME) => Some(CARDANO_BLOCK_PRODUCER_EXTENSION_ID), | ||
| Some(DOLOS_CHART_NAME) => Some(DOLOS_EXTENSION_ID), | ||
| Some(HYDRA_NODE_CHART_NAME) => Some(HYDRA_NODE_EXTENSION_ID), | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn installed_extension_ids(releases: &[HelmReleaseSummary]) -> BTreeSet<String> { | ||
| pub(crate) fn installed_extension_ids( | ||
| releases: &[HelmReleaseSummary], | ||
| catalog: &ExtensionCatalog, | ||
| ) -> BTreeSet<String> { | ||
| releases | ||
| .iter() | ||
| .filter(|release| release.status.as_deref() == Some("deployed")) | ||
| .filter_map(|release| extension_id_for_chart(release.chart.name.as_deref())) | ||
| .map(str::to_string) | ||
| .filter_map(|release| release.chart.name.as_deref()) | ||
| .filter(|extension_id| catalog.get(extension_id).is_some()) | ||
| .map(ToString::to_string) | ||
| .collect() |
There was a problem hiding this comment.
Resolve releases by chart reference, not extension.id.
release.chart.name is a chart identifier, but catalog.get(...) is an ID lookup. If IDs diverge from chart names, catalog-managed workloads stop resolving (dynamic tools/metrics/catalogExtension mapping drop unexpectedly).
Proposed fix
pub(crate) fn extension_for_release<'a>(
release: &HelmReleaseSummary,
catalog: &'a ExtensionCatalog,
) -> Option<&'a ExtensionDefinition> {
- catalog.get(release.chart.name.as_deref()?)
+ let chart_name = release.chart.name.as_deref()?;
+ catalog
+ .list()
+ .find(|extension| extension.chart.rsplit('/').next() == Some(chart_name))
}
pub(crate) fn installed_extension_ids(
releases: &[HelmReleaseSummary],
catalog: &ExtensionCatalog,
) -> BTreeSet<String> {
releases
.iter()
.filter(|release| release.status.as_deref() == Some("deployed"))
- .filter_map(|release| release.chart.name.as_deref())
- .filter(|extension_id| catalog.get(extension_id).is_some())
- .map(ToString::to_string)
+ .filter_map(|release| {
+ extension_for_release(release, catalog).map(|extension| extension.id.clone())
+ })
.collect()
}🤖 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 `@mcp-server/src/tools/workloads/registry.rs` around lines 18 - 31,
installed_extension_ids is using release.chart.name but then calls
catalog.get(extension_id) which expects an extension ID, causing misses when
chart names differ from IDs; update installed_extension_ids to resolve by chart
reference instead of ID—e.g., replace the catalog.get(extension_id) call with a
lookup that finds a catalog entry by its chart name (use an existing method like
catalog.get_by_chart_name or iterate catalog entries and match their chart/name
field against release.chart.name), keep filtering by deployed status and return
the matched extension IDs via their canonical ID field.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp-server/src/catalog/oci.rs (1)
47-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify digest-pinned manifest integrity in
fetch_manifest
mcp-server/src/catalog/oci.rs’sfetch_manifestdownloads and JSON-decodes the manifest but never checks that the returned manifest bytes hash to the digest from anoci://...@sha256:<digest>reference (digest verification is only applied to the catalog blob afterward). Updatefetch_manifestto read manifest bytes, verify SHA-256 against the digest when the reference is digest-pinned (and reject unsupported digests viaUnsupportedDigest), then parse JSON—adding anOciCatalogErrorvariant for invalid manifest JSON since the current enum only mapsreqwest::Error.Also consider avoiding
fetch_blob’s full buffering (response.bytes().await?.to_vec()) before enforcingmax_bytes; bounded/streaming reads would prevent large allocations.🤖 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 `@mcp-server/src/catalog/oci.rs` around lines 47 - 60, The fetch_manifest function must verify digest-pinned manifests: when OciReference contains a digest (e.g., sha256:<digest>), read the raw response bytes from client.get(reference.manifest_url()), compute and compare the SHA-256 hash to the reference digest and return OciCatalogError::UnsupportedDigest for unknown algorithms or a new OciCatalogError variant (e.g., InvalidManifestJson) when JSON parsing fails; only after a successful digest check should you parse bytes into OciManifest. Update the OciCatalogError enum to include UnsupportedDigest and InvalidManifestJson variants and use them in fetch_manifest, and similarly change fetch_blob to perform bounded/streaming reads that enforce max_bytes before fully buffering to avoid large allocations (referencing fetch_blob and max_bytes).
🧹 Nitpick comments (1)
extensions/control-plane/values.yaml (1)
178-178: ⚡ Quick winConsider pinning
ociRefby digest for immutability.
oci://oci.supernode.store/extension-catalog:0.1.0is a tag reference, and tags are mutable in OCI registries. Since the in-process verification only checks the blob digest against what the manifest claims (the manifest itself is fetched by tag), a tag reference shifts trust onto the registry. A@sha256:<digest>reference makes the catalog immutable end-to-end:♻️ Suggested change
- ociRef: oci://oci.supernode.store/extension-catalog:0.1.0 + ociRef: oci://oci.supernode.store/extension-catalog@sha256:<digest-of-0.1.0>🤖 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 `@extensions/control-plane/values.yaml` at line 178, The ociRef in values.yaml currently uses a mutable tag ("oci://oci.supernode.store/extension-catalog:0.1.0"); replace it with an immutable digest form (OCI manifest reference) like "oci://oci.supernode.store/extension-catalog@sha256:<digest>" so the catalog is pinned end-to-end. Locate the ociRef key for the extension-catalog entry, fetch the registry manifest to obtain the correct sha256 digest, and substitute the :0.1.0 tag with `@sha256`:<digest>; ensure any deployment/config tooling that consumes ociRef accepts the digest form and update docs or CI that publishes this value when you publish new releases.
🤖 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.
Outside diff comments:
In `@mcp-server/src/catalog/oci.rs`:
- Around line 47-60: The fetch_manifest function must verify digest-pinned
manifests: when OciReference contains a digest (e.g., sha256:<digest>), read the
raw response bytes from client.get(reference.manifest_url()), compute and
compare the SHA-256 hash to the reference digest and return
OciCatalogError::UnsupportedDigest for unknown algorithms or a new
OciCatalogError variant (e.g., InvalidManifestJson) when JSON parsing fails;
only after a successful digest check should you parse bytes into OciManifest.
Update the OciCatalogError enum to include UnsupportedDigest and
InvalidManifestJson variants and use them in fetch_manifest, and similarly
change fetch_blob to perform bounded/streaming reads that enforce max_bytes
before fully buffering to avoid large allocations (referencing fetch_blob and
max_bytes).
---
Nitpick comments:
In `@extensions/control-plane/values.yaml`:
- Line 178: The ociRef in values.yaml currently uses a mutable tag
("oci://oci.supernode.store/extension-catalog:0.1.0"); replace it with an
immutable digest form (OCI manifest reference) like
"oci://oci.supernode.store/extension-catalog@sha256:<digest>" so the catalog is
pinned end-to-end. Locate the ociRef key for the extension-catalog entry, fetch
the registry manifest to obtain the correct sha256 digest, and substitute the
:0.1.0 tag with `@sha256`:<digest>; ensure any deployment/config tooling that
consumes ociRef accepts the digest form and update docs or CI that publishes
this value when you publish new releases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69c2e052-55bd-4497-b7b6-5e5569722af5
📒 Files selected for processing (3)
extensions/control-plane/values.yamlmcp-server/src/catalog/mod.rsmcp-server/src/catalog/oci.rs
Summary by CodeRabbit
New Features
Documentation
Refactor