sync from kubeflow

[Al-Pragliola]

Description

superseeds #1685

• keep fix: toolLimit query parameter has no effect on MCP servers listing endpoint kubeflow/model-registry#2493
• keep fix(ui): gracefully handle SAR forbidden for non-admin namespace registry access check kubeflow/model-registry#2494
• keep build(deps-dev): bump requests from 2.32.5 to 2.33.0 in /clients/python kubeflow/model-registry#2490
• keep build(deps): bump requests from 2.32.5 to 2.33.0 in /jobs/async-upload kubeflow/model-registry#2489
• keep build(deps): bump handlebars from 4.7.8 to 4.7.9 in /clients/ui/frontend kubeflow/model-registry#2491
• keep build(deps): bump node-forge from 1.3.1 to 1.4.0 in /clients/ui/frontend kubeflow/model-registry#2492
• keep build(deps): bump yaml from 1.10.2 to 1.10.3 in /clients/ui/frontend kubeflow/model-registry#2488
• keep build(deps-dev): bump brace-expansion from 1.1.12 to 1.1.13 in /clients/ui/frontend kubeflow/model-registry#2495
• keep build(deps): bump azure/setup-helm from 4.3.1 to 5.0.0 kubeflow/model-registry#2501
• keep build(deps): bump mikefarah/yq from 4.52.4 to 4.52.5 kubeflow/model-registry#2502
• keep build(deps): bump golang.org/x/sync from 0.19.0 to 0.20.0 kubeflow/model-registry#2505
• keep fix(catalog): replace phantom yamlMCPArtifact struct with generated openapi.MCPArtifact kubeflow/model-registry#2512
• keep build(deps): bump github.com/testcontainers/testcontainers-go/modules/mysql from 0.40.0 to 0.41.0 kubeflow/model-registry#2504
• keep fix(ui): update settings nav items and page titles microcopy kubeflow/model-registry#2517
• keep feat(ui): add copy-to-clipboard button to markdown code blocks kubeflow/model-registry#2496
• keep KEP-0003: Technical implementation strategy for Kubeflow Hub rename kubeflow/model-registry#2239
• keep build(deps): bump actions/setup-go from 6.3.0 to 6.4.0 kubeflow/model-registry#2503
• merge commit

How Has This Been Tested?

Merge criteria:

• All the commits have been signed-off (To pass the DCO check)

• The commits have meaningful messages
• Automated tests are provided as part of the PR for major new functionalities; testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
• The developer has manually tested the changes and verified that the changes work.
• Code changes follow the kubeflow contribution guidelines.
• For first time contributors: Please reach out to the Reviewers to ensure all tests are being run, ensuring the label ok-to-test has been added to the PR.

If you have UI changes

• The developer has added tests or explained why testing cannot be added.
• Included any necessary screenshots or gifs if it was a UI change.
• Verify that UI/UX changes conform the UX guidelines for Kubeflow.

Summary by CodeRabbit

• New Features

  • Added pagination support for MCP server tool queries
  • Introduced clipboard copy button for code blocks in Markdown content

• Improvements

  • Enhanced namespace access validation with graceful handling when permissions cannot be determined; users now see informational alerts instead of errors in such cases
  • Standardized page titles and labels for consistency across settings pages

• Refactor

  • Refactored Markdown code block rendering component

• Chores

  • Updated GitHub Actions workflow step versions
  • Updated Go and Python dependencies
  • Added KEP documentation for product renaming strategy

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request updates GitHub Actions workflow versions (setup-go and setup-helm), implements pagination support for MCP server tools in the catalog service, introduces a new "cannotCheck" state for Kubernetes namespace registry access checks, refactors frontend Markdown code block rendering with clipboard copy functionality, updates Go and Python dependencies, and adds a KEP document detailing the "Kubeflow Model Registry" to "Kubeflow Hub" renaming strategy. No exported API contracts are substantially altered except for new optional fields and props throughout the frontend layer.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

---

Security & Code Quality Issues

1. Incomplete Permission Check Coverage in RegisterAndStoreFields.tsx

The form section rendering logic now permits submission when namespaceCannotCheck is true:

renders when formData.namespace is set and either 
namespaceHasAccess === true OR namespaceCannotCheck is truthy

Risk: When cannotCheck === true, the system cannot verify if the user actually has registry access. Allowing form progression under this condition creates a permission bypass window—the user may lack SAR permissions but proceed anyway, resulting in failures deeper in the stack rather than upfront denial.

Recommendation: Explicitly document and test the expected behavior when cannotCheck === true. If this is intentional (optimistic submission), add guards at the backend handler level. If not, consider blocking submission until access can be verified or the user is explicitly notified of the risk.

---

2. Clipboard Write Error Handling in CodeBlockComponent.tsx

The clipboard copy handler updates UI state (copied) on success but the failure path in tests only checks that the button remains accessible via waitFor—no verification that error state is logged or surfaced:

mocked writeText rejects → only verify button is still present

Risk: Silent failures in clipboard operations could confuse users (button appears clickable but nothing happens) without diagnostic information. No logging of clipboard API failures detected.

Recommendation: Add error logging and consider surfacing non-fatal clipboard failures to the user via a transient notification.

---

3. Go Module Indirect Dependency Removed

The diff removes github.com/cenkalti/backoff/v5 v5.0.3 // indirect from go.mod without explanation. If this dependency was previously required by a transitive import, this could indicate a version bump resolved an unused dependency—or a genuine gap in dependency tracking.

Recommendation: Verify that go mod tidy produces no errors and that no runtime imports of backoff/v5 exist in the codebase post-merge.

---

4. Markdown Pre-Block Text Extraction Logic

In MarkdownComponent.tsx, the new pre override flattens React children and concatenates only string segments:

flatten nested React children → concatenate string segments only

Risk: If children contain React components rendering user-controlled data, this approach discards them silently. Conversely, if malicious content is embedded as a string child via Markdown parser output, it reaches the clipboard as-is. Verify that the Markdown parser (react-markdown) properly escapes untrusted content.

Recommendation: Confirm escape strategy for Markdown input. If user-supplied Markdown is parsed, ensure no XSS vector exists through code block content.

---

5. KEP Renaming Strategy Go Module Path Change

The KEP proposals/KEP-0003-renaming-strategy/README.md specifies a major version bump and Go module path change (kubeflow/model-registry → kubeflow/hub). This is a breaking change for all Go consumers.

Risk: No migration tooling mentioned for automatic module rewrite. This could strand users on outdated versions if the cutover is not coordinated.

Recommendation: Provide concrete go mod migration instructions and consider a phased approach (dual import paths during a transition period).

🚥 Pre-merge checks | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	PR description is entirely boilerplate template with no substantive change narrative, testing details, or checklist completion.	Fill in the Description section with summary of changes, explain testing approach, and check off applicable merge criteria items to indicate review readiness.
Title check	❓ Inconclusive	Title is vague and does not clearly describe the primary change in the changeset.	Replace with a descriptive title that summarizes the main change, e.g., 'Add namespace access cannot-check handling and MCP pagination'.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Al-Pragliola, fege

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [Al-Pragliola,fege]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[fege]

/lgtm

[coderabbitai]

Actionable comments posted: 13

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)

catalog/internal/catalog/mcpcatalog/providers.go (1)

354-362: ⚠️ Potential issue | 🟠 Major

Validate artifact timestamp fields before persisting (CWE-20, CWE-345).

Severity: Major.
At Line 356 only artifact.Uri is validated, but at Line 360 the entire apimodels.MCPArtifact payload is persisted. This now includes CreateTimeSinceEpoch and LastUpdateTimeSinceEpoch from YAML input. Exploit scenario: a malicious/compromised catalog source can inject forged or malformed timestamp values, polluting provenance metadata and breaking consumers that assume epoch-millis format.

Remediation diff

@@
 	// Validate and convert artifacts to JSON
 	if len(ys.Artifacts) > 0 {
 		for i, artifact := range ys.Artifacts {
 			if err := basecatalog.ValidateArtifactURI(artifact.Uri); err != nil {
 				return MCPServerProviderRecord{Error: fmt.Errorf("server %q artifact %d: %w", ys.Name, i, err)}
 			}
+			if err := validateEpochMillisField("createTimeSinceEpoch", artifact.CreateTimeSinceEpoch); err != nil {
+				return MCPServerProviderRecord{Error: fmt.Errorf("server %q artifact %d: %w", ys.Name, i, err)}
+			}
+			if err := validateEpochMillisField("lastUpdateTimeSinceEpoch", artifact.LastUpdateTimeSinceEpoch); err != nil {
+				return MCPServerProviderRecord{Error: fmt.Errorf("server %q artifact %d: %w", ys.Name, i, err)}
+			}
 		}
 		if jsonBytes, err := json.Marshal(ys.Artifacts); err == nil {
 			properties = append(properties, mrmodels.NewStringProperty("artifacts", string(jsonBytes), false))
 		}
 	}
@@
+func validateEpochMillisField(field string, v *string) error {
+	if v == nil {
+		return nil
+	}
+	n, err := strconv.ParseInt(*v, 10, 64)
+	if err != nil || n < 0 {
+		return fmt.Errorf("%s must be a non-negative epoch-millis string", field)
+	}
+	return nil
+}

As per coding guidelines, “Security vulnerabilities (provide severity, exploit scenario, and remediation code)” and “Bug-prone patterns and error handling gaps” are review priorities.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@catalog/internal/catalog/mcpcatalog/providers.go` around lines 354 - 362, The
code validates only artifact.Uri for each element in ys.Artifacts but then
persists the full apimodels.MCPArtifact (including CreateTimeSinceEpoch and
LastUpdateTimeSinceEpoch) into properties via
mrmodels.NewStringProperty("artifacts", ...), which allows forged or malformed
timestamps to be stored; update the validation loop (iterating ys.Artifacts) to
also validate and canonicalize timestamp fields (CreateTimeSinceEpoch and
LastUpdateTimeSinceEpoch) for correct epoch-millis format/range before
marshalling: add checks in the same block that use a helper (e.g.,
validateArtifactTimestamps or inline validation inside the loop) to reject or
normalize invalid timestamps and return MCPServerProviderRecord{Error: ...} on
failure (similar to the existing basecatalog.ValidateArtifactURI handling), and
only marshal/persist the artifacts into properties after timestamps are
validated/normalized to ensure consumers receive safe epoch-millis values.

.github/workflows/csi-test.yml (2)

42-44: ⚠️ Potential issue | 🟠 Major

Avoid direct interpolation of event data in run blocks (CWE-94).

${{ github.event.after }} is interpolated directly into the shell script. Use an environment variable instead to prevent potential script injection attacks. As per coding guidelines, never interpolate event data directly in run blocks.

Recommended fix

       - name: Generate tag
         shell: bash
         id: tags
+        env:
+          COMMIT_SHA: ${{ github.event.after }}
         run: |
-          commit_sha=${{ github.event.after }}
+          commit_sha="${COMMIT_SHA}"
           tag=main-${commit_sha:0:7}
           echo "tag=${tag}" >> $GITHUB_OUTPUT

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/csi-test.yml around lines 42 - 44, The workflow currently
interpolates github.event.after directly into the run block; instead define an
environment variable (e.g., COMMIT_SHA) using ${{ github.event.after }} at the
job or step level and then reference that env var inside the run script (replace
direct `${{ github.event.after }}` usage with `$COMMIT_SHA`), e.g., set
commit_sha from $COMMIT_SHA and keep tag=main-${commit_sha:0:7} and the echo to
$GITHUB_OUTPUT; ensure the env var is quoted when used to avoid word-splitting
or injection.

---

72-72: ⚠️ Potential issue | 🟠 Major

Pin Docker image by digest, not tag (CWE-494).

The KinD node image uses a version tag instead of a digest. Tags are mutable and can be overwritten, enabling supply chain attacks. As per coding guidelines, pin Docker images by SHA256 digest.

Example fix

-          node_image: "kindest/node:v1.33.7"
+          node_image: "kindest/node:v1.33.7@sha256:<digest_here>"

Retrieve the digest:

docker pull kindest/node:v1.33.7
docker inspect kindest/node:v1.33.7 --format='{{.RepoDigests}}'

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/csi-test.yml at line 72, The node_image entry currently
pins the KinD node by mutable tag ("kindest/node:v1.33.7"); replace it with the
image digest (sha256) to prevent tampering: pull the image and get its
RepoDigest (e.g., via docker pull and docker inspect) and then update the
node_image value from "kindest/node:v1.33.7" to the fully qualified digest form
"kindest/node@sha256:..." so the workflow uses the immutable digest instead of
the tag.

.github/workflows/controller-test.yml (1)

38-40: ⚠️ Potential issue | 🟠 Major

Avoid direct interpolation of event data in run blocks (CWE-94).

${{ github.event.after }} is interpolated directly into the shell script. Use an environment variable to prevent potential script injection. As per coding guidelines, never interpolate event data directly in run blocks.

Recommended fix

       - name: Generate tag
         shell: bash
         id: tags
+        env:
+          COMMIT_SHA: ${{ github.event.after }}
         run: |
-          commit_sha=${{ github.event.after }}
+          commit_sha="${COMMIT_SHA}"
           tag="${BRANCH:-main}"-${commit_sha:0:7}
           echo "tag=${tag}" >> $GITHUB_OUTPUT

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/controller-test.yml around lines 38 - 40, The step
currently injects github.event.after directly into the run script
(commit_sha=${{ github.event.after }}), which risks script injection; instead
declare an environment variable in the workflow step (e.g., COMMIT_SHA) assigned
to ${{ github.event.after }} and then use that safe shell variable inside the
run block (replace uses of commit_sha and the tag assignment to reference
$COMMIT_SHA), finally write the computed tag to $GITHUB_OUTPUT as before; update
references to commit_sha and tag in the run block to use the new env var names
to avoid direct interpolation.

clients/ui/frontend/src/app/hooks/useCheckNamespaceRegistryAccess.ts (1)

54-58: ⚠️ Potential issue | 🟡 Minor

cannotCheck not reset in error path may cause stale state.

If a previous API call succeeded with cannotCheck: true, then inputs change and the new call throws an error, cannotCheck remains true while error is set. This is inconsistent—an errored state shouldn't retain a previous "cannot check" flag.

Suggested fix

       } catch (e) {
         if (!cancelled) {
           setError(e instanceof Error ? e : new Error(String(e)));
           setHasAccess(undefined);
+          setCannotCheck(false);
         }
       } finally {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@clients/ui/frontend/src/app/hooks/useCheckNamespaceRegistryAccess.ts` around
lines 54 - 58, The catch block in the useCheckNamespaceRegistryAccess hook is
not resetting the cannotCheck flag, so when an earlier call set cannotCheck=true
and a subsequent call errors the stale flag remains; update the catch handler in
useCheckNamespaceRegistryAccess to clear cannotCheck (call setCannotCheck(false)
or reset it to the intended default) along with setting error and
setHasAccess(undefined) so the hook's state remains consistent after errors.

🧹 Nitpick comments (6)

clients/ui/frontend/src/__tests__/cypress/cypress/pages/modelCatalogSettings.ts (1)

75-77: Remove { force: true } to avoid masking actionability issues.

The force option bypasses Cypress's actionability checks (visibility, enabled state, event handler presence). If the toggle becomes obscured or disabled in the UI, this test will pass but real users cannot interact with it.

♻️ Proposed fix

  toggleEnable() {
-    this.findEnableToggle().click({ force: true });
+    this.findEnableToggle().click();
    return this;
  }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@clients/ui/frontend/src/__tests__/cypress/cypress/pages/modelCatalogSettings.ts`
around lines 75 - 77, The test helper method toggleEnable currently forces a
click which masks Cypress actionability issues; update toggleEnable to call
this.findEnableToggle().click() without the { force: true } option so Cypress
verifies visibility/enabled state and surface real UI problems, and keep the
method returning this; reference the methods toggleEnable and findEnableToggle
to locate and modify the implementation.

go.mod (1)

23-25: Testcontainers module versions inconsistent but not breaking

Line 25 (modules/postgres v0.40.0) is behind lines 23-24 (v0.41.0). While inconsistent, testcontainers-go modules do not enforce strict version alignment with core or sibling modules—they are designed to work across compatible version ranges. Align versions for consistency and reduced maintenance friction, but this is not a compatibility blocker.

Suggested fix

-	github.com/testcontainers/testcontainers-go/modules/postgres v0.40.0
+	github.com/testcontainers/testcontainers-go/modules/postgres v0.41.0

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@go.mod` around lines 23 - 25, The go.mod entry for
"github.com/testcontainers/testcontainers-go/modules/postgres v0.40.0" is
inconsistent with the other testcontainers entries at v0.41.0; update the
postgres module reference to v0.41.0 (matching
"github.com/testcontainers/testcontainers-go" and
"github.com/testcontainers/testcontainers-go/modules/mysql") and run a go mod
tidy or `go get
github.com/testcontainers/testcontainers-go/modules/postgres@v0.41.0` to ensure
the dependency graph is updated and go.sum regenerated.

proposals/KEP-0003-renaming-strategy/README.md (2)

276-292: Documentation quality: Code blocks missing language specifiers.

Lines 276-280 and 286-292 contain code blocks without language identifiers, reducing readability and breaking syntax highlighting.

Proposed fix

 #### Implementation Strategy
 
-```
+```yaml
 # CI workflow change - publish to new registry only
 - name: Push to new registry (kubeflow/hub)
   run: docker push ghcr.io/kubeflow/hub/server:${{ env.VERSION }}

Consumer Migration Path

Users must update their manifests before upgrading to the new version:

- +yaml

Before (old versions use legacy registry)

image: ghcr.io/kubeflow/model-registry/server:v0.2.x

After (new versions use new registry exclusively)

image: ghcr.io/kubeflow/hub/server:v0.3.0

As per coding guidelines, this aligns with documentation quality standards. Flagged by static analysis: markdownlint MD040.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@proposals/KEP-0003-renaming-strategy/README.md` around lines 276 - 292, The
Markdown code fences around the CI step ("docker push
ghcr.io/kubeflow/hub/server:${{ env.VERSION }}") and the image examples ("image:
ghcr.io/kubeflow/model-registry/server:..." and "image:
ghcr.io/kubeflow/hub/server:...") lack language specifiers; update both fenced
code blocks to use ```yaml so syntax highlighting and markdownlint MD040 are
satisfied (i.e., change the three-backtick openings before the CI snippet and
before the manifest examples to ```yaml).

---

494-494: Operational fragility: Hardcoded person reference for critical task.

Line 494 states "update icon by raising CNCF ticket (Matteo can do it)" - relying on a specific individual by first name creates operational risk:

• What if Matteo is unavailable during execution?
• No fallback or escalation path specified
• New contributors won't know who "Matteo" refers to

Recommendation: Replace with role-based assignment:

-update icon by raising CNCF ticket (Matteo can do it)
+update icon by raising CNCF ticket (maintainer with CNCF access rights)

Or specify the process: "Submit logo update request to CNCF Service Desk (requires maintainer credentials) - see [internal runbook]"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@proposals/KEP-0003-renaming-strategy/README.md` at line 494, The README
contains a hardcoded person reference "update icon by raising CNCF ticket
(Matteo can do it)"; replace that text with a role-based instruction or process
link: change the line "update icon by raising CNCF ticket (Matteo can do it)" to
something like "submit logo update request to CNCF Service Desk (requires
maintainer credentials) — see [internal runbook]" and/or assign to a role (e.g.,
"Owner: website-maintainers; Backup: sig-website-lead") and include an
escalation/fallback contact or link to the runbook so contributors know who/what
to contact if the primary is unavailable. Ensure the new wording appears where
the current string is used so it removes the personal name and adds
role/contact/runbook details.

clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registerAndStoreFields.cy.ts (1)

257-260: Replace hardcoded API version in intercept route.

Line 257 hardcodes /api/v1/... while this file already centralizes API version via MODEL_REGISTRY_API_VERSION. This test will drift on version changes.

Proposed diff

-    cy.intercept('POST', '**/api/v1/check-namespace-registry-access', {
+    cy.intercept(
+      'POST',
+      `**/api/${MODEL_REGISTRY_API_VERSION}/check-namespace-registry-access`,
+      {
       statusCode: 200,
       body: { data: { hasAccess: false, cannotCheck: true } },
-    }).as('checkNamespaceAccessCannotCheck');
+      },
+    ).as('checkNamespaceAccessCannotCheck');

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registerAndStoreFields.cy.ts`
around lines 257 - 260, Replace the hardcoded API version in the cy.intercept
route with the centralized constant: update the cy.intercept call in the test
(the one registering 'checkNamespaceAccessCannotCheck') to build the URL using
MODEL_REGISTRY_API_VERSION (instead of '/api/v1') so the path becomes
`**/api/${MODEL_REGISTRY_API_VERSION}/check-namespace-registry-access`; keep the
same statusCode/body and alias but reference MODEL_REGISTRY_API_VERSION where
the version is currently hardcoded.

clients/ui/frontend/src/concepts/k8s/NamespaceSelectorField/NamespaceSelectorField.tsx (1)

197-208: Undefined registryName renders awkwardly.

When cannotCheck is true but registryName is undefined/empty, the alert message becomes "Make sure this namespace has access to the registry before proceeding" - missing the registry name context.

Consider providing a fallback:

Suggested fix

-          Make sure this namespace has access to the {registryName} registry before proceeding, or
+          Make sure this namespace has access to the {registryName || 'model'} registry before proceeding, or
           the model storage job will fail.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@clients/ui/frontend/src/concepts/k8s/NamespaceSelectorField/NamespaceSelectorField.tsx`
around lines 197 - 208, The alert message in NamespaceSelectorField renders
awkwardly when registryName is undefined; update the Alert rendering logic
(within NamespaceSelectorField where selectedNamespace && !isLoading &&
cannotCheck is checked) to use a safe fallback for registryName (e.g.,
registryName || "registry" or "the selected registry") or conditionally change
the sentence to omit "the {registryName} registry" when registryName is empty so
the message reads coherently; ensure you reference the registryName variable in
the Alert content and keep the existing data-testid and conditional display
logic intact.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@catalog/internal/catalog/mcpcatalog/service/mcp_server_tool.go`:
- Around line 75-78: The List method currently only applies PageSize and ignores
Pagination.OrderBy, Pagination.SortOrder and Pagination.NextPageToken, causing
non-deterministic paging; update the List implementation to either (A) respect
ordering and cursor semantics by applying deterministic ordering (reuse
ApplyNameOrdering used by MCPServerList) and implement NextPageToken cursor
handling consistent with MCPServerList, or (B) explicitly reject/omit
OrderBy/SortOrder/NextPageToken by returning an error or documenting the
limitation; locate the logic in the List function and modify the handling of
listOptions.Pagination (OrderBy, SortOrder, NextPageToken) to follow one of
these two approaches so paging becomes deterministic and callers are informed.

In
`@clients/ui/frontend/src/app/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx`:
- Around line 67-68: Validate the API response booleans before assigning to
state: in the useCheckNamespaceRegistryAccess result handling, check that
result.cannotCheck and result.hasAccess are actually booleans (e.g., typeof
result.cannotCheck === "boolean") or validate the response with a lightweight
schema (zod/io-ts) and only then call setCannotCheck(...) and setHasAccess(...);
if validation fails, set a safe default (false) or handle the error path so
mrName/registryNamespace/formData.namespace logic never assigns non-boolean
values to state.

In `@clients/ui/frontend/src/app/shared/markdown/MarkdownComponent.tsx`:
- Around line 63-72: The <pre> renderer in MarkdownComponent.tsx drops raw <pre>
text because it only grabs immediate string children via the current
React.Children.toArray(...).map(...).join(''), so change the pre rendering logic
(the anonymous pre: ({ children }) => { ... } block) to recursively extract text
from React nodes and nested elements (e.g., via a helper like
collectTextFromReactNode(node): string) so direct-string children like
<pre>kubectl get pods</pre> and nested nodes both produce the full code string
before passing it into CodeBlockComponent; also add a regression test that feeds
a direct-string <pre> input and asserts CodeBlockComponent receives the expected
"kubectl get pods" text.
- Around line 74-77: The custom `code` renderer in MarkdownComponent.tsx is
spreading non-DOM props (like `node`, `inline`) onto the native <code> element;
update the renderer (the `code: ({ className, children, ...props }) => ...`
handler) to destructure and remove `node` and other non-DOM props (e.g., `node`,
`inline`, `level`) before spreading the remaining valid DOM props onto the
<code> element so React warnings are eliminated and only valid attributes are
applied.

In `@jobs/async-upload/pyproject.toml`:
- Line 15: The pyproject.toml currently pins requests with the caret range
"requests = \"^2.33.0\"" which includes a release (2.33.0) with known security
advisories; update that dependency spec to a fixed, non-vulnerable release
(e.g., "requests = \"==2.33.1\"") in the pyproject.toml entry and run your
dependency resolver/lock (poetry lock or pip-compile) to regenerate lockfiles,
then verify the chosen version against OSV/GHSA; if advisories still appear for
2.33.1, either upgrade to the next patched release or replace requests with an
alternate HTTP client or apply vendor patches before merging.

In `@proposals/KEP-0003-renaming-strategy/README.md`:
- Around line 645-661: The Security Considerations section (headers "Image
Signing", "SBOM (Software Bill of Materials)", "Vulnerability Databases") is
high-level and missing concrete plans; update that section to add: a detailed
image-signing workflow migration plan naming who signs images, when, signing
keys rotation/ownership and how signatures are published/verified
(responsibility matrix for teams); an SBOM regeneration and distribution process
describing tools, file formats, storage location and consumer access; a CVE
coordination plan noting how to update NVD/GitHub Security Advisories and
communicate name-mapping to security researchers; a checklist to migrate
security scanning tool configurations; and a cutover timeline aligning
image-signing/SBOM/CVE updates with registry migration and release notes.
- Around line 739-748: The Implementation History under "Implementation History"
lists Alpha/Beta/Stable as "TBD", which prevents this KEP from being actionable;
update the "Alpha phase", "Beta phase", and "Stable phase" entries with concrete
target dates (choosing specific calendar dates) that respect the 60–90 day lead
time requirement called out elsewhere, and add a short coordination note
indicating planned synchronization windows with KServe, Pipelines, and Manifests
teams (or alternatively mark the KEP header/state clearly as "Draft" until those
dates and coordination commitments are finalized) so reviewers can verify the
timeline and cross-team coordination.
- Around line 190-228: The "Go Module Path Changes" section currently leaves
version choice and replace-directive strategy ambiguous; update the
"Recommendation" text (the bullet that starts "DO: Rename Go module paths..."
and the "CONSIDER: Using Go module `replace` directives temporarily" note) to:
commit to v1.0.0 as the new major version for the breaking rename, state a fixed
transition period of 6 months for supporting old imports via `replace`
directives, assign maintenance responsibility to the release lead/maintainers
(include contact or role), and define removal criteria that `replace` directives
will be deleted after 6 months or after downstream consumers have migrated
(whichever comes later) with a communicated deprecation schedule and one-month
final notice.
- Line 720: The README contains an unstable, authenticated JIRA link
(https://cncfservicedesk.atlassian.net/servicedesk/customer/portal/1/CNCFSD-3354?created=true)
which is not public and includes an ephemeral query param; replace that URL in
the line mentioning the updated logo with a stable, public reference (preferably
a GitHub issue/PR that tracks the logo change) or remove the hyperlink and
replace it with a generic description of the logo update process, and if
preserving the JIRA content is necessary, archive its relevant details into the
KEP or a linked public GitHub issue instead of linking to the private Atlassian
URL.
- Around line 688-736: The KEP currently lists "Rollback plan documented" under
the Beta exit criteria but lacks the plan; add a new "Rollback Plan" subsection
(place it under the Beta Exit Criteria or as its own section near "Graduation
Criteria") that enumerates concrete procedures for image rollback (how to revert
to ghcr.io/kubeflow/model-registry/* and restore tags/aliases), version rollback
(steps to downgrade releases and re-publish previous artifacts), Go module
rollback (consumer guidance for reverting import paths and versions),
coordination/communication steps (who to notify in kubeflow/manifests, KServe,
Pipelines and how to coordinate emergency rollbacks), decision criteria
(specific measurable triggers like % failure, error class, or security impact),
and a maximum rollback timeline (time window after which changes are considered
irreversible); reference the Beta exit criterion text ("Rollback plan
documented") and ensure the plan includes owners, commands/automation hints, and
rollback verification steps.
- Around line 531-553: The Recommendation section under "CI/CD Pipeline Changes"
contains a line ("Maintain ability to push to both old and new registries during
transition") that conflicts with the hard cutover stance elsewhere (the "DO:
Publish new version images exclusively..." language and Alternative 4 rejecting
dual publishing); resolve by choosing one approach and making consistent edits:
either remove the dual-publishing sentence and ensure all recommendations and
Alternative 4 keep the hard-cutover language, or convert the document to a
dual-publishing transition strategy and update the Recommendation, the earlier
"DO: Publish new version images exclusively..." phrasing, and Alternative 4 to
describe the dual-publish workflow, required CI changes (push to both
registries), testing steps, and rollback considerations so everything is
consistent. Ensure you update the "Recommendation" statement and the referenced
alternative text to match the chosen strategy.
- Around line 132-141: Update the Risk table entry for "Users upgrade without
updating manifests" to include concrete technical mitigations: add pre-upgrade
validation (admission webhook or preflight script), automated manifest migration
tooling (CLI or migration controller), and phased rollout/canary deployment
recommendation plus clearer liveness/readiness probe behavior to surface
image-pull failures; likewise update the "Air-gapped environments can't pull new
images" row to include providing image migration scripts/bundle tooling (OCI
image bundles, export/import scripts), mirror registry instructions and an
offline bundle build/release path so offline customers can migrate images
without manual rebuilds.
- Around line 577-603: The KEP incorrectly states that `.model-registry.yaml` is
unused; fix the document to reflect that cmd/root.go uses Viper
(viper.ReadInConfig()) to load `$HOME/.model-registry.yaml` at startup (logged
with "Using config file: "), so remove the recommendation to delete the config
file and instead document that the file is actively loaded and must not be
removed for deployments relying on it; update the "Recommendation" and
"Backwards Compatibility Impact" sections to state the file is used and advise
caution when renaming or removing it.

---

Outside diff comments:
In @.github/workflows/controller-test.yml:
- Around line 38-40: The step currently injects github.event.after directly into
the run script (commit_sha=${{ github.event.after }}), which risks script
injection; instead declare an environment variable in the workflow step (e.g.,
COMMIT_SHA) assigned to ${{ github.event.after }} and then use that safe shell
variable inside the run block (replace uses of commit_sha and the tag assignment
to reference $COMMIT_SHA), finally write the computed tag to $GITHUB_OUTPUT as
before; update references to commit_sha and tag in the run block to use the new
env var names to avoid direct interpolation.

In @.github/workflows/csi-test.yml:
- Around line 42-44: The workflow currently interpolates github.event.after
directly into the run block; instead define an environment variable (e.g.,
COMMIT_SHA) using ${{ github.event.after }} at the job or step level and then
reference that env var inside the run script (replace direct `${{
github.event.after }}` usage with `$COMMIT_SHA`), e.g., set commit_sha from
$COMMIT_SHA and keep tag=main-${commit_sha:0:7} and the echo to $GITHUB_OUTPUT;
ensure the env var is quoted when used to avoid word-splitting or injection.
- Line 72: The node_image entry currently pins the KinD node by mutable tag
("kindest/node:v1.33.7"); replace it with the image digest (sha256) to prevent
tampering: pull the image and get its RepoDigest (e.g., via docker pull and
docker inspect) and then update the node_image value from "kindest/node:v1.33.7"
to the fully qualified digest form "kindest/node@sha256:..." so the workflow
uses the immutable digest instead of the tag.

In `@catalog/internal/catalog/mcpcatalog/providers.go`:
- Around line 354-362: The code validates only artifact.Uri for each element in
ys.Artifacts but then persists the full apimodels.MCPArtifact (including
CreateTimeSinceEpoch and LastUpdateTimeSinceEpoch) into properties via
mrmodels.NewStringProperty("artifacts", ...), which allows forged or malformed
timestamps to be stored; update the validation loop (iterating ys.Artifacts) to
also validate and canonicalize timestamp fields (CreateTimeSinceEpoch and
LastUpdateTimeSinceEpoch) for correct epoch-millis format/range before
marshalling: add checks in the same block that use a helper (e.g.,
validateArtifactTimestamps or inline validation inside the loop) to reject or
normalize invalid timestamps and return MCPServerProviderRecord{Error: ...} on
failure (similar to the existing basecatalog.ValidateArtifactURI handling), and
only marshal/persist the artifacts into properties after timestamps are
validated/normalized to ensure consumers receive safe epoch-millis values.

In `@clients/ui/frontend/src/app/hooks/useCheckNamespaceRegistryAccess.ts`:
- Around line 54-58: The catch block in the useCheckNamespaceRegistryAccess hook
is not resetting the cannotCheck flag, so when an earlier call set
cannotCheck=true and a subsequent call errors the stale flag remains; update the
catch handler in useCheckNamespaceRegistryAccess to clear cannotCheck (call
setCannotCheck(false) or reset it to the intended default) along with setting
error and setHasAccess(undefined) so the hook's state remains consistent after
errors.

---

Nitpick comments:
In
`@clients/ui/frontend/src/__tests__/cypress/cypress/pages/modelCatalogSettings.ts`:
- Around line 75-77: The test helper method toggleEnable currently forces a
click which masks Cypress actionability issues; update toggleEnable to call
this.findEnableToggle().click() without the { force: true } option so Cypress
verifies visibility/enabled state and surface real UI problems, and keep the
method returning this; reference the methods toggleEnable and findEnableToggle
to locate and modify the implementation.

In
`@clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registerAndStoreFields.cy.ts`:
- Around line 257-260: Replace the hardcoded API version in the cy.intercept
route with the centralized constant: update the cy.intercept call in the test
(the one registering 'checkNamespaceAccessCannotCheck') to build the URL using
MODEL_REGISTRY_API_VERSION (instead of '/api/v1') so the path becomes
`**/api/${MODEL_REGISTRY_API_VERSION}/check-namespace-registry-access`; keep the
same statusCode/body and alias but reference MODEL_REGISTRY_API_VERSION where
the version is currently hardcoded.

In
`@clients/ui/frontend/src/concepts/k8s/NamespaceSelectorField/NamespaceSelectorField.tsx`:
- Around line 197-208: The alert message in NamespaceSelectorField renders
awkwardly when registryName is undefined; update the Alert rendering logic
(within NamespaceSelectorField where selectedNamespace && !isLoading &&
cannotCheck is checked) to use a safe fallback for registryName (e.g.,
registryName || "registry" or "the selected registry") or conditionally change
the sentence to omit "the {registryName} registry" when registryName is empty so
the message reads coherently; ensure you reference the registryName variable in
the Alert content and keep the existing data-testid and conditional display
logic intact.

In `@go.mod`:
- Around line 23-25: The go.mod entry for
"github.com/testcontainers/testcontainers-go/modules/postgres v0.40.0" is
inconsistent with the other testcontainers entries at v0.41.0; update the
postgres module reference to v0.41.0 (matching
"github.com/testcontainers/testcontainers-go" and
"github.com/testcontainers/testcontainers-go/modules/mysql") and run a go mod
tidy or `go get
github.com/testcontainers/testcontainers-go/modules/postgres@v0.41.0` to ensure
the dependency graph is updated and go.sum regenerated.

In `@proposals/KEP-0003-renaming-strategy/README.md`:
- Around line 276-292: The Markdown code fences around the CI step ("docker push
ghcr.io/kubeflow/hub/server:${{ env.VERSION }}") and the image examples ("image:
ghcr.io/kubeflow/model-registry/server:..." and "image:
ghcr.io/kubeflow/hub/server:...") lack language specifiers; update both fenced
code blocks to use ```yaml so syntax highlighting and markdownlint MD040 are
satisfied (i.e., change the three-backtick openings before the CI snippet and
before the manifest examples to ```yaml).
- Line 494: The README contains a hardcoded person reference "update icon by
raising CNCF ticket (Matteo can do it)"; replace that text with a role-based
instruction or process link: change the line "update icon by raising CNCF ticket
(Matteo can do it)" to something like "submit logo update request to CNCF
Service Desk (requires maintainer credentials) — see [internal runbook]" and/or
assign to a role (e.g., "Owner: website-maintainers; Backup: sig-website-lead")
and include an escalation/fallback contact or link to the runbook so
contributors know who/what to contact if the primary is unavailable. Ensure the
new wording appears where the current string is used so it removes the personal
name and adds role/contact/runbook details.

🪄 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: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: eb1ccd1a-40f5-493b-8e2b-e431ab59fdbd

📥 Commits

Reviewing files that changed from the base of the PR and between 83bf4de and f65b1ed.

⛔ Files ignored due to path filters (4)

• clients/python/poetry.lock is excluded by !**/*.lock
• clients/ui/frontend/package-lock.json is excluded by !**/package-lock.json
• go.sum is excluded by !**/*.sum, !**/*.sum
• jobs/async-upload/poetry.lock is excluded by !**/*.lock

📒 Files selected for processing (41)

• .github/workflows/build.yml
• .github/workflows/check-db-schema-structs.yaml
• .github/workflows/controller-test.yml
• .github/workflows/csi-test.yml
• .github/workflows/go-generate.yml
• .github/workflows/go-mod-tidy-diff-check.yml
• .github/workflows/prepare.yml
• .github/workflows/ui-bff-build.yml
• catalog/internal/catalog/mcpcatalog/db_mcp_test.go
• catalog/internal/catalog/mcpcatalog/providers.go
• catalog/internal/catalog/mcpcatalog/providers_test.go
• catalog/internal/catalog/mcpcatalog/service/mcp_server_tool.go
• catalog/internal/catalog/mcpcatalog/service/mcp_server_tool_test.go
• clients/ui/bff/internal/api/check_namespace_registry_access_handler.go
• clients/ui/bff/internal/integrations/kubernetes/namespace_registry_access.go
• clients/ui/bff/internal/mocks/static_data_mock.go
• clients/ui/frontend/src/__tests__/cypress/cypress/pages/modelCatalogSettings.ts
• clients/ui/frontend/src/__tests__/cypress/cypress/pages/modelRegistrySettings.ts
• clients/ui/frontend/src/__tests__/cypress/cypress/pages/modelRegistryView/registerAndStoreFields.ts
• clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registerAndStoreFields.cy.ts
• clients/ui/frontend/src/app/AppRoutes.tsx
• clients/ui/frontend/src/app/api/k8s.ts
• clients/ui/frontend/src/app/hooks/__tests__/useCheckNamespaceRegistryAccess.spec.ts
• clients/ui/frontend/src/app/hooks/useCheckNamespaceRegistryAccess.ts
• clients/ui/frontend/src/app/pages/modelCatalog/screens/RegisterCatalogModelForm.tsx
• clients/ui/frontend/src/app/pages/modelRegistry/screens/RegisterModel/RegisterAndStoreFields.tsx
• clients/ui/frontend/src/app/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx
• clients/ui/frontend/src/app/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx
• clients/ui/frontend/src/app/pages/modelRegistry/screens/RegisterModel/RegistrationCommonFormSections.tsx
• clients/ui/frontend/src/app/pages/settings/ModelRegistrySettings.tsx
• clients/ui/frontend/src/app/routes/modelCatalogSettings/modelCatalogSettings.ts
• clients/ui/frontend/src/app/shared/markdown/MarkdownComponent.scss
• clients/ui/frontend/src/app/shared/markdown/MarkdownComponent.tsx
• clients/ui/frontend/src/app/shared/markdown/__tests__/MarkdownComponent.spec.tsx
• clients/ui/frontend/src/app/shared/markdown/components/CodeBlockComponent.tsx
• clients/ui/frontend/src/app/shared/markdown/components/__tests__/CodeBlockComponent.spec.tsx
• clients/ui/frontend/src/concepts/k8s/NamespaceSelectorField/NamespaceSelectorField.tsx
• go.mod
• jobs/async-upload/pyproject.toml
• jobs/async-upload/requirements.txt
• proposals/KEP-0003-renaming-strategy/README.md

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if NextPageToken or OrderBy are used elsewhere for MCPServerToolListOptions
rg -n 'MCPServerToolListOptions' --type=go -C3 | rg -i 'nextpagetoken|orderby|sortorder'

Repository: opendatahub-io/model-registry

Length of output: 55

---

🏁 Script executed:

# First, find and examine the mcp_server_tool.go file
find . -name "mcp_server_tool.go" -type f

Repository: opendatahub-io/model-registry

Length of output: 201

---

🏁 Script executed:

# Get file size and examine the List method around lines 75-78
wc -l catalog/internal/catalog/mcpcatalog/service/mcp_server_tool.go

Repository: opendatahub-io/model-registry

Length of output: 139

---

🏁 Script executed:

# Read the relevant sections of the file to understand the implementation
head -120 catalog/internal/catalog/mcpcatalog/service/mcp_server_tool.go | tail -60

Repository: opendatahub-io/model-registry

Length of output: 2401

---

🏁 Script executed:

# Search for MCPServerToolListOptions and Pagination struct definitions
rg -n 'type MCPServerToolListOptions|type Pagination' --type=go -A5

Repository: opendatahub-io/model-registry

Length of output: 1109

---

🏁 Script executed:

# Look for the dbmodels.Pagination definition
find . -name "*.go" -type f -exec grep -l "type Pagination struct" {} \;

Repository: opendatahub-io/model-registry

Length of output: 107

---

🏁 Script executed:

# Search for calls to the List method for MCPServerTool
rg -n '\.List\(' --type=go catalog/internal/catalog/mcpcatalog/service/mcp_server_tool.go -B5 -A5

Repository: opendatahub-io/model-registry

Length of output: 55

---

🏁 Script executed:

# Search for where MCPServerToolListOptions is constructed
rg -n 'MCPServerToolListOptions\s*{' --type=go -A10

Repository: opendatahub-io/model-registry

Length of output: 192

---

🏁 Script executed:

# Check if NextPageToken is set anywhere in MCPServerTool related code
rg -n 'NextPageToken' --type=go catalog/internal/catalog/mcpcatalog/

Repository: opendatahub-io/model-registry

Length of output: 1528

---

🏁 Script executed:

# Look at the full List method implementation to understand ordering
sed -n '50,90p' catalog/internal/catalog/mcpcatalog/service/mcp_server_tool.go

Repository: opendatahub-io/model-registry

Length of output: 1729

---

🏁 Script executed:

# Check if there's a default ordering in the query or schema
rg -n 'Order\(' --type=go catalog/internal/catalog/mcpcatalog/service/mcp_server_tool.go

Repository: opendatahub-io/model-registry

Length of output: 55

---

🏁 Script executed:

# See the context around the TODO comment for pagination token
sed -n '260,280p' catalog/internal/catalog/mcpcatalog/db_mcp.go

Repository: opendatahub-io/model-registry

Length of output: 700

---

🏁 Script executed:

# Check if there's a ListMCPServers that has pagination working correctly
rg -n 'ListMCPServers' --type=go -A20 catalog/internal/catalog/mcpcatalog/db_mcp.go

Repository: opendatahub-io/model-registry

Length of output: 1133

---

🏁 Script executed:

# Look for ApplyNameOrdering to understand how pagination should work
rg -n 'ApplyNameOrdering' --type=go -B2 -A8

Repository: opendatahub-io/model-registry

Length of output: 6610

---

🏁 Script executed:

# Check git log or comments to see if pagination for tools is planned
rg -n 'pagination.*tool|tool.*pagination' --type=go -i

Repository: opendatahub-io/model-registry

Length of output: 495

---

Incomplete pagination implementation: OrderBy, SortOrder, and NextPageToken are silently ignored.

The List method applies only PageSize via LIMIT but ignores OrderBy, SortOrder, and NextPageToken from the embedded Pagination struct. These fields are explicitly populated in db_mcp.go (lines 238-241) before calling List(), but the method discards them without error or warning.

Ignoring OrderBy/SortOrder creates non-deterministic results: repeated calls with the same PageSize may return different subsets depending on database row order. While NextPageToken is documented as TODO (line 268), the OrderBy/SortOrder handling is undocumented, suggesting unintentional omission.

Either:

1. Remove OrderBy/SortOrder from pagination options for tools and document this limitation, or
2. Apply deterministic ordering (e.g., by ID or creation time) and implement cursor-based pagination like MCPServerList does via ApplyNameOrdering.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@catalog/internal/catalog/mcpcatalog/service/mcp_server_tool.go` around lines
75 - 78, The List method currently only applies PageSize and ignores
Pagination.OrderBy, Pagination.SortOrder and Pagination.NextPageToken, causing
non-deterministic paging; update the List implementation to either (A) respect
ordering and cursor semantics by applying deterministic ordering (reuse
ApplyNameOrdering used by MCPServerList) and implement NextPageToken cursor
handling consistent with MCPServerList, or (B) explicitly reject/omit
OrderBy/SortOrder/NextPageToken by returning an error or documenting the
limitation; locate the logic in the List function and modify the handling of
listOptions.Pagination (OrderBy, SortOrder, NextPageToken) to follow one of
these two approaches so paging becomes deterministic and callers are informed.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

hook_file="$(fd -i 'useCheckNamespaceRegistryAccess.ts' | head -n1)"
echo "Hook file: ${hook_file}"

sed -n '1,260p' "${hook_file}"

echo
echo "Search for runtime validation/parsing of response booleans:"
rg -n "hasAccess|cannotCheck|safeParse|zod|io-ts|superstruct|typeof .*boolean|parse" "${hook_file}"

Repository: opendatahub-io/model-registry

Length of output: 2760

---

Add runtime validation for API response booleans before state assignment.

Lines 49-52 assign result.cannotCheck and result.hasAccess directly to state without validating they are booleans. Use a schema validator (zod, io-ts) or explicit typeof checks to ensure the API response contains boolean values before setCannotCheck() and setHasAccess() calls. Violates guideline: "Validate all API responses before rendering" (**/*.{ts,tsx,js,jsx}).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@clients/ui/frontend/src/app/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx`
around lines 67 - 68, Validate the API response booleans before assigning to
state: in the useCheckNamespaceRegistryAccess result handling, check that
result.cannotCheck and result.hasAccess are actually booleans (e.g., typeof
result.cannotCheck === "boolean") or validate the response with a lightweight
schema (zod/io-ts) and only then call setCannotCheck(...) and setHasAccess(...);
if validation fails, set a safe default (false) or handle the error path so
mrName/registryNamespace/formData.namespace logic never assigns non-boolean
values to state.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Raw <pre> content is dropped by this extractor.

This only keeps text found under element children, so authored markdown like <pre>kubectl get pods</pre> collapses to ''. With rehypeRaw enabled, that path is reachable from sanitized content. Extract text recursively from both direct text nodes and nested elements before rendering CodeBlockComponent, and add a regression case for direct-string <pre> input.

Suggested fix

         pre: ({ children }) => {
-          const code = React.Children.toArray(children)
-            .filter((child): child is React.ReactElement<{ children: React.ReactNode }> =>
-              React.isValidElement(child),
-            )
-            .flatMap((child) => React.Children.toArray(child.props.children))
-            .map((child) => (typeof child === 'string' ? child : ''))
-            .join('')
-            .replace(/\n$/, '');
+          const extractText = (node: React.ReactNode): string =>
+            React.Children.toArray(node)
+              .map((child) => {
+                if (typeof child === 'string' || typeof child === 'number') {
+                  return String(child);
+                }
+                return React.isValidElement<{ children?: React.ReactNode }>(child)
+                  ? extractText(child.props.children)
+                  : '';
+              })
+              .join('');
+          const code = extractText(children).replace(/\n$/, '');
           return <CodeBlockComponent>{code}</CodeBlockComponent>;
         },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@clients/ui/frontend/src/app/shared/markdown/MarkdownComponent.tsx` around
lines 63 - 72, The <pre> renderer in MarkdownComponent.tsx drops raw <pre> text
because it only grabs immediate string children via the current
React.Children.toArray(...).map(...).join(''), so change the pre rendering logic
(the anonymous pre: ({ children }) => { ... } block) to recursively extract text
from React nodes and nested elements (e.g., via a helper like
collectTextFromReactNode(node): string) so direct-string children like
<pre>kubectl get pods</pre> and nested nodes both produce the full code string
before passing it into CodeBlockComponent; also add a regression test that feeds
a direct-string <pre> input and asserts CodeBlockComponent receives the expected
"kubectl get pods" text.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

In react-markdown custom componentsrenderers, does thecodecallback receive anodeprop, and should that prop be omitted before spreading onto a native element?

💡 Result:

Yes, the code callback in react-markdown's custom components renderers receives a node prop, which is the original HAST Element from the markdown AST. Yes, the node prop (along with other non-native props like inline) should be omitted before spreading onto a native element, as it is not a valid HTML attribute and examples consistently destructure it out before spreading the remaining props. This is confirmed by official documentation stating "Every component will receive a node. This is the original [Element from hast] element being rendered." and TypeScript types via ExtraProps including node?: Element. All code examples (official syntax highlighting demo, blog analyses, Stack Overflow) destructure {node, inline, className, children, ...props} then spread {...props} or {...rest} onto , avoiding invalid attributes on the DOM element.

Citations:

• 1: https://www.singlehanded.dev/blog/understanding-the-components-prop-in-react-markdown
• 2: https://www.npmjs.com/package/react-markdown
• 3: https://github.com/remarkjs/react-markdown
• 4: https://stackoverflow.com/questions/77335936/whats-the-type-of-props-of-the-methods-in-components-object-in-react-markdown
• 5: https://www.npmjs.com/package/react-markdown/v/7.1.0

---

Extract node and other non-DOM props before spreading onto <code> element.

The code renderer in react-markdown receives metadata props like node and inline alongside valid DOM props. These non-native attributes should not be spread onto the native <code> element, as they cause React warnings and invalid DOM attributes.

Suggested fix

-        code: ({ className, children, ...props }) => (
+        code: ({ node: _node, className, children, ...props }) => (
           <code className={className} {...props}>
             {children}
           </code>
         ),

📝 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.

Suggested change

	code: ({ className, children, ...props }) => (
	<code className={className} {...props}>
	{children}
	</code>
	code: ({ node: _node, inline: _inline, className, children, ...props }) => (
	<code className={className} {...props}>
	{children}
	</code>
	),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@clients/ui/frontend/src/app/shared/markdown/MarkdownComponent.tsx` around
lines 74 - 77, The custom `code` renderer in MarkdownComponent.tsx is spreading
non-DOM props (like `node`, `inline`) onto the native <code> element; update the
renderer (the `code: ({ className, children, ...props }) => ...` handler) to
destructure and remove `node` and other non-DOM props (e.g., `node`, `inline`,
`level`) before spreading the remaining valid DOM props onto the <code> element
so React warnings are eliminated and only valid attributes are applied.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Verify the exact release exists on PyPI
curl -fsSL https://pypi.org/pypi/requests/json \
| python -c 'import sys,json; d=json.load(sys.stdin); print("2.33.0" in d["releases"]); print(d["info"]["version"])'

# 2) Query OSV for known vulns in requests
curl -fsSL https://api.osv.dev/v1/query -H "Content-Type: application/json" \
  -d '{"package":{"name":"requests","ecosystem":"PyPI"}}' \
| python -c 'import sys,json; d=json.load(sys.stdin); print("vuln_count=", len(d.get("vulns", []))); [print(v.get("id")) for v in d.get("vulns", [])]'

Repository: opendatahub-io/model-registry

Length of output: 329

---

Pin requests to a version without known security advisories before merge.

Line 15 specifies requests = "^2.33.0". Verification confirms version 2.33.0 exists but 13 known CVE/advisory entries (GHSA-652x-xj99-gmcc, GHSA-9hjg-9r4m-mvj7, GHSA-9wx4-h78v-vm56, GHSA-cfj3-7x9c-4p3h, GHSA-gc5v-m9x4-r6x2, GHSA-j8r2-6x86-q33q, GHSA-pg2w-x9wp-vw92, GHSA-x84v-xcm2-53pg, PYSEC-2014-13, PYSEC-2014-14, PYSEC-2015-17, PYSEC-2018-28, PYSEC-2023-74) are documented for the requests package, exposing runtime HTTP paths to upstream risk (CWE-1104).

Upgrade to 2.33.1 (latest) and verify against OSV/GHSA databases before merge. If advisories persist for the newer release, apply vendor patches or use an alternate HTTP client.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@jobs/async-upload/pyproject.toml` at line 15, The pyproject.toml currently
pins requests with the caret range "requests = \"^2.33.0\"" which includes a
release (2.33.0) with known security advisories; update that dependency spec to
a fixed, non-vulnerable release (e.g., "requests = \"==2.33.1\"") in the
pyproject.toml entry and run your dependency resolver/lock (poetry lock or
pip-compile) to regenerate lockfiles, then verify the chosen version against
OSV/GHSA; if advisories still appear for 2.33.1, either upgrade to the next
patched release or replace requests with an alternate HTTP client or apply
vendor patches before merging.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify .model-registry.yaml is not referenced in codebase

echo "Searching for references to .model-registry.yaml..."
rg -i "model-registry\.yaml" -g '!proposals/**' -C3

echo -e "\nSearching for config file loading patterns..."
rg -i "config.*file|load.*config|read.*config" -g '*.go' -g '*.py' -C3 | head -50

echo -e "\nSearching for yaml config loading..."
rg -i "viper\.(Read|Set)|yaml\.(load|Load|Unmarshal)" -g '*.go' -C3 | head -30

Repository: opendatahub-io/model-registry

Length of output: 22504

---

Incorrect claim: .model-registry.yaml is unused and safe to remove.

The codebase actively loads this file. cmd/root.go uses Viper to search for and load .model-registry.yaml from $HOME/.model-registry.yaml at startup via viper.ReadInConfig(). When the file exists, it is logged as "Using config file: ". Removing it would break deployments that rely on this configuration.

Do not implement the recommendation to "Remove unused .model-registry.yaml file" in the KEP document—the file is in active use.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@proposals/KEP-0003-renaming-strategy/README.md` around lines 577 - 603, The
KEP incorrectly states that `.model-registry.yaml` is unused; fix the document
to reflect that cmd/root.go uses Viper (viper.ReadInConfig()) to load
`$HOME/.model-registry.yaml` at startup (logged with "Using config file: "), so
remove the recommendation to delete the config file and instead document that
the file is actively loaded and must not be removed for deployments relying on
it; update the "Recommendation" and "Backwards Compatibility Impact" sections to
state the file is used and advise caution when renaming or removing it.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Incomplete security implementation planning.

Lines 645-661 identify security concerns (image signing, SBOM, vulnerability databases) but lack concrete implementation details:

• Image Signing (647-650): "New images need new signatures" - but who generates them, when, and how are they distributed to consumers?
• SBOM (652-654): "Security scanning references" - no plan for updating scanning tools or vulnerability tracking systems
• CVE References (656-660): "CVE references may use old project name" - no plan for updating vulnerability databases or coordinating with security researchers

For a production system handling ML models (potentially including sensitive training data or proprietary algorithms), security artifact continuity during migration is critical.

Required additions:

1. Image signing workflow migration plan with responsibility matrix
2. SBOM regeneration process and distribution mechanism
3. CVE database update coordination (NVD, GitHub Security Advisories, etc.)
4. Security scanning tool configuration migration checklist
5. Timeline for security artifact cutover aligned with container registry migration

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@proposals/KEP-0003-renaming-strategy/README.md` around lines 645 - 661, The
Security Considerations section (headers "Image Signing", "SBOM (Software Bill
of Materials)", "Vulnerability Databases") is high-level and missing concrete
plans; update that section to add: a detailed image-signing workflow migration
plan naming who signs images, when, signing keys rotation/ownership and how
signatures are published/verified (responsibility matrix for teams); an SBOM
regeneration and distribution process describing tools, file formats, storage
location and consumer access; a CVE coordination plan noting how to update
NVD/GitHub Security Advisories and communicate name-mapping to security
researchers; a checklist to migrate security scanning tool configurations; and a
cutover timeline aligning image-signing/SBOM/CVE updates with registry migration
and release notes.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Missing required artifact: Rollback plan.

Line 712 lists "Rollback plan documented" as an exit criterion for the Beta phase, but the KEP contains no rollback plan. For a migration involving breaking changes to container images and Go module paths, a detailed rollback strategy is essential:

Required rollback plan components:

1. Image rollback: Process for reverting to ghcr.io/kubeflow/model-registry/* namespace if critical issues emerge
2. Version rollback: Procedure for downgrading from new version to previous stable release
3. Go module rollback: Guidance for consumers who updated imports but need to revert
4. Coordination: Communication plan for emergency rollback across kubeflow/manifests, KServe, Pipelines
5. Decision criteria: Specific conditions that trigger rollback (e.g., >X% deployment failures, critical security issue)
6. Timeline: Maximum time window for rollback before migration is considered irreversible

This rollback plan should be added to the KEP before Beta phase begins, as specified by the document's own criteria.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@proposals/KEP-0003-renaming-strategy/README.md` around lines 688 - 736, The
KEP currently lists "Rollback plan documented" under the Beta exit criteria but
lacks the plan; add a new "Rollback Plan" subsection (place it under the Beta
Exit Criteria or as its own section near "Graduation Criteria") that enumerates
concrete procedures for image rollback (how to revert to
ghcr.io/kubeflow/model-registry/* and restore tags/aliases), version rollback
(steps to downgrade releases and re-publish previous artifacts), Go module
rollback (consumer guidance for reverting import paths and versions),
coordination/communication steps (who to notify in kubeflow/manifests, KServe,
Pipelines and how to coordinate emergency rollbacks), decision criteria
(specific measurable triggers like % failure, error class, or security impact),
and a maximum rollback timeline (time window after which changes are considered
irreversible); reference the Beta exit criterion text ("Rollback plan
documented") and ensure the plan includes owners, commands/automation hints, and
rollback verification steps.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Unstable external reference: Authenticated JIRA link.

Line 720 links to https://cncfservicedesk.atlassian.net/servicedesk/customer/portal/1/CNCFSD-3354?created=true which:

1. Requires CNCF authentication - not accessible to all KEP readers
2. May not be a stable long-term reference
3. Query parameter ?created=true suggests ephemeral state

Mitigation options:

• Replace with public GitHub issue tracking logo update
• Remove link and describe the logo update process generically
• Archive relevant JIRA content in the KEP or linked GitHub issue

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@proposals/KEP-0003-renaming-strategy/README.md` at line 720, The README
contains an unstable, authenticated JIRA link
(https://cncfservicedesk.atlassian.net/servicedesk/customer/portal/1/CNCFSD-3354?created=true)
which is not public and includes an ephemeral query param; replace that URL in
the line mentioning the updated logo with a stable, public reference (preferably
a GitHub issue/PR that tracks the logo change) or remove the hyperlink and
replace it with a generic description of the logo update process, and if
preserving the JIRA content is necessary, archive its relevant details into the
KEP or a linked public GitHub issue instead of linking to the private Atlassian
URL.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Implementation readiness: All critical dates are TBD.

Lines 739-748 show the Implementation History with every milestone marked "TBD". Before this KEP can be considered actionable:

1. Timeline required: Establish concrete dates for Alpha/Beta/Stable phases to enable downstream coordination
2. Lead time: Container registry namespace change requires minimum 60-90 days notice to ecosystem (per risk mitigation in line 135)
3. Coordination windows: KServe, Pipelines, and Manifests teams need advance notice for synchronized releases

Current state: This KEP documents the strategy but is not implementation-ready without a committed timeline.

Recommendation: Establish timeline before merging this KEP, or clearly label it as "Draft" until dates are finalized.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@proposals/KEP-0003-renaming-strategy/README.md` around lines 739 - 748, The
Implementation History under "Implementation History" lists Alpha/Beta/Stable as
"TBD", which prevents this KEP from being actionable; update the "Alpha phase",
"Beta phase", and "Stable phase" entries with concrete target dates (choosing
specific calendar dates) that respect the 60–90 day lead time requirement called
out elsewhere, and add a short coordination note indicating planned
synchronization windows with KServe, Pipelines, and Manifests teams (or
alternatively mark the KEP header/state clearly as "Draft" until those dates and
coordination commitments are finalized) so reviewers can verify the timeline and
cross-team coordination.