feat(agent-ops): OpenShell SDK prototype with federation fixes - #8515
Gkrumbach07 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds OpenShell configuration and sandbox lifecycle integration to the BFF, including agent listing, deployment, deletion, start/stop operations, agent-card probing, health status, and chat proxying. Extends runtime metadata with pod names and readiness fallbacks. Enables the Agent Ops dashboard area, adds frontend proxy and webpack configuration, provides an agent-card API helper, and introduces an agent deployments page. Updates dashboard defaults, Go dependencies, and Go build images. 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.69.3)Trivy execution failed: 2026-07-13T14:12:35Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: azure-arm scan error: fs filter error: fs filter error: walk error range error: stat backend/doctor.config.json: no such file or directory: range error: stat backend/doctor.config.json: no such file or directory Comment |
537e7bd to
d5f4cb2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/agent-ops/bff/internal/mapper/agent.go (1)
202-216: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ReadyReplicaCountfallback conflates "pod name present" with "ready".When
readyReplicas/ready_replicasare absent, the function returns1merely becauseAgentPodName(status)is non-empty — i.e., a pod being named in status is treated as proof it's ready. A pod can be present but crashing/pending, in which case this reportsPodCount: 1for a non-ready workload, misleading callers of a field whose name promises actual readiness.♻️ Suggested tightening
if v, ok := intFromAny(status["ready_replicas"]); ok { return v } - if podName := AgentPodName(status); podName != "" { - return 1 - } - return 0 + // Only treat pod presence as a proxy for ready count if the workload is + // otherwise reporting a ready condition; avoid asserting readiness solely + // from pod-name presence. + return 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 `@packages/agent-ops/bff/internal/mapper/agent.go` around lines 202 - 216, Update ReadyReplicaCount so the fallback after checking readyReplicas and ready_replicas does not infer readiness from AgentPodName; return zero when no explicit ready-replica count is available, preserving the existing numeric parsing behavior.
🤖 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 `@frontend/config/webpack.dev.js`:
- Around line 258-271: Update the agentOpsProxies entry in the webpack dev proxy
configuration to forward the user token using the same onProxyReq or equivalent
header logic as the neighboring proxy blocks. Ensure both
x-forwarded-access-token and Authorization are propagated to the agent-ops
target while preserving its existing context, target, and pathRewrite settings.
In `@packages/agent-ops/bff/GAPS.md`:
- Around line 20-24: Update the Sandbox CR route-creation flow so every created
Route includes an OwnerReference pointing to its owning Sandbox resource. Ensure
deletion of the Sandbox garbage-collects the Route, while preserving the
existing ownerReference behavior for the AgentRuntime path.
- Around line 5-11: Update DeleteAgent, StopAgent, and StartAgent to perform a
per-request SubjectAccessReview for the relevant Sandbox CR delete/patch action
before executing it, using the authenticated caller’s identity and target
resource context. Deny the operation when SSAR is not allowed, and remove the
documentation framing this authorization as an unassigned backlog item.
In `@packages/agent-ops/bff/go.mod`:
- Around line 3-11: Replace the unvetted github.com/rhuss/openshell-sdk-go
dependency in go.mod with the official NVIDIA/OpenShell Go SDK or an
upstream-maintained equivalent, updating imports and usages throughout the
affected code to match its API. If no suitable upstream package exists, remove
the dependency and vendor an approved, provenance-reviewed implementation
instead.
In `@packages/agent-ops/bff/internal/api/agent_chat_handler.go`:
- Around line 54-60: Update the response-body proxying flow around the handler’s
header forwarding and io.Copy call to copy through io.LimitReader with the
configured response-size limit, handle the copy error instead of discarding it,
and filter hop-by-hop headers such as Transfer-Encoding and Connection before
forwarding them. Preserve the upstream status code and bounded body forwarding
behavior.
In `@packages/agent-ops/bff/internal/api/app.go`:
- Around line 127-129: The AUTH_METHOD=disabled validation in app initialization
now permits a real OpenShell backend without a development-only safeguard.
Update the condition around cfg.AuthMethod, cfg.MockAgentClient, and
cfg.OpenShellGatewayURL so disabled authentication remains restricted to the
explicitly approved MockAgentClient path, or add an equivalent explicit dev-only
safeguard and warning for OpenShell; preserve rejection of unsafe
unauthenticated production configurations.
- Around line 184-201: Update the OpenShell client construction in the
cfg.OpenShellGatewayURL branch to use the configured TLS verification behavior
and gateway authentication instead of hard-coded v1.NoAuth() and Insecure: true.
Map cfg.InsecureSkipVerify and the existing configured credential/authentication
settings into v1.Config, preserving the current error handling and factory
setup.
In `@packages/agent-ops/bff/internal/api/healthcheck_handler.go`:
- Around line 17-22: Update the healthcheck handler’s healthCheck.OpenShell
construction to expose only the Enabled status; remove the Gateway and Namespace
assignments from the unauthenticated liveness response, leaving internal
OpenShell details for an authenticated route if needed.
In `@packages/agent-ops/bff/internal/integrations/agents/openshell/client.go`:
- Around line 280-291: Update the AgentSummary construction flow containing
displayName, description, and framework so the annotation-derived displayName is
assigned to the appropriate AgentSummary field instead of being discarded.
Remove the `_ = displayName` suppression and preserve the existing fallback
behavior for description and framework.
- Around line 45-83: Enforce the request namespace in ListAgents, GetAgent, and
DeleteAgent before invoking OpenShell operations. Replace ignored namespace
parameters with the requested namespace and reject requests when it differs from
c.namespace, returning the existing authorization/validation error used by the
integration. Ensure all read and delete paths operate only on the validated
configured namespace.
- Around line 326-333: Update the spec map construction in the relevant Sandbox
response logic to source operatingMode from the Sandbox spec rather than the
status phase, preserving values such as Suspended and Running; alternatively
remove the operatingMode key if this response should expose only phase. Do not
populate it from status.
In `@packages/agent-ops/Dockerfile`:
- Line 7: Update the GOLANG_BASE_IMAGE argument to use a reproducible, immutable
image reference by pinning golang to a specific full patch version and
preferably its sha256 digest; do not leave it as the floating golang:1.25 tag.
In `@packages/agent-ops/Dockerfile.workspace`:
- Line 12: Update the GOLANG_BASE_IMAGE argument in the workspace Dockerfile to
use a fixed, immutable image digest instead of the floating golang:1.25 tag,
preserving the intended Go 1.25 base image.
In `@packages/agent-ops/src/AgentDeploymentsPage.tsx`:
- Around line 55-71: Update fetchAgents and the other agent-runtime fetch paths
to use the existing validated API layer in
frontend/src/app/api/agentRuntimes.ts, including isModArchResponse and
handleRestFailures, instead of raw fetch parsing. Ensure the validated response
confirms runtimes is an array before passing it to setAgents, preserving the
existing loading and error-state handling.
In `@packages/agent-ops/V1-IMPLEMENTATION-GAPS.md`:
- Around line 98-102: Update item 10, “Env var secrets via k8s Secret,” to
require v1 gating: block plaintext API_KEY-style inputs or require creation and
reference of a Kubernetes Secret before deployment. Do not leave plaintext
environment-variable storage as an unassigned follow-up; document the deployment
wizard behavior and acceptance criteria accordingly.
---
Outside diff comments:
In `@packages/agent-ops/bff/internal/mapper/agent.go`:
- Around line 202-216: Update ReadyReplicaCount so the fallback after checking
readyReplicas and ready_replicas does not infer readiness from AgentPodName;
return zero when no explicit ready-replica count is available, preserving the
existing numeric parsing behavior.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f8eb6cbf-785c-4f74-bffb-5fcb8074963f
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.jsonpackages/agent-ops/bff/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (27)
backend/src/types.tsbackend/src/utils/constants.tsfrontend/config/webpack.common.jsfrontend/config/webpack.dev.jsfrontend/src/concepts/areas/const.tspackages/agent-ops/Dockerfilepackages/agent-ops/Dockerfile.workspacepackages/agent-ops/V1-IMPLEMENTATION-GAPS.mdpackages/agent-ops/bff/GAPS.mdpackages/agent-ops/bff/cmd/main.gopackages/agent-ops/bff/go.modpackages/agent-ops/bff/internal/api/agent_chat_handler.gopackages/agent-ops/bff/internal/api/app.gopackages/agent-ops/bff/internal/api/healthcheck_handler.gopackages/agent-ops/bff/internal/config/environment.gopackages/agent-ops/bff/internal/integrations/agents/openshell/client.gopackages/agent-ops/bff/internal/integrations/agents/openshell/factory.gopackages/agent-ops/bff/internal/mapper/agent.gopackages/agent-ops/bff/internal/models/agent_runtime_detail.gopackages/agent-ops/bff/internal/models/health_check.gopackages/agent-ops/extensions.tspackages/agent-ops/frontend/GAPS.mdpackages/agent-ops/frontend/config/webpack.common.jspackages/agent-ops/frontend/src/app/api/agentRuntimes.tspackages/agent-ops/frontend/src/app/components/DeleteAgentModal.tsxpackages/agent-ops/frontend/src/app/hooks/useAgentCardDetail.tspackages/agent-ops/src/AgentDeploymentsPage.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: ODH Dashboard Agent
- GitHub Check: preflight (8515, 537e7bd, agent-ops/openshell-prototype, Gkrumba...
⚠️ CI failures not shown inline (5)
Commit Status: ci/prow/images: ci/prow/images
Conclusion: failure
Job failed. BaseSHA:47376829260e320babe797f02e7dcd570b98ac99
Commit Status: ci/prow/odh-mod-arch-agent-ops-pr-image-mirror: ci/prow/odh-mod-arch-agent-ops-pr-image-mirror
Conclusion: failure
Job failed. BaseSHA:47376829260e320babe797f02e7dcd570b98ac99
Commit Status: tide: tide
Conclusion: failure
Not mergeable. PR has a merge conflict.
Commit Status: ci/prow/odh-dashboard-operator-pr-image-mirror: ci/prow/odh-dashboard-operator-pr-image-mirror
Conclusion: failure
Job failed. BaseSHA:47376829260e320babe797f02e7dcd570b98ac99
Commit Status: ci/prow/odh-dashboard-pr-image-mirror: ci/prow/odh-dashboard-pr-image-mirror
Conclusion: failure
Job failed. BaseSHA:47376829260e320babe797f02e7dcd570b98ac99
🧰 Additional context used
📓 Path-based instructions (10)
**
⚙️ CodeRabbit configuration file
**: REVIEW PRIORITIES:
- Security vulnerabilities — provide severity, exploit scenario,
and remediation code. Cite CWE/CVE IDs.- Bugs that could reach production — logic errors, null/undefined,
race conditions, incorrect async handling, resource leaks.- API contract correctness — shape mismatches, missing error handling,
silent failures, wrong HTTP status codes.- Performance only when measurable — O(n^2) in hot paths, unbounded
memory growth, missing pagination.Do not comment on:
- Naming preferences (unless genuinely misleading)
- Import ordering or formatting (handled by ESLint and Prettier)
- Alternative patterns that are equally valid
- Missing docs unless a public API is genuinely unclear
- Code deduplication / DRY suggestions where both copies are short
and self-contained (< 20 lines)- Adding explicit type annotations when TypeScript can infer the type
- Suggesting exhaustive switch/if-else when a default branch exists
Files:
packages/agent-ops/Dockerfile.workspacepackages/agent-ops/frontend/GAPS.mdpackages/agent-ops/bff/internal/models/agent_runtime_detail.gopackages/agent-ops/frontend/src/app/components/DeleteAgentModal.tsxpackages/agent-ops/bff/GAPS.mdpackages/agent-ops/Dockerfilebackend/src/types.tspackages/agent-ops/bff/internal/api/healthcheck_handler.gopackages/agent-ops/bff/cmd/main.gopackages/agent-ops/extensions.tspackages/agent-ops/bff/internal/api/agent_chat_handler.gopackages/agent-ops/frontend/src/app/hooks/useAgentCardDetail.tsfrontend/src/concepts/areas/const.tsfrontend/config/webpack.common.jspackages/agent-ops/bff/internal/integrations/agents/openshell/factory.gopackages/agent-ops/bff/internal/config/environment.gopackages/agent-ops/frontend/src/app/api/agentRuntimes.tspackages/agent-ops/bff/internal/models/health_check.gopackages/agent-ops/V1-IMPLEMENTATION-GAPS.mdfrontend/config/webpack.dev.jsbackend/src/utils/constants.tspackages/agent-ops/src/AgentDeploymentsPage.tsxpackages/agent-ops/frontend/config/webpack.common.jspackages/agent-ops/bff/internal/api/app.gopackages/agent-ops/bff/internal/mapper/agent.gopackages/agent-ops/bff/internal/integrations/agents/openshell/client.gopackages/agent-ops/bff/go.mod
**/Dockerfile*
⚙️ CodeRabbit configuration file
**/Dockerfile*: DOCKERFILES:
- Multi-stage builds: separate build and runtime stages.
- Copy only necessary artifacts to runtime stage (no source code, node_modules, .git).
- Use the same base image version as the main Dockerfile where possible.
- No npm install in runtime stage — copy from build stage.
- Dockerfile.workspace files are for dev workspace images — follow the same base image
conventions.
Files:
packages/agent-ops/Dockerfile.workspacepackages/agent-ops/Dockerfile
packages/*/bff/**/*.go
⚙️ CodeRabbit configuration file
packages/*/bff/**/*.go: GO BFF SERVICE (Backend-for-Frontend):
Packages with BFFs: automl, autorag, eval-hub, gen-ai, maas, mlflow.
- Validate all incoming request bodies and query parameters at handler level.
- No credentials in BFF code — use mounted secrets or environment variables.
- Use the shared K8s client config; do not create ad-hoc kubeconfig readers.
- HTTP clients to external services must set timeouts and use TLS verification.
- No panic in handlers — use explicit error returns with context.
- OpenAPI spec in api/ or docs/ must match actual handler signatures.
- Follow .golangci.yaml rules in the package root.
- Repository pattern: data access through repository interfaces, not direct K8s calls
in handlers. Mock interfaces for unit testing (see mocks/ directory).
Files:
packages/agent-ops/bff/internal/models/agent_runtime_detail.gopackages/agent-ops/bff/internal/api/healthcheck_handler.gopackages/agent-ops/bff/cmd/main.gopackages/agent-ops/bff/internal/api/agent_chat_handler.gopackages/agent-ops/bff/internal/integrations/agents/openshell/factory.gopackages/agent-ops/bff/internal/config/environment.gopackages/agent-ops/bff/internal/models/health_check.gopackages/agent-ops/bff/internal/api/app.gopackages/agent-ops/bff/internal/mapper/agent.gopackages/agent-ops/bff/internal/integrations/agents/openshell/client.go
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: GO SECURITY (Kubernetes Controllers):
- Use io.LimitReader for HTTP response bodies (prevent memory exhaustion)
- Validate all data from json.Unmarshal before storing in ConfigMaps/Secrets
- No InsecureSkipVerify in TLS configs (enables MITM attacks)
- Validate CR spec fields before using in ConfigMaps/Secrets
- Set OwnerReferences on all child resources
- Validate inputs before exec.Command (prevent command injection)
- Never log sensitive fields (Password, Token, APIKey, Secret.Data)
- Avoid weak cryptography (MD5, SHA1) for security operations
Files:
packages/agent-ops/bff/internal/models/agent_runtime_detail.gopackages/agent-ops/bff/internal/api/healthcheck_handler.gopackages/agent-ops/bff/cmd/main.gopackages/agent-ops/bff/internal/api/agent_chat_handler.gopackages/agent-ops/bff/internal/integrations/agents/openshell/factory.gopackages/agent-ops/bff/internal/config/environment.gopackages/agent-ops/bff/internal/models/health_check.gopackages/agent-ops/bff/internal/api/app.gopackages/agent-ops/bff/internal/mapper/agent.gopackages/agent-ops/bff/internal/integrations/agents/openshell/client.go
packages/*/frontend/src/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
packages/*/frontend/src/**/*.{ts,tsx}: FEATURE PLUGIN FRONTEND (Module Federation):
These packages (automl, autorag, eval-hub, gen-ai, maas, mlflow) use Module Federation
to load as remotes into the host dashboard app.
- Plugins must use plugin-core APIs for navigation, not direct router manipulation.
- Shared dependencies (React, PatternFly, Redux) must come from the host app — do not
bundle duplicates.- No global CSS — use PatternFly utility classes or CSS modules only.
- Lazy-load heavy components; plugins load on demand via Module Federation.
- Follow PatternFly v6 patterns consistent with the main frontend app.
Files:
packages/agent-ops/frontend/src/app/components/DeleteAgentModal.tsxpackages/agent-ops/frontend/src/app/hooks/useAgentCardDetail.tspackages/agent-ops/frontend/src/app/api/agentRuntimes.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
**/*.{ts,tsx,js,jsx}: WEB SECURITY (XSS, CSRF Prevention):
- No dangerouslySetInnerHTML without sanitization (XSS - CWE-79)
- Validate all API responses before rendering
- CSRF token validation for state-changing operations
- No sensitive data in localStorage
Files:
packages/agent-ops/frontend/src/app/components/DeleteAgentModal.tsxbackend/src/types.tspackages/agent-ops/extensions.tspackages/agent-ops/frontend/src/app/hooks/useAgentCardDetail.tsfrontend/src/concepts/areas/const.tsfrontend/config/webpack.common.jspackages/agent-ops/frontend/src/app/api/agentRuntimes.tsfrontend/config/webpack.dev.jsbackend/src/utils/constants.tspackages/agent-ops/src/AgentDeploymentsPage.tsxpackages/agent-ops/frontend/config/webpack.common.js
backend/src/**/*.{ts,js}
⚙️ CodeRabbit configuration file
backend/src/**/*.{ts,js}: ODH DASHBOARD BACKEND (Node.js BFF):
- All external API calls must use the service account token, never user-provided tokens.
- Validate and sanitize route parameters before K8s API calls (prevent injection).
- Proxy endpoints must not expose internal cluster addresses to the client.
- Error responses must not leak cluster internals (pod names, IPs, stack traces).
- Verify RBAC: backend routes should check user permissions via SubjectAccessReview.
Files:
backend/src/types.tsbackend/src/utils/constants.ts
frontend/src/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
frontend/src/**/*.{ts,tsx}: ODH DASHBOARD FRONTEND (main app):
- PatternFly v6: use PF6 imports from
@patternfly/react-core, not custom wrappers.
Avoid custom CSS — if you need to "nudge" PF layout, check frontend/src/concepts/dashboard first.- Functional components only (no class components). Use hooks for state management.
- API calls: use the shared API utilities, never raw fetch(); handle loading/error states.
- No hardcoded cluster URLs or API endpoints — use config from backend.
- Route guards: protected routes must check user permissions before rendering.
- Performance: avoid unnecessary useCallback/useMemo/useRef — React is performant by default.
Only use useCallback when the function is passed as a prop, used as a useEffect dependency,
or returned from a custom hook (see docs/best-practices.md).- Custom components go in frontend/src/components. PF-first: verify with the team before
creating new custom components.
Files:
frontend/src/concepts/areas/const.ts
**/webpack.{common,dev,prod}.{ts,js}
⚙️ CodeRabbit configuration file
**/webpack.{common,dev,prod}.{ts,js}: WEBPACK CONFIG:
(Suppression) Do not suggest alternative bundler configurations
or plugin replacements. Only flag security issues and broken build logic.
Files:
frontend/config/webpack.common.jsfrontend/config/webpack.dev.jspackages/agent-ops/frontend/config/webpack.common.js
**/go.mod
⚙️ CodeRabbit configuration file
**/go.mod: GO DEPENDENCY SECURITY (CWE-829):
- Verify new dependencies are from trusted organizations
- Check for replace directives pointing to forks (supply chain risk).
Flag replace directives that redirect well-known modules
(golang.org/x/, k8s.io/, sigs.k8s.io/*) to personal forks- Flag indirect dependency additions unrelated to the PR
- Verify no downgrade of security-critical dependencies
- retract directives that could force consumers to upgrade to
specific versions (potential for malicious version steering)- toolchain directive changes (Go 1.21+) that force specific Go
toolchain downloads from untrusted sources
Files:
packages/agent-ops/bff/go.mod
🪛 LanguageTool
packages/agent-ops/frontend/GAPS.md
[uncategorized] ~44-~44: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... cross-module navigation which causes a full page reload. Consider using a shared navigat...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
packages/agent-ops/V1-IMPLEMENTATION-GAPS.md
[uncategorized] ~91-~91: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...es window.location.href** - Causes full page reload instead of SPA navigation - S...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.23.0)
packages/agent-ops/frontend/GAPS.md
[warning] 24-24: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 29-29: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 36-36: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 40-40: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 46-46: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 54-54: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
packages/agent-ops/V1-IMPLEMENTATION-GAPS.md
[warning] 14-14: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 36-36: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 72-72: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/2/3
(MD029, ol-prefix)
[warning] 76-76: Ordered list item prefix
Expected: 2; Actual: 5; Style: 1/2/3
(MD029, ol-prefix)
[warning] 80-80: Ordered list item prefix
Expected: 3; Actual: 6; Style: 1/2/3
(MD029, ol-prefix)
[warning] 85-85: Ordered list item prefix
Expected: 4; Actual: 7; Style: 1/2/3
(MD029, ol-prefix)
[warning] 90-90: Ordered list item prefix
Expected: 5; Actual: 8; Style: 1/2/3
(MD029, ol-prefix)
[warning] 94-94: Ordered list item prefix
Expected: 6; Actual: 9; Style: 1/2/3
(MD029, ol-prefix)
[warning] 98-98: Ordered list item prefix
Expected: 7; Actual: 10; Style: 1/2/3
(MD029, ol-prefix)
[warning] 105-105: Ordered list item prefix
Expected: 1; Actual: 11; Style: 1/2/3
(MD029, ol-prefix)
[warning] 106-106: Ordered list item prefix
Expected: 2; Actual: 12; Style: 1/2/3
(MD029, ol-prefix)
[warning] 107-107: Ordered list item prefix
Expected: 3; Actual: 13; Style: 1/2/3
(MD029, ol-prefix)
[warning] 108-108: Ordered list item prefix
Expected: 4; Actual: 14; Style: 1/2/3
(MD029, ol-prefix)
[warning] 109-109: Ordered list item prefix
Expected: 5; Actual: 15; Style: 1/2/3
(MD029, ol-prefix)
[warning] 110-110: Ordered list item prefix
Expected: 6; Actual: 16; Style: 1/2/3
(MD029, ol-prefix)
🔇 Additional comments (18)
packages/agent-ops/frontend/GAPS.md (1)
1-57: LGTM!packages/agent-ops/bff/internal/models/agent_runtime_detail.go (1)
30-30: LGTM!packages/agent-ops/frontend/src/app/api/agentRuntimes.ts (1)
3-8: LGTM!Also applies to: 64-70
packages/agent-ops/frontend/src/app/hooks/useAgentCardDetail.ts (1)
1-29: LGTM!packages/agent-ops/frontend/src/app/components/DeleteAgentModal.tsx (1)
1-66: LGTM!packages/agent-ops/bff/internal/api/agent_chat_handler.go (1)
20-25: 🎯 Functional CorrectnessDrop the SSRF concern
validateAgentPathParamsalready rejects non-DNS1123ns/name, and the route is further gated byAttachNamespaceFromParamplusRequireAccessToAgentbeforeChatAgentHandlerruns.> Likely an incorrect or invalid review comment.backend/src/types.ts (1)
57-57: LGTM!backend/src/utils/constants.ts (1)
89-89: LGTM!frontend/src/concepts/areas/const.ts (1)
35-35: LGTM!packages/agent-ops/extensions.ts (1)
1-18: LGTM!packages/agent-ops/frontend/config/webpack.common.js (1)
251-251: LGTM!packages/agent-ops/bff/cmd/main.go (1)
56-61: 🎯 Functional CorrectnessDrop this warning.
packages/agent-ops/bff/internal/api/app.goalready falls back toagent-ops-demowhen the namespace flag is empty, so this does not break sandbox operations.> Likely an incorrect or invalid review comment.frontend/config/webpack.common.js (1)
319-325: 🩺 Stability & AvailabilityConfirm no runtime import of
react-dom/serverfrontend/config/webpack.common.js:323-325
alias: falseonly stubs the module. If any bundled dependency reachesreact-dom/server, the failure moves to runtime instead of build time.packages/agent-ops/bff/internal/config/environment.go (1)
115-121: LGTM!packages/agent-ops/bff/internal/models/health_check.go (1)
7-16: LGTM!packages/agent-ops/bff/internal/integrations/agents/openshell/factory.go (1)
31-47: LGTM!packages/agent-ops/bff/internal/integrations/agents/openshell/client.go (1)
364-372: 🎯 Functional CorrectnessRemove this concern:
agentPodName()only populates status. The probe and chat paths build their URLs fromsb.Namedirectly, so this fallback does not affect connectivity here.> Likely an incorrect or invalid review comment.packages/agent-ops/bff/internal/api/app.go (1)
267-269: 🔒 Security & PrivacyNo SSRF path here.
nsandnameare validated as DNS-1123 labels before the proxy URL is built, so they can’t introduce an arbitrary host or scheme.> Likely an incorrect or invalid review comment.
| const agentOpsProxies = mfProxies.filter((p) => p.startsWith('/agent-ops')); | ||
| const otherMfProxies = mfProxies.filter((p) => !p.startsWith('/agent-ops')); | ||
| return [ | ||
| ...(agentOpsProxies.length > 0 | ||
| ? [ | ||
| { | ||
| context: agentOpsProxies, | ||
| target: `http://0.0.0.0:${process.env.AGENT_OPS_PORT || 4001}`, | ||
| pathRewrite: { '^/agent-ops': '' }, | ||
| }, | ||
| ] | ||
| : []), | ||
| { | ||
| context: ['/api', '/_mf', '/mlflow', ...mfProxies], | ||
| context: ['/api', '/_mf', '/mlflow', ...otherMfProxies], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="frontend/config/webpack.dev.js"
echo "== relevant proxy blocks =="
rg -n "agentOpsProxies|getCurrentToken|shouldFwdAccessToken|onProxyReq|Authorization|x-forwarded-access-token|auth-method|agent-ops" "$file"
echo
echo "== surrounding lines =="
sed -n '220,290p' "$file"Repository: opendatahub-io/odh-dashboard
Length of output: 4257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== agent-ops references =="
rg -n "AGENT_OPS_PORT|agent-ops|auth-method=user_token|x-forwarded-access-token|getCurrentToken|shouldFwdAccessToken" . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'
echo
echo "== files containing AGENT_OPS_PORT =="
rg -n "AGENT_OPS_PORT" frontend . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'
echo
echo "== package/service hints around agent-ops =="
fd -H "agent-ops|AgentOps|agentOps" .Repository: opendatahub-io/odh-dashboard
Length of output: 44301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' packages/agent-ops/frontend/config/webpack.dev.js
echo
sed -n '1,260p' packages/agent-ops/bff/README.md | sed -n '145,220p'Repository: opendatahub-io/odh-dashboard
Length of output: 8019
Forward the user token in the agent-ops dev proxy
frontend/config/webpack.dev.js:258-271 needs the same auth forwarding as the neighboring proxy blocks. Without onProxyReq or equivalent headers, the local agent-ops BFF gets no x-forwarded-access-token/Authorization, and AUTH_METHOD=user_token dev requests fail (CWE-306, CWE-287).
🤖 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 `@frontend/config/webpack.dev.js` around lines 258 - 271, Update the
agentOpsProxies entry in the webpack dev proxy configuration to forward the user
token using the same onProxyReq or equivalent header logic as the neighboring
proxy blocks. Ensure both x-forwarded-access-token and Authorization are
propagated to the agent-ops target while preserving its existing context,
target, and pathRewrite settings.
| ## RBAC / SSAR for Sandbox CRs | ||
|
|
||
| - The deploy RBAC checks now verify `agents.x-k8s.io/sandboxes` create/get, but there is no SSAR | ||
| check for delete/patch (used by DeleteAgent, StopAgent, StartAgent). Story 73189 covers this | ||
| but is unassigned. | ||
| - The enrichment RBAC still checks `agents.x-k8s.io/sandboxes` get/list for card enrichment | ||
| access, which is correct but may need refinement once the Sandbox CRD stabilizes. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Missing SSAR authorization for delete/patch — CWE-862 Missing Authorization.
Stop/Start/Delete operate on Sandbox CRs with no per-request SubjectAccessReview check. If the BFF's own service account has broader RBAC than the calling user (typical for a BFF pattern), any user reaching these routes can stop/start/delete sandboxes they aren't authorized to touch. This should not ship as a "should-address" backlog item — it's an authz bypass on destructive operations.
🤖 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 `@packages/agent-ops/bff/GAPS.md` around lines 5 - 11, Update DeleteAgent,
StopAgent, and StartAgent to perform a per-request SubjectAccessReview for the
relevant Sandbox CR delete/patch action before executing it, using the
authenticated caller’s identity and target resource context. Deny the operation
when SSAR is not allowed, and remove the documentation framing this
authorization as an unassigned backlog item.
| ## Route creation for external access | ||
|
|
||
| - Routes are created without an ownerReference when using the Sandbox CR path (unlike the old | ||
| AgentRuntime path where the CR was the owner). The Sandbox controller may auto-create Routes | ||
| in the future. Needs alignment with the controller team. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Routes created without OwnerReference — orphaned child resources.
Routes created on the Sandbox CR path have no ownerReference, so deleting the Sandbox leaves orphaned Route objects (resource leak, and a stale-endpoint exposure risk if the Route continues routing traffic to a torn-down agent). Go security conventions for controllers require OwnerReferences on all child resources.
🤖 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 `@packages/agent-ops/bff/GAPS.md` around lines 20 - 24, Update the Sandbox CR
route-creation flow so every created Route includes an OwnerReference pointing
to its owning Sandbox resource. Ensure deletion of the Sandbox garbage-collects
the Route, while preserving the existing ownerReference behavior for the
AgentRuntime path.
Source: Path instructions
| go 1.25.0 | ||
|
|
||
| require ( | ||
| github.com/getkin/kin-openapi v0.132.0 | ||
| github.com/google/uuid v1.6.0 | ||
| github.com/julienschmidt/httprouter v1.3.0 | ||
| github.com/rhuss/openshell-sdk-go v0.2.2 | ||
| github.com/rs/cors v1.11.1 | ||
| github.com/stretchr/testify v1.11.0 | ||
| github.com/stretchr/testify v1.11.1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is github.com/rhuss/openshell-sdk-go an official or community Go SDK for the OpenShell project (github.com/NVIDIA/OpenShell)?
💡 Result:
No, github.com/rhuss/openshell-sdk-go is not an official community Go SDK for the NVIDIA OpenShell project [1][2]. It is a third-party project created by a contributor who proposed it as a potential starting point for an official Go SDK [1]. As of July 13, 2026, the NVIDIA OpenShell project has not adopted this repository, and official development efforts for OpenShell SDKs are currently focused on other languages and architectures (such as Rust and Python) as outlined in the project's official RFCs [3]. Official communication and documentation for the project are hosted within the main NVIDIA/OpenShell GitHub repository and the official NVIDIA documentation site [2][4][5].
Citations:
- 1: feat(sdk): proposal for Go SDK following client-go conventions NVIDIA/OpenShell#2044
- 2: https://github.com/nvidia/openShell
- 3: rfc-0007: SDK design principles and entrypoints NVIDIA/OpenShell#1590
- 4: https://docs.nvidia.com/openshell/home
- 5: https://docs.nvidia.com/openshell/latest/about/installation
Unvetted OpenShell SDK dependency (CWE-829). github.com/rhuss/openshell-sdk-go is a third-party personal-account repository, not the official NVIDIA/OpenShell Go SDK. Pulling this client into go.mod adds supply-chain risk; use an upstream-maintained package or vendor the code only after provenance review.
🤖 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 `@packages/agent-ops/bff/go.mod` around lines 3 - 11, Replace the unvetted
github.com/rhuss/openshell-sdk-go dependency in go.mod with the official
NVIDIA/OpenShell Go SDK or an upstream-maintained equivalent, updating imports
and usages throughout the affected code to match its API. If no suitable
upstream package exists, remove the dependency and vendor an approved,
provenance-reviewed implementation instead.
Source: Path instructions
| for key, values := range resp.Header { | ||
| for _, v := range values { | ||
| w.Header().Add(key, v) | ||
| } | ||
| } | ||
| w.WriteHeader(resp.StatusCode) | ||
| io.Copy(w, resp.Body) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing io.LimitReader on proxied response body; io.Copy error swallowed.
Path instructions require io.LimitReader for HTTP response bodies to prevent memory/resource exhaustion. Here resp.Body is copied to w unbounded, and io.Copy's error return is discarded so partial/failed proxy writes go unnoticed. All response headers (including hop-by-hop ones like Transfer-Encoding/Connection) are also forwarded verbatim without filtering.
As per path instructions, "Use io.LimitReader for HTTP response bodies (prevent memory exhaustion)."
🛡️ Proposed fix
- w.WriteHeader(resp.StatusCode)
- io.Copy(w, resp.Body)
+ w.WriteHeader(resp.StatusCode)
+ if _, copyErr := io.Copy(w, io.LimitReader(resp.Body, maxChatResponseBytes)); copyErr != nil {
+ logger.Error("Failed to stream agent chat response", slog.Any("error", copyErr))
+ }🤖 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 `@packages/agent-ops/bff/internal/api/agent_chat_handler.go` around lines 54 -
60, Update the response-body proxying flow around the handler’s header
forwarding and io.Copy call to copy through io.LimitReader with the configured
response-size limit, handle the copy error instead of discarding it, and filter
hop-by-hop headers such as Transfer-Encoding and Connection before forwarding
them. Preserve the upstream status code and bounded body forwarding behavior.
Source: Path instructions
| spec := map[string]any{ | ||
| "operatingMode": status, | ||
| } | ||
| if sb.Spec.Template != nil { | ||
| spec["image"] = sb.Spec.Template.Image | ||
| spec["environment"] = sb.Spec.Environment | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect relevant regions around the referenced lines and related methods.
ast-grep outline packages/agent-ops/bff/internal/integrations/agents/openshell/client.go --view expanded || true
printf '\n--- lines 280-380 ---\n'
sed -n '280,380p' packages/agent-ops/bff/internal/integrations/agents/openshell/client.go
printf '\n--- search for operatingMode / Phase / StopAgent / StartAgent ---\n'
rg -n 'operatingMode|Phase|StopAgent|StartAgent|Spec\.OperatingMode|operatingMode"' packages/agent-ops/bff/internal/integrations/agents/openshell/client.goRepository: opendatahub-io/odh-dashboard
Length of output: 4743
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant type definitions ---'
rg -n 'type AgentDetail|operatingMode|ReadyStatus|phase|AgentService|AgentMetadata' packages/agent-ops -g '!**/*_test.go'
printf '\n%s\n' '--- callers/usages of AgentDetail.Spec operatingMode ---'
rg -n '"operatingMode"|Spec\[.*operatingMode|ReadyStatus|AgentDetail' packages/agent-ops packages -g '!**/*_test.go'
printf '\n%s\n' '--- surrounding code for StopAgent/StartAgent ---'
sed -n '145,205p' packages/agent-ops/bff/internal/integrations/agents/openshell/client.goRepository: opendatahub-io/odh-dashboard
Length of output: 30631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sandbox type definitions / operatingMode field ---'
rg -n 'type Sandbox|type .*Spec|OperatingMode|operatingMode' packages/agent-ops -g '!**/*_test.go'
printf '\n%s\n' '--- open-shell client type imports and any spec field access in the file ---'
sed -n '1,140p' packages/agent-ops/bff/internal/integrations/agents/openshell/client.go
printf '\n%s\n' '--- sandbox API / CRD references in repo ---'
rg -n 'operatingMode' . -g '!**/*_test.go' | sed -n '1,120p'Repository: opendatahub-io/odh-dashboard
Length of output: 8960
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- AgentDetail type ---'
sed -n '1,140p' packages/agent-ops/bff/internal/integrations/agents/types.go
printf '\n%s\n' '--- runtime detail model ---'
sed -n '1,120p' packages/agent-ops/bff/internal/models/agent_runtime_detail.go
printf '\n%s\n' '--- mapper from AgentDetail ---'
sed -n '1,220p' packages/agent-ops/bff/internal/mapper/agent.go
printf '\n%s\n' '--- frontend references to operatingMode ---'
rg -n 'operatingMode|readyStatus|ReadyStatus|AgentRuntimeDetail|AgentDetail' packages/agent-ops/frontend packages/agent-ops/bff -g '!**/*_test.go'Repository: opendatahub-io/odh-dashboard
Length of output: 25381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- AgentDetail schema in OpenAPI ---'
rg -n 'AgentDetail:|operatingMode|spec:|readyStatus|status:' packages/agent-ops/bff/openapi/src/agent-ops.yaml
printf '\n%s\n' '--- AgentRuntimeDetail schema in OpenAPI around definition ---'
sed -n '424,520p' packages/agent-ops/bff/openapi/src/agent-ops.yamlRepository: opendatahub-io/odh-dashboard
Length of output: 3244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- where AgentDetail.Spec is read ---'
rg -n '\.Spec\b|Spec\]' packages/agent-ops/bff packages/agent-ops/frontend -g '!**/*_test.go'
printf '\n%s\n' '--- AgentCardDetail model/schema around any spec-bearing fields ---'
rg -n 'type AgentCardDetail|spec|Spec' packages/agent-ops/bff/internal/models packages/agent-ops/bff/openapi/src/agent-ops.yaml
printf '\n%s\n' '--- mapper files that consume AgentDetail ---'
sed -n '1,220p' packages/agent-ops/bff/internal/mapper/agent_card.goRepository: opendatahub-io/odh-dashboard
Length of output: 10111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- AgentDetail in OpenAPI / API handlers ---'
rg -n 'AgentDetail|GetAgentHandler|GetAgentRuntimeDetailHandler|AgentRuntimeDetailResponse|AgentDetailResponse' packages/agent-ops/bff/openapi/src packages/agent-ops/bff/internal/api packages/agent-ops/frontend -g '!**/*_test.go'
printf '\n%s\n' '--- any direct reads of spec.operatingMode ---'
rg -n 'spec\.operatingMode|operatingMode"\]|operatingMode\]' packages/agent-ops/bff packages/agent-ops/frontend -g '!**/*_test.go'Repository: opendatahub-io/odh-dashboard
Length of output: 909
Populate operatingMode from the Sandbox spec, not Status.Phase packages/agent-ops/bff/internal/integrations/agents/openshell/client.go:326-333 currently writes Ready/Pending/Unknown into spec.operatingMode, while stop/start patch the actual spec value to Suspended/Running. That conflates readiness with configuration; read the spec field here or drop this key if the response is meant to expose only phase.
🤖 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 `@packages/agent-ops/bff/internal/integrations/agents/openshell/client.go`
around lines 326 - 333, Update the spec map construction in the relevant Sandbox
response logic to source operatingMode from the Sandbox spec rather than the
status phase, preserving values such as Suspended and Running; alternatively
remove the operatingMode key if this response should expose only phase. Do not
populate it from status.
| @@ -4,7 +4,7 @@ ARG BFF_SOURCE_CODE=./bff | |||
|
|
|||
| # Set the base images for the build stages | |||
| ARG NODE_BASE_IMAGE=node:22 | |||
| ARG GOLANG_BASE_IMAGE=golang:1.24.3 | |||
| ARG GOLANG_BASE_IMAGE=golang:1.25 | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Unpinned floating tag — supply chain risk (CWE-829).
golang:1.25 floats to the latest patch/point release on every rebuild. Untested toolchain changes get pulled in silently, and the build is not reproducible or attestable. Pin to a digest (golang:1.25@sha256:...) or at minimum a full patch version.
🤖 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 `@packages/agent-ops/Dockerfile` at line 7, Update the GOLANG_BASE_IMAGE
argument to use a reproducible, immutable image reference by pinning golang to a
specific full patch version and preferably its sha256 digest; do not leave it as
the floating golang:1.25 tag.
| @@ -9,7 +9,7 @@ ARG BFF_SOURCE_CODE=./packages/${MODULE_NAME}/bff | |||
|
|
|||
| # Set the base images for the build stages | |||
| ARG NODE_BASE_IMAGE=registry.access.redhat.com/ubi9/nodejs-22:latest | |||
| ARG GOLANG_BASE_IMAGE=registry.access.redhat.com/ubi9/go-toolset:1.24 | |||
| ARG GOLANG_BASE_IMAGE=docker.io/library/golang:1.25 | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Unpinned floating tag — same supply chain concern as the main Dockerfile.
See consolidated comment.
🤖 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 `@packages/agent-ops/Dockerfile.workspace` at line 12, Update the
GOLANG_BASE_IMAGE argument in the workspace Dockerfile to use a fixed, immutable
image digest instead of the floating golang:1.25 tag, preserving the intended Go
1.25 base image.
| 10. **Env var secrets via k8s Secret** (RHOAIENG-73640, New) | ||
| - Deploy wizard creates plain env vars visible via `oc describe` | ||
| - Secret creation story exists but unassigned | ||
| - Security hygiene gap for API_KEY values | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
API keys stored as plaintext env vars — CWE-312 cleartext storage of sensitive info.
Item 10 documents that the deploy wizard writes API_KEY-style values as plain env vars, visible to anyone with oc describe/get pod -o yaml RBAC. This is shipping a real credential-exposure vector, not a cosmetic gap — recommend gating this in v1 (block plaintext secret input, or require k8s Secret creation) rather than deferring to an unassigned story.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 98-98: Ordered list item prefix
Expected: 7; Actual: 10; Style: 1/2/3
(MD029, ol-prefix)
🤖 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 `@packages/agent-ops/V1-IMPLEMENTATION-GAPS.md` around lines 98 - 102, Update
item 10, “Env var secrets via k8s Secret,” to require v1 gating: block plaintext
API_KEY-style inputs or require creation and reference of a Kubernetes Secret
before deployment. Do not leave plaintext environment-variable storage as an
unassigned follow-up; document the deployment wizard behavior and acceptance
criteria accordingly.
d5f4cb2 to
c87631b
Compare
![odh-dashboard-agent[bot]](https://avatars.githubusercontent.com/in/3714079?s=60&v=4){kind=small}
There was a problem hiding this comment.
Preflight Agent Report
Verdict: ❌ NOT READY
Commit: 537e7bd
Checks
| Check | Status | Details |
|---|---|---|
| Conflicts | ❌ | PR has a merge conflict — rebase required |
| CI | router ✅; Prow image/mirror jobs ❌ (ran on old base SHA before conflict; tide blocked: "Not mergeable"); Konflux |
|
| Lint | GHA Lint job pending (router passed, downstream jobs still running) | |
| Type Check | GHA Type-Check job pending | |
| Unit Tests | GHA Unit-Tests job pending | |
| Jira | ❌ | No Jira key found in PR title, body, or branch |
| Test Coverage | ❌ | 29 files changed, no test files added; no Test Impact explanation in PR body |
| PR Body | ❌ | Does not follow template: uses ## Summary instead of ## Description; missing ## How Has This Been Tested?, ## Test Impact, checklist, and Jira URL; has .tsx UI changes with no screenshots |
| Review (Claude) | 🟠 2 major | Wrong Spec.Environment field (confirmed); hop-by-hop headers forwarded in chat proxy (confirmed) — see inline comments |
Automated by ODH Dashboard Agent
| } | ||
| if sb.Spec.Template != nil { | ||
| spec["image"] = sb.Spec.Template.Image | ||
| spec["environment"] = sb.Spec.Environment |
There was a problem hiding this comment.
🟠 Major · Claude review
Wrong field: reads sb.Spec.Environment instead of sb.Spec.Template.Environment, silently discarding deployed env vars.
The guard above checks sb.Spec.Template != nil, so Template-level fields are intended here. sb.Spec.Environment is a separate outer Spec field that is never populated by DeployAgent — which writes env vars to SandboxSpec.Template.Environment. Any UI that displays the agent's environment variables will always show empty regardless of what was set at deploy time.
Suggested fix
- spec["environment"] = sb.Spec.Environment
+ spec["environment"] = sb.Spec.Template.Environment| defer resp.Body.Close() | ||
|
|
||
| // Forward the agent's response headers and body back to the client | ||
| for key, values := range resp.Header { |
There was a problem hiding this comment.
🟠 Major · Claude review
Hop-by-hop headers (e.g. Transfer-Encoding, Connection) forwarded verbatim to the client, corrupting chunked responses.
All upstream agent response headers — including hop-by-hop headers that must not be retransmitted — are copied to the downstream writer with no filtering. When the upstream agent sends Transfer-Encoding: chunked, the Go HTTP server applies its own chunked framing on top, resulting in double-encoded responses that clients cannot parse. Filter hop-by-hop headers before forwarding.
Suggested fix
+ hopByHop := map[string]bool{
+ "Connection": true, "Keep-Alive": true, "Transfer-Encoding": true,
+ "Trailer": true, "Te": true, "Upgrade": true, "Proxy-Authorization": true,
+ }
for key, values := range resp.Header {
+ if hopByHop[key] {
+ continue
+ }
for _, v := range values {
w.Header().Add(key, v)
}
}| } | ||
|
|
||
| if cfg.AuthMethod == config.AuthMethodDisabled && !cfg.MockAgentClient { | ||
| if cfg.AuthMethod == config.AuthMethodDisabled && !cfg.MockAgentClient && cfg.OpenShellGatewayURL == "" { |
There was a problem hiding this comment.
🟠 Major · Claude review
Security gate weakened: AUTH_METHOD=disabled + OpenShellGatewayURL set allows real agent routes with no authentication.
The original guard ensured AUTH_METHOD=disabled was only safe with the mock client. The new condition allows a real OpenShell Gateway URL to bypass this check — the gateway is configured with v1.NoAuth() and the RequireAccessToAgent middleware short-circuits entirely when auth is disabled. An operator who sets OPENSHELL_GATEWAY_URL in an environment with AUTH_METHOD=disabled (e.g. misconfigured staging) will expose live agent CRUD routes without any auth. The change should at minimum log a prominent warning when this combination is detected, and the production operator deployment should enforce AUTH_METHOD != disabled.
- OpenShell SDK integration: deploy/list/get/delete via Gateway, stop/start via CR patch - Bundled area extension for nav visibility without federated module - BFF: endpoint URL, service info, pod name/count from sandbox status - Backend type fix: agentOps in DashboardConfig for /api/config passthrough - Webpack: resolve fallbacks for Node.js built-ins, agent-ops proxy split - Go 1.25 base image for OpenShell SDK compatibility - getAgentCard API function for agent card detail hook Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
c87631b to
b6fa36d
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
frontend/config/webpack.dev.js (1)
261-267: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winForward the authentication context through the Agent Ops proxy.
The new proxy only sets
context,target, andpathRewrite. It still lacks the token-forwarding logic from the existing authenticated proxy path, so localAUTH_METHOD=user_tokenrequests arrive withoutx-forwarded-access-token/Authorizationand fail authentication. This is an authentication integration failure, not an auth bypass.Reuse the neighboring proxy’s header-forwarding logic.
As per path instructions, webpack configuration changes must be reviewed for security and broken build logic.🤖 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 `@frontend/config/webpack.dev.js` around lines 261 - 267, Update the Agent Ops proxy entry in the webpack configuration to reuse the neighboring authenticated proxy’s header-forwarding logic, forwarding both x-forwarded-access-token and Authorization for local AUTH_METHOD=user_token requests. Keep the existing context, target, and pathRewrite behavior unchanged.Source: Path instructions
packages/agent-ops/src/AgentDeploymentsPage.tsx (1)
59-65: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the validated runtime API helper before rendering.
json.data?.runtimes ?? []only handles an absent value. A non-array response reachesagents.length/agents.map()and crashes rendering, while this page also bypasses the existing validated API layer inpackages/agent-ops/frontend/src/app/api/agentRuntimes.ts.Reuse that helper and verify that
runtimesis an array before callingsetAgents.
As per path instructions, API responses must be validated before rendering.🤖 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 `@packages/agent-ops/src/AgentDeploymentsPage.tsx` around lines 59 - 65, Update the runtime-loading logic in AgentDeploymentsPage to reuse the validated helper from agentRuntimes.ts instead of directly parsing fetch JSON. Before calling setAgents, ensure the returned runtimes value is an array, using the existing helper’s validation and fallback behavior so agents always contains render-safe data.Source: Path instructions
🤖 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 `@frontend/config/webpack.common.js`:
- Around line 319-322: Update the resolve.conditionNames configuration near the
fallback setting to preserve Webpack’s default export conditions, including
webpack and browser. Add the spread defaults marker alongside the existing
conditions or remove the override entirely, while keeping the fallback
diagnostics_channel configuration unchanged.
- Around line 243-245: Update the NormalModuleReplacementPlugin callback to
apply the node: prefix removal to both resource.request and
resource.createData.request, preserving the existing replacement behavior for
each request value.
In `@packages/agent-ops/bff/cmd/main.go`:
- Around line 56-58: Update the OpenShell gateway configuration used by the
openshell-gateway-url flag and the client construction in app.go to require
verified TLS with a configured CA bundle. Remove or disable the
TLSConfig{Insecure: true} behavior, and ensure privileged agent operations
cannot proceed unless certificate verification is enabled.
In `@packages/agent-ops/bff/GAPS.md`:
- Around line 15-18: Reconcile the endpoint documentation in GAPS.md with the
route registrations in app.go: remove or revise the undocumented /restart and
/card endpoint entries unless they are actually registered, and document the
existing GetAgent response enrichment with card data instead. Verify the delete,
stop, start, and chat routes remain accurately represented before updating
OpenAPI.
- Around line 28-31: Update the GetAgent card-probe host selection to use the
Sandbox CR status.serviceFQDN value first, falling back to the constructed
service host only when that field is empty; preserve the existing probe behavior
otherwise.
In `@packages/agent-ops/bff/internal/api/app.go`:
- Line 127: Update the configuration branch around cfg.AuthMethod,
cfg.MockAgentClient, and cfg.OpenShellGatewayURL to reject real OpenShell when
authentication is disabled: require cfg.MockAgentClient to be true, or otherwise
fail configuration before constructing the unauthenticated client. Preserve the
existing behavior for mock-agent mode and configured gateway URLs.
In `@packages/agent-ops/bff/internal/integrations/agents/openshell/client.go`:
- Line 26: Harden cardProbeClient against SSRF by disabling automatic redirect
following, so agent-controlled Location responses cannot reach internal
endpoints. Validate the SDK-provided sandbox and namespace values as DNS-1123
labels before constructing the card-probe target URL, rejecting invalid values.
Apply these changes to every card-probe request path, including the usages near
the noted locations.
In `@packages/agent-ops/bff/internal/models/health_check.go`:
- Around line 7-10: Update OpenShellStatus to expose only the enabled field in
the unauthenticated health-check response, removing Gateway and Namespace plus
their JSON fields. Keep any diagnostic gateway or namespace data available only
through an existing admin-only endpoint, without exposing it from /healthcheck.
- Line 16: Add openshell to the health response schema and optional podName to
the runtime-detail schema in packages/agent-ops/api/openapi/agent-ops.yaml, then
synchronize packages/agent-ops/bff/openapi/src/agent-ops.yaml with the OpenAPI
definitions and the OpenShellStatus, health-check, and agent runtime-detail
models in packages/agent-ops/bff/internal/models/health_check.go and
packages/agent-ops/bff/internal/models/agent_runtime_detail.go.
In `@packages/agent-ops/src/AgentDeploymentsPage.tsx`:
- Around line 60-67: Update the fetch error handling in AgentDeploymentsPage
around the !res.ok branch to stop including res.text() in the Error message
displayed through setError and the Alert. Throw a fixed generic failure message
for non-2xx responses, while preserving server-side logging of the raw response
body if an existing logging path is available.
---
Duplicate comments:
In `@frontend/config/webpack.dev.js`:
- Around line 261-267: Update the Agent Ops proxy entry in the webpack
configuration to reuse the neighboring authenticated proxy’s header-forwarding
logic, forwarding both x-forwarded-access-token and Authorization for local
AUTH_METHOD=user_token requests. Keep the existing context, target, and
pathRewrite behavior unchanged.
In `@packages/agent-ops/src/AgentDeploymentsPage.tsx`:
- Around line 59-65: Update the runtime-loading logic in AgentDeploymentsPage to
reuse the validated helper from agentRuntimes.ts instead of directly parsing
fetch JSON. Before calling setAgents, ensure the returned runtimes value is an
array, using the existing helper’s validation and fallback behavior so agents
always contains render-safe data.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: eefd1780-37bf-432a-a275-cee394117cf8
⛔ Files ignored due to path filters (1)
packages/agent-ops/bff/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (22)
backend/src/types.tsbackend/src/utils/constants.tsfrontend/config/webpack.common.jsfrontend/config/webpack.dev.jsfrontend/src/concepts/areas/const.tspackages/agent-ops/Dockerfilepackages/agent-ops/Dockerfile.workspacepackages/agent-ops/bff/GAPS.mdpackages/agent-ops/bff/cmd/main.gopackages/agent-ops/bff/go.modpackages/agent-ops/bff/internal/api/agent_chat_handler.gopackages/agent-ops/bff/internal/api/app.gopackages/agent-ops/bff/internal/api/healthcheck_handler.gopackages/agent-ops/bff/internal/config/environment.gopackages/agent-ops/bff/internal/integrations/agents/openshell/client.gopackages/agent-ops/bff/internal/integrations/agents/openshell/factory.gopackages/agent-ops/bff/internal/mapper/agent.gopackages/agent-ops/bff/internal/models/agent_runtime_detail.gopackages/agent-ops/bff/internal/models/health_check.gopackages/agent-ops/extensions.tspackages/agent-ops/frontend/src/app/api/agentRuntimes.tspackages/agent-ops/src/AgentDeploymentsPage.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (14)
- GitHub Check: Cypress-Setup (packages/autorag,
@odh-dashboard/autorag) - GitHub Check: Cypress-Setup (frontend, odh-dashboard-frontend)
- GitHub Check: Cypress-Setup (packages/agent-ops,
@odh-dashboard/agent-ops) - GitHub Check: Cypress-Setup (packages/automl,
@odh-dashboard/automl) - GitHub Check: Cypress-Setup (packages/eval-hub,
@odh-dashboard/eval-hub) - GitHub Check: Cypress-Setup (packages/mlflow,
@odh-dashboard/mlflow) - GitHub Check: Cypress-Setup (packages/maas,
@odh-dashboard/maas) - GitHub Check: Cypress-Setup (packages/gen-ai,
@odh-dashboard/gen-ai) - GitHub Check: Cypress-Setup (packages/model-registry,
@odh-dashboard/model-registry) - GitHub Check: Phase 1: Docker Build (ODH mode)
- GitHub Check: Phase 1: Docker Build (RHOAI mode)
- GitHub Check: Get-Test-Groups
- GitHub Check: Lint
- GitHub Check: Type-Check
🧰 Additional context used
📓 Path-based instructions (10)
**
⚙️ CodeRabbit configuration file
**: REVIEW PRIORITIES:
- Security vulnerabilities — provide severity, exploit scenario,
and remediation code. Cite CWE/CVE IDs.- Bugs that could reach production — logic errors, null/undefined,
race conditions, incorrect async handling, resource leaks.- API contract correctness — shape mismatches, missing error handling,
silent failures, wrong HTTP status codes.- Performance only when measurable — O(n^2) in hot paths, unbounded
memory growth, missing pagination.Do not comment on:
- Naming preferences (unless genuinely misleading)
- Import ordering or formatting (handled by ESLint and Prettier)
- Alternative patterns that are equally valid
- Missing docs unless a public API is genuinely unclear
- Code deduplication / DRY suggestions where both copies are short
and self-contained (< 20 lines)- Adding explicit type annotations when TypeScript can infer the type
- Suggesting exhaustive switch/if-else when a default branch exists
Files:
backend/src/types.tspackages/agent-ops/Dockerfile.workspacepackages/agent-ops/bff/internal/models/agent_runtime_detail.gopackages/agent-ops/bff/internal/api/healthcheck_handler.gofrontend/src/concepts/areas/const.tsfrontend/config/webpack.common.jspackages/agent-ops/bff/GAPS.mdpackages/agent-ops/extensions.tspackages/agent-ops/bff/cmd/main.gobackend/src/utils/constants.tspackages/agent-ops/bff/internal/integrations/agents/openshell/factory.gopackages/agent-ops/frontend/src/app/api/agentRuntimes.tspackages/agent-ops/Dockerfilepackages/agent-ops/bff/internal/config/environment.gopackages/agent-ops/src/AgentDeploymentsPage.tsxfrontend/config/webpack.dev.jspackages/agent-ops/bff/internal/models/health_check.gopackages/agent-ops/bff/internal/api/agent_chat_handler.gopackages/agent-ops/bff/go.modpackages/agent-ops/bff/internal/mapper/agent.gopackages/agent-ops/bff/internal/api/app.gopackages/agent-ops/bff/internal/integrations/agents/openshell/client.go
backend/src/**/*.{ts,js}
⚙️ CodeRabbit configuration file
backend/src/**/*.{ts,js}: ODH DASHBOARD BACKEND (Node.js BFF):
- All external API calls must use the service account token, never user-provided tokens.
- Validate and sanitize route parameters before K8s API calls (prevent injection).
- Proxy endpoints must not expose internal cluster addresses to the client.
- Error responses must not leak cluster internals (pod names, IPs, stack traces).
- Verify RBAC: backend routes should check user permissions via SubjectAccessReview.
Files:
backend/src/types.tsbackend/src/utils/constants.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
**/*.{ts,tsx,js,jsx}: WEB SECURITY (XSS, CSRF Prevention):
- No dangerouslySetInnerHTML without sanitization (XSS - CWE-79)
- Validate all API responses before rendering
- CSRF token validation for state-changing operations
- No sensitive data in localStorage
Files:
backend/src/types.tsfrontend/src/concepts/areas/const.tsfrontend/config/webpack.common.jspackages/agent-ops/extensions.tsbackend/src/utils/constants.tspackages/agent-ops/frontend/src/app/api/agentRuntimes.tspackages/agent-ops/src/AgentDeploymentsPage.tsxfrontend/config/webpack.dev.js
**/Dockerfile*
⚙️ CodeRabbit configuration file
**/Dockerfile*: DOCKERFILES:
- Multi-stage builds: separate build and runtime stages.
- Copy only necessary artifacts to runtime stage (no source code, node_modules, .git).
- Use the same base image version as the main Dockerfile where possible.
- No npm install in runtime stage — copy from build stage.
- Dockerfile.workspace files are for dev workspace images — follow the same base image
conventions.
Files:
packages/agent-ops/Dockerfile.workspacepackages/agent-ops/Dockerfile
packages/*/bff/**/*.go
⚙️ CodeRabbit configuration file
packages/*/bff/**/*.go: GO BFF SERVICE (Backend-for-Frontend):
Packages with BFFs: automl, autorag, eval-hub, gen-ai, maas, mlflow.
- Validate all incoming request bodies and query parameters at handler level.
- No credentials in BFF code — use mounted secrets or environment variables.
- Use the shared K8s client config; do not create ad-hoc kubeconfig readers.
- HTTP clients to external services must set timeouts and use TLS verification.
- No panic in handlers — use explicit error returns with context.
- OpenAPI spec in api/ or docs/ must match actual handler signatures.
- Follow .golangci.yaml rules in the package root.
- Repository pattern: data access through repository interfaces, not direct K8s calls
in handlers. Mock interfaces for unit testing (see mocks/ directory).
Files:
packages/agent-ops/bff/internal/models/agent_runtime_detail.gopackages/agent-ops/bff/internal/api/healthcheck_handler.gopackages/agent-ops/bff/cmd/main.gopackages/agent-ops/bff/internal/integrations/agents/openshell/factory.gopackages/agent-ops/bff/internal/config/environment.gopackages/agent-ops/bff/internal/models/health_check.gopackages/agent-ops/bff/internal/api/agent_chat_handler.gopackages/agent-ops/bff/internal/mapper/agent.gopackages/agent-ops/bff/internal/api/app.gopackages/agent-ops/bff/internal/integrations/agents/openshell/client.go
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: GO SECURITY (Kubernetes Controllers):
- Use io.LimitReader for HTTP response bodies (prevent memory exhaustion)
- Validate all data from json.Unmarshal before storing in ConfigMaps/Secrets
- No InsecureSkipVerify in TLS configs (enables MITM attacks)
- Validate CR spec fields before using in ConfigMaps/Secrets
- Set OwnerReferences on all child resources
- Validate inputs before exec.Command (prevent command injection)
- Never log sensitive fields (Password, Token, APIKey, Secret.Data)
- Avoid weak cryptography (MD5, SHA1) for security operations
Files:
packages/agent-ops/bff/internal/models/agent_runtime_detail.gopackages/agent-ops/bff/internal/api/healthcheck_handler.gopackages/agent-ops/bff/cmd/main.gopackages/agent-ops/bff/internal/integrations/agents/openshell/factory.gopackages/agent-ops/bff/internal/config/environment.gopackages/agent-ops/bff/internal/models/health_check.gopackages/agent-ops/bff/internal/api/agent_chat_handler.gopackages/agent-ops/bff/internal/mapper/agent.gopackages/agent-ops/bff/internal/api/app.gopackages/agent-ops/bff/internal/integrations/agents/openshell/client.go
frontend/src/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
frontend/src/**/*.{ts,tsx}: ODH DASHBOARD FRONTEND (main app):
- PatternFly v6: use PF6 imports from
@patternfly/react-core, not custom wrappers.
Avoid custom CSS — if you need to "nudge" PF layout, check frontend/src/concepts/dashboard first.- Functional components only (no class components). Use hooks for state management.
- API calls: use the shared API utilities, never raw fetch(); handle loading/error states.
- No hardcoded cluster URLs or API endpoints — use config from backend.
- Route guards: protected routes must check user permissions before rendering.
- Performance: avoid unnecessary useCallback/useMemo/useRef — React is performant by default.
Only use useCallback when the function is passed as a prop, used as a useEffect dependency,
or returned from a custom hook (see docs/best-practices.md).- Custom components go in frontend/src/components. PF-first: verify with the team before
creating new custom components.
Files:
frontend/src/concepts/areas/const.ts
**/webpack.{common,dev,prod}.{ts,js}
⚙️ CodeRabbit configuration file
**/webpack.{common,dev,prod}.{ts,js}: WEBPACK CONFIG:
(Suppression) Do not suggest alternative bundler configurations
or plugin replacements. Only flag security issues and broken build logic.
Files:
frontend/config/webpack.common.jsfrontend/config/webpack.dev.js
packages/*/frontend/src/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
packages/*/frontend/src/**/*.{ts,tsx}: FEATURE PLUGIN FRONTEND (Module Federation):
These packages (automl, autorag, eval-hub, gen-ai, maas, mlflow) use Module Federation
to load as remotes into the host dashboard app.
- Plugins must use plugin-core APIs for navigation, not direct router manipulation.
- Shared dependencies (React, PatternFly, Redux) must come from the host app — do not
bundle duplicates.- No global CSS — use PatternFly utility classes or CSS modules only.
- Lazy-load heavy components; plugins load on demand via Module Federation.
- Follow PatternFly v6 patterns consistent with the main frontend app.
Files:
packages/agent-ops/frontend/src/app/api/agentRuntimes.ts
**/go.mod
⚙️ CodeRabbit configuration file
**/go.mod: GO DEPENDENCY SECURITY (CWE-829):
- Verify new dependencies are from trusted organizations
- Check for replace directives pointing to forks (supply chain risk).
Flag replace directives that redirect well-known modules
(golang.org/x/, k8s.io/, sigs.k8s.io/*) to personal forks- Flag indirect dependency additions unrelated to the PR
- Verify no downgrade of security-critical dependencies
- retract directives that could force consumers to upgrade to
specific versions (potential for malicious version steering)- toolchain directive changes (Go 1.21+) that force specific Go
toolchain downloads from untrusted sources
Files:
packages/agent-ops/bff/go.mod
🔇 Additional comments (18)
packages/agent-ops/bff/GAPS.md (3)
7-9: Previously reported: destructive Sandbox operations still lack per-request SSAR.This remains the documented CWE-862 authorization gap for DeleteAgent, StopAgent, and StartAgent. Do not treat an unassigned story as sufficient mitigation.
22-24: Previously reported: Sandbox-created Routes still lack ownerReferences.This remains the documented orphaned-resource risk. The controller path must establish ownership or explicitly guarantee cleanup.
49-51: 🎯 Functional CorrectnessClarify Sandbox CRD cache behavior. Add TTL/invalidation for
isSandboxCRDAvailable(), or document restart as a hard requirement; otherwise the BFF can keep using the Deployment fallback after the CRD appears.packages/agent-ops/bff/internal/api/healthcheck_handler.go (1)
17-22: Remove Gateway and Namespace from the unauthenticated healthcheck response (CWE-200). This remains the previously reported internal-information disclosure.packages/agent-ops/Dockerfile (1)
7-7: Pin the Go builder image to an immutable digest (CWE-829).golang:1.25remains a floating tag.packages/agent-ops/Dockerfile.workspace (1)
12-12: Pin the Go builder image to an immutable digest (CWE-829).docker.io/library/golang:1.25remains a floating tag.packages/agent-ops/bff/internal/api/app.go (1)
184-201: Replace hard-codedNoAuth()and insecure TLS (CWE-295, CWE-306). This remains the previously reported OpenShell gateway transport vulnerability.packages/agent-ops/bff/internal/api/agent_chat_handler.go (1)
54-60: Bound proxied responses and filter hop-by-hop headers (CWE-400). This remains the previously reported unbounded response-body and header-forwarding issue.packages/agent-ops/bff/go.mod (2)
9-9: Previously flagged supply-chain risk remains (CWE-829).
github.com/rhuss/openshell-sdk-goremains a personal-account dependency rather than an upstream-maintained OpenShell SDK. The existing provenance-review comment still applies tov0.2.2.Source: Path instructions
3-8: LGTM!Also applies to: 10-11, 54-62
packages/agent-ops/bff/internal/integrations/agents/openshell/client.go (4)
41-45: Namespace authorization mismatch remains (CWE-863).
CanListAgentsInNamespacealways allows access, reads/deletes discard the authorized namespace, while lifecycle patches accept it. The previously reported requirement to enforcenamespace == c.namespaceconsistently remains applicable.Also applies to: 59-60, 145-148, 159-198
280-291: Previously flagged:displayNameis still discarded.The annotation is parsed and then suppressed rather than mapped into the summary.
326-332: Previously flagged detail mapping defects remain.
operatingModestill receives readiness phase rather than the Sandbox spec value, and environment data is still read fromsb.Spec.Environmentinstead ofsb.Spec.Template.Environment.
20-39: LGTM!Also applies to: 87-143, 220-270, 293-308, 334-372
packages/agent-ops/bff/cmd/main.go (1)
59-61: LGTM!packages/agent-ops/bff/internal/integrations/agents/openshell/factory.go (1)
1-47: LGTM!packages/agent-ops/bff/internal/mapper/agent.go (1)
84-84: LGTM!Also applies to: 212-228
frontend/src/concepts/areas/const.ts (1)
28-28: 🎯 Functional CorrectnessVerify the MaaS redesign flag precedence.
frontend/src/concepts/areas/const.tskeepsmaasSettingsIaRedesignenabled whilebackend/src/utils/constants.tschanges the default dashboard value tofalse. Confirm which source wins when flags are merged; otherwise the UI can expose a redesign that the backend configuration disables.
| new webpack.NormalModuleReplacementPlugin(/^node:/, (resource) => { | ||
| resource.request = resource.request.replace(/^node:/, ''); | ||
| }), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant section with line numbers.
git ls-files 'frontend/config/webpack.common.js'
wc -l frontend/config/webpack.common.js
sed -n '210,280p' frontend/config/webpack.common.js
# Inspect surrounding resolver configuration and any related comments/usages.
rg -n "conditionNames|NormalModuleReplacementPlugin|node:" frontend/config/webpack.common.js frontend -SRepository: opendatahub-io/odh-dashboard
Length of output: 6212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the webpack version and any explicit resolver fallbacks/polyfills.
sed -n '1,220p' package.json
printf '\n---\n'
sed -n '300,340p' frontend/config/webpack.common.js
# Check for any existing node: imports in the repo.
rg -n --glob '!**/node_modules/**' 'from ['\''"]node:|require\(['\''"]node:' frontend src . -SRepository: opendatahub-io/odh-dashboard
Length of output: 6272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find actual node:-prefixed imports in the browser-facing workspaces.
rg -n --glob 'frontend/**/*.{js,jsx,ts,tsx,mjs,cjs}' --glob 'packages/**/*.{js,jsx,ts,tsx,mjs,cjs}' 'node:' frontend packages -S
# Inspect the webpack resolver's handling of builtins/polyfills in this config.
sed -n '312,332p' frontend/config/webpack.common.jsRepository: opendatahub-io/odh-dashboard
Length of output: 14410
🌐 Web query:
webpack 5 node: prefix NormalModuleReplacementPlugin builtin module resolution browser target userland package conflict
💡 Result:
In Webpack 5, the node: prefix (introduced in Node.js v14.13.1/v16) for built-in modules is treated as a specialized URI scheme [1]. When using a target: 'web' (or browser-like) configuration, this often triggers an UnhandledSchemeError because Webpack routes node:-prefixed requests directly to a scheme handler that, until recently, had no default implementation for non-Node.js targets [2][3][4]. Key points regarding conflict and resolution: 1. Handling of 'node:' Imports: Because Webpack's NormalModuleFactory previously routed these requests through resolveForScheme, they often bypassed standard resolve.alias and resolve.fallback configuration pipelines [5][4]. Recent updates (e.g., Pull Request #20733) have improved this by allowing the resolver to check aliases and fallbacks for node:-prefixed modules before falling back to the default scheme resolution [5][4]. 2. Using NormalModuleReplacementPlugin: If you are encountering conflicts with userland packages or need to polyfill built-ins, NormalModuleReplacementPlugin can be used to rewrite the request [6]. When using this plugin, be aware that Webpack 5 requires you to modify both resource.request and resource.createData.request to ensure the replacement is applied consistently across different resolution phases [7]. Additionally, you may need to manually update resource.createData.context to ensure subsequent imports inside the replaced module are resolved correctly [8][9]. Example of polyfilling a node module via the plugin: new webpack.NormalModuleReplacementPlugin(/^node:fs$/, (resource) => { resource.request = 'fs-browserify'; if (resource.createData) { resource.createData.request = 'fs-browserify'; } }); 3. Best Practices: While NormalModuleReplacementPlugin is flexible, resolve.alias or resolve.fallback is often preferred for simple replacements or disabling modules (e.g., setting them to false) [3][4][10]. Ensure your Webpack version is up to date to take advantage of improved node: scheme support, which reduces the need for custom plugin workarounds [5][4]. If a userland package is specifically causing conflicts by providing its own node:* implementation, verify that your alias order correctly prioritizes the desired polyfill or implementation [5].
Citations:
- 1: Add builtin modules with node: prefix webpack/webpack#13311
- 2: New node: scheme from Node 16 not handled webpack/webpack#13290
- 3: resolve.fallback and resolve.alias do not work with
node:prefix webpack/webpack#14166 - 4: fix: apply resolve.alias and resolve.fallback to node: prefixed imports webpack/webpack#20733
- 5: fix: check aliases before node: builtin scheme in resolver webpack/webpack#21359
- 6: New node: scheme from Node 16 not handled webpack/webpack#13290
- 7: Outdated documention for
NormalModuleReplacementPluginwhen using function replacement option (webpack5) webpack/webpack.js.org#5059 - 8: [Breaking change] NormalModuleReplacementPlugin resolves modules using old resource's context in 5.49.0 webpack/webpack#13957
- 9: NormalModuleReplacementPlugin doesn't resolve relative imports of also replaced modules within replaced modules anymore in webpack 5 webpack/webpack#17716
- 10: NormalModuleReplacementPlugin: newResource is not resolved relative to the previous resource webpack/webpack#8105
🌐 Web query:
webpack resolve.conditionNames browser target default conditionNames web target browser export condition
💡 Result:
In Webpack, resolve.conditionNames is used to determine which conditions in a package's package.json exports field should be matched during module resolution [1][2]. Default Behavior The default value for conditionNames is not a static list; it is determined dynamically based on several factors, including the Webpack mode (development or production), the configured target (e.g., web, node, electron), and the type of dependency being resolved (e.g., import, require) [3][4]. Common Conditions While the list is dynamic, Webpack commonly supports the following conditions when resolving exports [2]: - import: Matched when the package is loaded via ESM import or import. - require: Matched when the package is loaded via CommonJS require. - node: Matched when the target is node or electron. - electron: Matched when the target is electron. - browser: Matched when the target is a web environment (e.g., target: 'web'). - worker: Matched when the target is 'webworker'. - development / production: Matched based on the Webpack mode. - webpack: Always included as a Webpack-specific condition. - default: A catch-all condition. Key Points - Priority: The order of conditions inside a package.json exports object takes precedence over the order defined in conditionNames [5]. The general practice is to order entries from most specific to least specific [5]. - Configuration: You can override the defaults by explicitly setting resolve.conditionNames in your Webpack configuration [1][6]. - Target Impact: If your target is set to browser (the default for most web projects), the browser condition is automatically included, ensuring that exports defined under "browser" are prioritized [7][2]. For authoritative details on the current implementation for your specific version, you can consult the Webpack source code [4].
Citations:
- 1: https://docs.webpack.js.org/configuration/resolve
- 2: add exportsFields and conditionNames options webpack/webpack#10953
- 3: https://webpack.js.org/configuration/resolve/
- 4: Default export conditions are not documented webpack/webpack.js.org#7560
- 5: Exports and
conditionNamesmatching webpack/enhanced-resolve#318 - 6: https://docs.webpack.js.org/concepts/module-resolution
- 7: https://webpack.js.org/guides/package-exports/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the top-level webpack target and any built-in handling in the config.
sed -n '1,120p' frontend/config/webpack.common.js
printf '\n---\n'
rg -n "target:|fallback:|alias:|conditionNames|NormalModuleReplacementPlugin" frontend/config/webpack.common.js -n -SRepository: opendatahub-io/odh-dashboard
Length of output: 4747
🌐 Web query:
webpack NormalModuleReplacementPlugin resource.createData.request webpack 5 official docs
💡 Result:
When using the NormalModuleReplacementPlugin in webpack 5 with a function to modify the resource, you may need to update both resource.request and resource.createData.request to ensure the replacement is applied correctly across the plugin's multiple lifecycle phases [1][2]. The NormalModuleReplacementPlugin operates during both the beforeResolve and afterResolve hooks [1][3]. While modifying resource.request is sufficient in some contexts, internal structures in webpack 5 can lead to situations where this modification is not persisted or correctly utilized if the resolution process has already progressed [1]. To ensure robust behavior that works reliably across these phases, it is a recommended best practice to update both properties when using the function callback approach [1]: new webpack.NormalModuleReplacementPlugin(/-APP_TARGET$/, (resource) => { // Update the primary request resource.request = resource.request.replace(/-APP_TARGET/, '-new-value'); // Ensure compatibility by updating createData if it exists if (resource.createData) { resource.createData.request = resource.request; } }); This pattern addresses known issues where documentation might only show updating resource.request, which may cause inconsistencies in webpack 5 depending on exactly when the plugin executes in the dependency graph resolution [1].
Citations:
- 1: Outdated documention for
NormalModuleReplacementPluginwhen using function replacement option (webpack5) webpack/webpack.js.org#5059 - 2: https://docs.w3cub.com/webpack~5/plugins/normal-module-replacement-plugin
- 3: https://docs.webpack.js.org/plugins/normal-module-replacement-plugin
Mirror the rewritten request into resource.createData.request. resource.request alone can be ignored in later webpack 5 resolution phases, so some node: imports may not be replaced consistently.
🤖 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 `@frontend/config/webpack.common.js` around lines 243 - 245, Update the
NormalModuleReplacementPlugin callback to apply the node: prefix removal to both
resource.request and resource.createData.request, preserving the existing
replacement behavior for each request value.
Source: Path instructions
| conditionNames: ['import', 'module', 'require', 'default'], | ||
| fallback: { | ||
| diagnostics_channel: false, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore Webpack’s default export conditions.
resolve.conditionNames replaces the defaults here, so webpack/browser are no longer considered during conditional export resolution. That can select a non-browser entrypoint and break the client bundle. Keep the defaults with '...' or remove this override.
🤖 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 `@frontend/config/webpack.common.js` around lines 319 - 322, Update the
resolve.conditionNames configuration near the fallback setting to preserve
Webpack’s default export conditions, including webpack and browser. Add the
spread defaults marker alongside the existing conditions or remove the override
entirely, while keeping the fallback diagnostics_channel configuration
unchanged.
| flag.StringVar(&cfg.OpenShellGatewayURL, "openshell-gateway-url", | ||
| os.Getenv("OPENSHELL_GATEWAY_URL"), | ||
| "gRPC address of the OpenShell Gateway (e.g. localhost:8090). When set, uses OpenShell SDK for agent operations.") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target files and nearby symbols first.
ast-grep outline packages/agent-ops/bff/cmd/main.go --view expanded
printf '\n---\n'
ast-grep outline packages/agent-ops/bff/internal/api/app.go --view expanded
printf '\n--- main.go excerpt ---\n'
sed -n '1,140p' packages/agent-ops/bff/cmd/main.go
printf '\n--- app.go excerpt ---\n'
sed -n '1,220p' packages/agent-ops/bff/internal/api/app.goRepository: opendatahub-io/odh-dashboard
Length of output: 16411
Do not activate an insecure OpenShell transport (CWE-295).
packages/agent-ops/bff/cmd/main.go:56-58 enables packages/agent-ops/bff/internal/api/app.go to build the OpenShell client with TLSConfig{Insecure: true}, which lets a network attacker MITM privileged agent deploy/stop/chat traffic. Require verified TLS and a CA bundle instead of disabling certificate checks.
🤖 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 `@packages/agent-ops/bff/cmd/main.go` around lines 56 - 58, Update the
OpenShell gateway configuration used by the openshell-gateway-url flag and the
client construction in app.go to require verified TLS with a configured CA
bundle. Remove or disable the TLSConfig{Insecure: true} behavior, and ensure
privileged agent operations cannot proceed unless certificate verification is
enabled.
Source: Path instructions
| - `POST /agents/:ns/:name/restart` is implemented as stop + start (paused -> running). A true | ||
| restart (delete pod, let controller recreate) requires looking up the Pod owned by the Sandbox | ||
| CR and deleting it. This needs a separate story once the Sandbox controller's pod ownership | ||
| model is confirmed. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reconcile the documented endpoints with actual route registrations.
The supplied packages/agent-ops/bff/internal/api/app.go route table shows delete, stop, start, and chat routes, but no /restart or /card route. The supplied OpenShell client instead enriches agent details with card data during GetAgent. Verify these routes before updating OpenAPI; otherwise the spec may document nonexistent endpoints while missing the actual response-schema change.
Also applies to: 42-45
🤖 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 `@packages/agent-ops/bff/GAPS.md` around lines 15 - 18, Reconcile the endpoint
documentation in GAPS.md with the route registrations in app.go: remove or
revise the undocumented /restart and /card endpoint entries unless they are
actually registered, and document the existing GetAgent response enrichment with
card data instead. Verify the delete, stop, start, and chat routes remain
accurately represented before updating OpenAPI.
| - The code falls back to constructing the service URL from the agent name when no Service object | ||
| is found. The Sandbox CR `status.serviceFQDN` field is read but only used as a fallback in | ||
| summary listing. Once the controller reliably populates this, we should prefer it over manual | ||
| Service lookups. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File list ==\n'
git ls-files 'packages/agent-ops/bff/GAPS.md' 'packages/agent-ops/bff/internal/integrations/agents/openshell/client.go' 'packages/agent-ops/bff/internal/integrations/agents/openshell/*' | sed 's#^`#-` #'
printf '\n== Outline: client.go ==\n'
ast-grep outline packages/agent-ops/bff/internal/integrations/agents/openshell/client.go --view expanded || true
printf '\n== serviceFQDN search ==\n'
rg -n --hidden --glob '!**/node_modules/**' 'serviceFQDN|serviceFqdn|FQDN' packages/agent-ops/bff || true
printf '\n== Relevant lines from GAPS.md ==\n'
cat -n packages/agent-ops/bff/GAPS.md | sed -n '1,120p'
printf '\n== Relevant lines from client.go ==\n'
cat -n packages/agent-ops/bff/internal/integrations/agents/openshell/client.go | sed -n '1,260p'Repository: opendatahub-io/odh-dashboard
Length of output: 15245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'packages/agent-ops/bff/GAPS.md' 'packages/agent-ops/bff/internal/integrations/agents/openshell/client.go' 'packages/agent-ops/bff/internal/integrations/agents/openshell/*'
ast-grep outline packages/agent-ops/bff/internal/integrations/agents/openshell/client.go --view expanded || true
rg -n --hidden --glob '!**/node_modules/**' 'serviceFQDN|serviceFqdn|FQDN' packages/agent-ops/bff || true
cat -n packages/agent-ops/bff/GAPS.md | sed -n '1,120p'
cat -n packages/agent-ops/bff/internal/integrations/agents/openshell/client.go | sed -n '1,260p'Repository: opendatahub-io/odh-dashboard
Length of output: 15098
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "README? no"Repository: opendatahub-io/odh-dashboard
Length of output: 177
Use status.serviceFQDN for card probes.
GetAgent still builds <sandbox-name>.<namespace>.svc.cluster.local and probes that address, so a controller-advertised FQDN is ignored on the detail path. If the service name or cluster domain differs, the card probe will miss the agent; read status.serviceFQDN first and fall back to the constructed host only when it is empty.
🤖 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 `@packages/agent-ops/bff/GAPS.md` around lines 28 - 31, Update the GetAgent
card-probe host selection to use the Sandbox CR status.serviceFQDN value first,
falling back to the constructed service host only when that field is empty;
preserve the existing probe behavior otherwise.
| } | ||
|
|
||
| if cfg.AuthMethod == config.AuthMethodDisabled && !cfg.MockAgentClient { | ||
| if cfg.AuthMethod == config.AuthMethodDisabled && !cfg.MockAgentClient && cfg.OpenShellGatewayURL == "" { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/agent-ops/bff/internal/integrations/agents/openshell/factory.go --items all
rg -n -C 4 '\bk8sFactory\b' packages/agent-ops/bff/internal/integrations/agents/openshell -g '*.go'Repository: opendatahub-io/odh-dashboard
Length of output: 3710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/agent-ops/bff/internal/api/app.go --items all
sed -n '110,230p' packages/agent-ops/bff/internal/api/app.go
echo '---'
sed -n '1,120p' packages/agent-ops/bff/internal/integrations/agents/openshell/factory.goRepository: opendatahub-io/odh-dashboard
Length of output: 8698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/agent-ops/bff/internal/integrations/agents/openshell --items all
rg -n -C 3 'k8sClient|operatingMode|stop|start|Patch|Sandbox' packages/agent-ops/bff/internal/integrations/agents/openshell -g '*.go'Repository: opendatahub-io/odh-dashboard
Length of output: 15406
Reject real OpenShell when auth is disabled.
Require MOCK_AGENT_CLIENT=true on this branch; otherwise AUTH_METHOD=disabled still builds an OpenShell client with NoAuth() + TLS.Insecure=true and leaves k8sFactory nil, exposing agent routes without auth and breaking stop/start (CWE-306, CWE-295).
🤖 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 `@packages/agent-ops/bff/internal/api/app.go` at line 127, Update the
configuration branch around cfg.AuthMethod, cfg.MockAgentClient, and
cfg.OpenShellGatewayURL to reject real OpenShell when authentication is
disabled: require cfg.MockAgentClient to be true, or otherwise fail
configuration before constructing the unauthenticated client. Preserve the
existing behavior for mock-agent mode and configured gateway URLs.
| Resource: "sandboxes", | ||
| } | ||
|
|
||
| var cardProbeClient = &http.Client{Timeout: 3 * time.Second} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Block agent-controlled redirects in the card probe (CWE-918).
A deployed agent can return 302 Location: http://169.254.169.254/...; http.Client.Get follows it with BFF network privileges. Disable redirects and validate SDK-provided sandbox and namespace values as DNS-1123 labels before constructing the target URL.
Remediation
-var cardProbeClient = &http.Client{Timeout: 3 * time.Second}
+var cardProbeClient = &http.Client{
+ Timeout: 3 * time.Second,
+ CheckRedirect: func(_ *http.Request, _ []*http.Request) error {
+ return http.ErrUseLastResponse
+ },
+}Also applies to: 75-77, 207-210
🤖 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 `@packages/agent-ops/bff/internal/integrations/agents/openshell/client.go` at
line 26, Harden cardProbeClient against SSRF by disabling automatic redirect
following, so agent-controlled Location responses cannot reach internal
endpoints. Validate the SDK-provided sandbox and namespace values as DNS-1123
labels before constructing the card-probe target URL, rejecting invalid values.
Apply these changes to every card-probe request path, including the usages near
the noted locations.
| type OpenShellStatus struct { | ||
| Enabled bool `json:"enabled"` | ||
| Gateway string `json:"gateway,omitempty"` | ||
| Namespace string `json:"namespace,omitempty"` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File list ==\n'
git ls-files 'packages/agent-ops/bff/internal/models/health_check.go' \
'packages/agent-ops/bff' | sed -n '1,200p'
printf '\n== Outline: health_check.go ==\n'
ast-grep outline packages/agent-ops/bff/internal/models/health_check.go --view expanded || true
printf '\n== Relevant references ==\n'
rg -n --hidden --no-messages -S 'OpenShellStatus|gateway|namespace|health' packages/agent-ops/bff | sed -n '1,220p'Repository: opendatahub-io/odh-dashboard
Length of output: 40162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== healthcheck handler ==\n'
cat -n packages/agent-ops/bff/internal/api/healthcheck_handler.go | sed -n '1,120p'
printf '\n== route wiring / middleware around healthcheck ==\n'
rg -n --hidden --no-messages -S 'healthcheck|/healthcheck|healthCheck|HealthCheck' packages/agent-ops/bff/internal/api packages/agent-ops/bff/cmd packages/agent-ops/bff/README.md | sed -n '1,220p'
printf '\n== openapi references for healthcheck/openShell ==\n'
rg -n --hidden --no-messages -S 'openshell|healthcheck|OpenShellStatus|HealthCheckModel' packages/agent-ops/bff/openapi packages/agent-ops/bff/internal | sed -n '1,220p'Repository: opendatahub-io/odh-dashboard
Length of output: 6395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '285,340p' packages/agent-ops/bff/openapi/src/agent-ops.yamlRepository: opendatahub-io/odh-dashboard
Length of output: 1482
Remove OpenShell gateway/namespace from public /healthcheck (CWE-200)
/healthcheck is unauthenticated, but this response leaks OpenShellGatewayURL and OpenShellSandboxNamespace, exposing internal cluster topology to any caller. Keep only enabled here; move any diagnostic details behind an admin-only endpoint.
Remediation
type OpenShellStatus struct {
- Enabled bool `json:"enabled"`
- Gateway string `json:"gateway,omitempty"`
- Namespace string `json:"namespace,omitempty"`
+ Enabled bool `json:"enabled"`
}📝 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.
| type OpenShellStatus struct { | |
| Enabled bool `json:"enabled"` | |
| Gateway string `json:"gateway,omitempty"` | |
| Namespace string `json:"namespace,omitempty"` | |
| type OpenShellStatus struct { | |
| Enabled bool `json:"enabled"` | |
| } |
🤖 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 `@packages/agent-ops/bff/internal/models/health_check.go` around lines 7 - 10,
Update OpenShellStatus to expose only the enabled field in the unauthenticated
health-check response, removing Gateway and Namespace plus their JSON fields.
Keep any diagnostic gateway or namespace data available only through an existing
admin-only endpoint, without exposing it from /healthcheck.
| SystemInfo SystemInfo `json:"system_info"` | ||
| Status string `json:"status"` | ||
| SystemInfo SystemInfo `json:"system_info"` | ||
| OpenShell OpenShellStatus `json:"openshell"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 '\bopenshell\b|\bpodName\b' packages/agent-ops/bff \
-g '*.yaml' -g '*.yml' -g '*.json'Repository: opendatahub-io/odh-dashboard
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== repo files mentioning openshell/podName =='
rg -n -S '\bopenshell\b|\bpodName\b' . || true
echo
echo '== likely OpenAPI/spec files in packages/agent-ops/bff =='
fd -a 'openapi*|swagger*|api.*(yaml|yml|json)|docs' packages/agent-ops/bff 2>/dev/null || true
echo
echo '== model file locations =='
git ls-files 'packages/agent-ops/bff/internal/models/*.go' | sed -n '1,200p'Repository: opendatahub-io/odh-dashboard
Length of output: 24849
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== openapi tree =='
find packages/agent-ops/bff/openapi -maxdepth 3 -type f | sort
echo
echo '== search in openapi/docs for fields =='
rg -n -S '\bopenshell\b|\bpodName\b' packages/agent-ops/bff/openapi packages/agent-ops/bff/docs packages/agent-ops/bff/internal/api/openapi_handler.go || true
echo
echo '== openapi handler outline =='
ast-grep outline packages/agent-ops/bff/internal/api/openapi_handler.go --view expandedRepository: opendatahub-io/odh-dashboard
Length of output: 1589
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== outline agent-ops.yaml =='
ast-grep outline packages/agent-ops/bff/openapi/src/agent-ops.yaml --view expanded || true
echo
echo '== field and schema search in agent-ops.yaml =='
rg -n -C 3 -S 'health|runtime|openshell|podName|pod_name|pod-name' packages/agent-ops/bff/openapi/src/agent-ops.yaml || true
echo
echo '== generated-type or spec consumer search =='
rg -n -S 'agent-ops\.yaml|openapi\.json|openapi\.yaml|openapi/src/agent-ops' packages/agent-ops/bff packages/agent-ops/frontend frontend packages/agent-ops || trueRepository: opendatahub-io/odh-dashboard
Length of output: 12935
Add openshell to the health response schema and optional podName to the runtime-detail schema in packages/agent-ops/api/openapi/agent-ops.yaml, then sync packages/agent-ops/bff/openapi/src/agent-ops.yaml so it matches packages/agent-ops/bff/internal/models/health_check.go and packages/agent-ops/bff/internal/models/agent_runtime_detail.go.
📍 Affects 2 files
packages/agent-ops/bff/internal/models/health_check.go#L16-L16(this comment)packages/agent-ops/bff/internal/models/agent_runtime_detail.go#L30-L30
🤖 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 `@packages/agent-ops/bff/internal/models/health_check.go` at line 16, Add
openshell to the health response schema and optional podName to the
runtime-detail schema in packages/agent-ops/api/openapi/agent-ops.yaml, then
synchronize packages/agent-ops/bff/openapi/src/agent-ops.yaml with the OpenAPI
definitions and the OpenShellStatus, health-check, and agent runtime-detail
models in packages/agent-ops/bff/internal/models/health_check.go and
packages/agent-ops/bff/internal/models/agent_runtime_detail.go.
Source: Path instructions
| if (!res.ok) { | ||
| const body = await res.text(); | ||
| throw new Error(`${res.status}: ${body}`); | ||
| } | ||
| const json = await res.json(); | ||
| setAgents(json.data?.runtimes ?? []); | ||
| } catch (e) { | ||
| setError(e instanceof Error ? e.message : String(e)); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file around the reported lines and render path
file="packages/agent-ops/src/AgentDeploymentsPage.tsx"
wc -l "$file"
sed -n '1,220p' "$file" | nl -ba | sed -n '1,220p'
# Search for setError usage and any sanitization helpers in the file
rg -n "setError|error|res\.text\(|dangerouslySetInnerHTML|sanitize|innerHTML" "$file"Repository: opendatahub-io/odh-dashboard
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/agent-ops/src/AgentDeploymentsPage.tsx"
# Show the whole file with line numbers using awk (nl unavailable)
awk '{printf("%4d %s\n", NR, $0)}' "$file" | sed -n '1,220p'
# Focus on the error handling/render lines if present
awk '{printf("%4d %s\n", NR, $0)}' "$file" | sed -n '50,110p'
# Search for the state and render usage in the file
rg -n "setError|error|res\.text\(|setAgents|setLoading|dangerouslySetInnerHTML|sanitize|innerHTML" "$file"Repository: opendatahub-io/odh-dashboard
Length of output: 7179
Stop rendering upstream error bodies in the UI. res.text() is concatenated into the thrown message and shown in the <Alert>, so non-2xx responses can leak pod names, IPs, or stack traces to authenticated users (CWE-209). Replace this with a fixed generic message and keep the raw body server-side/logged.
🤖 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 `@packages/agent-ops/src/AgentDeploymentsPage.tsx` around lines 60 - 67, Update
the fetch error handling in AgentDeploymentsPage around the !res.ok branch to
stop including res.text() in the Error message displayed through setError and
the Alert. Throw a fixed generic failure message for non-2xx responses, while
preserving server-side logging of the raw response body if an existing logging
path is available.
Source: Path instructions
|
/early-gate-build |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
2 participants
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes