Conversation
This comment has been minimized.
This comment has been minimized.
|
Claude Code Review SummaryThis PR introduces scheduled sessions backed by Kubernetes CronJobs, spanning backend CRUD handlers, an operator Issues by SeverityBlocker IssuesNone. Critical Issues1. Mutating handlers bypass resource-specific RBAC checks
The handlers verify token validity and rely on This means any user who can ssar := &authv1.SelfSubjectAccessReview{
Spec: authv1.SelfSubjectAccessReviewSpec{
ResourceAttributes: &authv1.ResourceAttributes{
Group: "vteam.ambient-code",
Resource: "agenticsessions",
Verb: "create",
Namespace: projectName,
},
},
}
res, err := reqK8s.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, ssar, v1.CreateOptions{})
if err != nil || !res.Status.Allowed {
c.JSON(http.StatusForbidden, gin.H{"error": "Unauthorized to manage scheduled sessions"})
return
}Major Issues1. Type mismatch: TriggerScheduledSession backend response vs. frontend TypeScript type Backend ( Either align the backend to include a 2. Internal parse errors exposed to API clients
// Current - leaks binding internals to the caller
c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("Invalid request body: %v", err)})
// Correct pattern per error-handling.md
log.Printf("Failed to bind scheduled session request in project %s: %v", project, err)
c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid request body"})3.
if err := ensureSessionTriggerRBAC(namespace); err != nil {
log.Printf("Error ensuring session trigger RBAC in namespace %s: %v", namespace, err)
// error discarded — reconciliation continues
}Per 4.
Move to Minor Issues1. RBAC resources in
The 2.
Positive Highlights
Recommendations
Generated with Claude Code using |
mprpic
force-pushed
the
scheduled-sessions
branch
from
4030cb1 to
2b65a85
Compare
Claude Code ReviewSummaryPR 864 introduces a well-structured scheduled sessions feature spanning all platform layers: backend CronJob CRUD handlers, an operator Issues by SeverityBlocker IssuesNone. Critical Issues1. Missing resource-level RBAC check on all write handlers Files: Problem: Every handler verifies the user has a valid token ( Standard violated: Suggested fix — add a ssar := &authv1.SelfSubjectAccessReview{
Spec: authv1.SelfSubjectAccessReviewSpec{
ResourceAttributes: &authv1.ResourceAttributes{
Group: "batch",
Resource: "cronjobs",
Verb: "create",
Namespace: project,
},
},
}
res, err := k8s.AuthorizationV1().SelfSubjectAccessReviews().Create(c.Request.Context(), ssar, metav1.CreateOptions{})
if err != nil || !res.Status.Allowed {
c.JSON(http.StatusForbidden, gin.H{"error": "You do not have permission to create scheduled sessions in this project"})
return
}2. Files: Problem: The backend struct has Fix (pick one): Remove Major Issues3. Trigger API response type mismatch File: Problem: Fix: export async function triggerScheduledSession(
projectName: string,
name: string
): Promise<{ name: string; namespace: string }> { ... }4. Gin binding error details leaked to API response File: Problem: Standard violated: Fix: log.Printf("Invalid request body for scheduled session in project %s: %v", project, err)
c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid request body"})5. Native File: Problem: Standard violated: Fix: Replace with a Shadcn 6. Missing initial loading and error states in File: Problem: Standard violated: Fix: const { data: scheduledSessions, isFetching, isLoading, error, refetch } = useScheduledSessions(projectName);
if (isLoading) return <div className="flex justify-center p-8"><Loader2 className="animate-spin" /></div>;
if (error) return <div className="text-destructive p-4">Failed to load scheduled sessions</div>;Minor Issues7. Unvalidated URL parameter used in a Kubernetes label selector File: Problem: Fix: Validate 8. File: Problem: Fix: ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
_, err = dynamicClient.Resource(gvr).Namespace(projectNamespace).Create(ctx, session, metav1.CreateOptions{})9. Redundant Files: Problem: Fix: Remove 10. File: Problem: This dialog is only used by the scheduled sessions feature. Standard violated: Fix: Move to Positive Highlights
Recommendations (priority order)
|
- Add SelfSubjectAccessReview RBAC checks to all scheduled session handlers (list, get, create, update, delete, suspend/resume, trigger) - Make displayName optional in CreateScheduledSessionRequest - Fix trigger API response type mismatch (name/namespace vs message) - Stop leaking Gin binding errors to API responses - Propagate ensureSessionTriggerRBAC errors into ProjectSettings status - Add OwnerReferences to SA/Role/RoleBinding created by trigger RBAC - Add loading and error states to SchedulesSection component - Fix sanitizeName trailing-hyphen bug (truncate before trim) - Move create-scheduled-session-dialog to colocated location Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Prpič <mprpic@redhat.com>
WalkthroughThis PR introduces a complete "scheduled sessions" feature enabling users to create, manage, and trigger recurring session jobs using Kubernetes CronJobs. It includes backend handlers for CRUD operations, frontend UI components for management, API routes, React Query hooks, RBAC provisioning, and operator trigger functionality. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend
participant FrontendAPI as Frontend<br/>API Routes
participant BackendAPI as Backend<br/>Handlers
participant K8s as Kubernetes<br/>API
participant Operator as Operator<br/>Trigger
User->>Frontend: Create scheduled session
Frontend->>Frontend: Validate cron expression
Frontend->>FrontendAPI: POST /scheduled-sessions
FrontendAPI->>BackendAPI: POST create (forwarded)
BackendAPI->>K8s: Create CronJob<br/>(with SESSION_TEMPLATE env)
K8s-->>BackendAPI: CronJob created
BackendAPI->>K8s: Convert to ScheduledSession
BackendAPI-->>FrontendAPI: Return ScheduledSession
FrontendAPI-->>Frontend: Response
Frontend-->>User: Show created session
User->>Frontend: Trigger now
Frontend->>FrontendAPI: POST /trigger
FrontendAPI->>BackendAPI: POST trigger (forwarded)
BackendAPI->>K8s: Create Job from CronJob<br/>JobTemplate
K8s-->>BackendAPI: Job created
BackendAPI->>K8s: Create AgenticSession<br/>(from template)
K8s->>Operator: CronJob controller<br/>sees new Job
Operator->>Operator: session-trigger<br/>reads env vars
Operator->>K8s: Create AgenticSession<br/>with displayName
K8s-->>BackendAPI: AgenticSession created
BackendAPI-->>FrontendAPI: Return job details
FrontendAPI-->>Frontend: Response
Frontend-->>User: Show triggered run
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
- Add SelfSubjectAccessReview RBAC checks to all scheduled session handlers (list, get, create, update, delete, suspend/resume, trigger) - Make displayName optional in CreateScheduledSessionRequest - Fix trigger API response type mismatch (name/namespace vs message) - Stop leaking Gin binding errors to API responses - Propagate ensureSessionTriggerRBAC errors into ProjectSettings status - Add OwnerReferences to SA/Role/RoleBinding created by trigger RBAC - Add loading and error states to SchedulesSection component - Fix sanitizeName trailing-hyphen bug (truncate before trim) - Move create-scheduled-session-dialog to colocated location Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Prpič <mprpic@redhat.com>
mprpic
force-pushed
the
scheduled-sessions
branch
from
da257a4 to
7b9f763
Compare
Claude Code ReviewSummaryThis PR introduces a well-structured scheduled sessions feature spanning all platform layers (backend CronJob CRUD, operator RBAC provisioning, frontend React Query + UI). The overall architecture is sound: user-token RBAC checks are correctly gated before service-account CronJob operations, and the frontend follows React Query and Shadcn patterns. A few issues need attention before merge—the most important are error-wrapping violations in the operator, RBAC role drift in the new reconciler, and native Issues by SeverityBlocker IssuesNone. Critical IssuesNone. Major Issues1. Error wrapping uses
2. RBAC role drift — existing Role is never updated
3.
4.
Minor Issues5. Silent error swallowing in
6.
7. Hardcoded
8.
Positive Highlights
Recommendations
🤖 Generated with Claude Code 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Claude Code ReviewSummaryThis PR introduces a well-architected scheduled sessions feature spanning all platform layers — backend CronJob CRUD, operator trigger subcommand with per-namespace RBAC provisioning, and a complete frontend with React Query hooks, Shadcn UI, and thorough unit tests. The security model is sound: user token auth gates every operation via RBAC proxy on Blocker IssuesNone Critical IssuesNone Major IssuesNone Minor Issues1. All other handlers (and the reqK8s, reqDyn := GetK8sClientsForRequest(c)
if reqK8s == nil {
c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid or missing token"})
c.Abort()
return
}2. Every other handler in this file calls 3. Missing log statement before marshal error in Per log.Printf("Failed to marshal session template for update %s in project %s: %v", name, project, err)4. The table row Link uses bare 5. Delete confirmation uses Both 6. All three K8s calls in this function use 7. The 8. The trigger binary only reads env vars and makes a K8s API call — it writes nothing to disk. Setting Positive Highlights
Recommendations (priority order)
Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/frontend/src/components/jira-connection-card.tsx (1)
40-49:⚠️ Potential issue | 🟠 MajorAdding
urlandusernameto the dependency array prevents users from clearing these fields.With this change, if a user clears the URL or username field entirely (e.g., to paste a different value), the effect immediately re-runs and repopulates the default. The original dependencies
[showForm, currentUser?.email]correctly limited this "initial population" behavior to when the form is first shown.Proposed fix: revert to original dependencies
}, [showForm, currentUser?.email, url, username]) + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [showForm, currentUser?.email])Alternatively, use a ref to track whether initial population has occurred:
const didPopulateRef = useRef(false) useEffect(() => { if (showForm && !didPopulateRef.current) { didPopulateRef.current = true if (!url) setUrl(DEFAULT_JIRA_URL) if (!username && currentUser?.email) setUsername(currentUser.email) } if (!showForm) { didPopulateRef.current = false } }, [showForm, currentUser?.email, url, username])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/components/jira-connection-card.tsx` around lines 40 - 49, The effect currently reruns when `url` or `username` change, causing cleared fields to be immediately repopulated; revert the dependency array on the `useEffect` that sets `DEFAULT_JIRA_URL` and pre-fills `username` back to only `[showForm, currentUser?.email]` so the initial population runs only when the form is shown, or alternatively implement a `didPopulateRef` boolean to ensure `useEffect` (the same block using `setUrl`, `setUsername`, `DEFAULT_JIRA_URL`, and `currentUser?.email`) only populates fields once per show and resets the ref when `showForm` becomes false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/handlers/scheduled_sessions.go`:
- Around line 239-292: The Get->modify->Update sequence on the CronJob
(K8sClientScheduled.BatchV1().CronJobs(...).Get and Update) can race with
concurrent writers; implement retry-on-conflict: wrap the read-modify-update
logic in a small retry loop (e.g., 3 attempts), on errors.IsConflict(err)
re-fetch the CronJob, reapply the same modifications (schedule, suspend,
annotations, SESSION_TEMPLATE env update) and call Update again, aborting on
non-conflict errors and returning the same HTTP errors/logs; preserve use of
c.Request.Context(), keep annotationDisplayName and cronJobToScheduledSession
handling intact, and log conflicts when retries are exhausted.
- Around line 266-282: The current update loop in scheduled_sessions.go only
replaces SESSION_TEMPLATE if that env var already exists in the "trigger"
container, so when it's absent the change is silently skipped; modify the logic
in the handler that processes cj.Spec.JobTemplate.Spec.Template.Spec.Containers
(the block that finds container.Name == "trigger") to track whether an env var
named "SESSION_TEMPLATE" was found and updated, and if not append a new
corev1.EnvVar (or appropriate Env type used in this file) with Name
"SESSION_TEMPLATE" and Value set to the marshaled templateJSON; ensure you
perform this add within the same container handling (e.g., after the inner loop)
so the env var is created when missing.
- Around line 415-441: Add an explicit RBAC check using the existing
checkScheduledSessionAccess helper at the start of ListScheduledSessionRuns
(before calling k8sDyn.Resource(...).List). Call checkScheduledSessionAccess(c,
project, name, "list") (or the module's actual checkScheduledSessionAccess
signature) and if it denies access return the same HTTP error handling used by
other handlers (abort with 403/unauthorized and JSON error) so the pattern
matches ListScheduledSessions/CreateScheduledSession/GetScheduledSession/etc.;
ensure this check runs before the k8s list call and references
labelScheduledSessionName/name as in the current function.
In
`@components/frontend/src/app/api/projects/`[name]/scheduled-sessions/[scheduledSessionName]/suspend/route.ts:
- Around line 15-16: The current code always calls response.text() and
reconstructs a Response, which breaks for null-body statuses (204/205/304);
update the suspend route handler to first check response.status (or use a helper
like isNoContentStatus) and if it is 204, 205, or 304 return new Response(null,
{ status: response.status, headers: { ... } }) (matching the DELETE handler
pattern), otherwise await response.text() and return the reconstructed Response
with the JSON Content-Type; reference the existing response variable, the const
text assignment, and the Response constructor when making the change.
In `@components/frontend/src/app/api/projects/`[name]/scheduled-sessions/route.ts:
- Around line 5-41: Both GET and POST duplicate proxy logic (header forwarding,
fetch to BACKEND_URL, reading text, reconstructing Response, and error
handling); extract this into a single helper (e.g., forwardProxy or
proxyScheduledSessionRequest) that accepts the incoming Request, resolved params
(await params), the target path suffix ("/scheduled-sessions"), and HTTP method,
then: build headers with buildForwardHeadersAsync(request), perform fetch to
`${BACKEND_URL}/projects/${encodeURIComponent(name)}${pathSuffix}` with the
method, pass through body when present, read response.text(), and return a new
Response preserving response.status and Content-Type: application/json; on
exceptions return Response.json({ error: ... }, { status: 500 }) and log the
error. Replace GET and POST to call this helper; reuse the same helper in the
runs, suspend, and trigger route files to remove duplication.
In
`@components/frontend/src/components/workspace-sections/create-scheduled-session-dialog.tsx`:
- Around line 169-172: The trigger wrapper currently uses a plain <div> with an
onClick (the element that calls setOpen(true) and renders trigger) which is not
keyboard-accessible; replace that clickable div with the DialogTrigger component
from your Dialog primitives (import DialogTrigger) so keyboard and ARIA
behaviors are handled, and remove the redundant fragment/closing fragment around
the Dialog; ensure Dialog still receives open and onOpenChange
(handleOpenChange) and that you pass the original trigger as DialogTrigger's
child.
In `@components/frontend/src/lib/__tests__/cron.test.ts`:
- Around line 21-29: The test is flaky because it calls Date.now() during
assertions instead of using a captured timestamp; before invoking getNextRuns('0
* * * *', 3) capture a const now = Date.now() and then use that captured now in
the assertions (replace Date.now() checks with now) to ensure comparisons
against a stable reference; update the test around the getNextRuns call and the
loop that checks date.getTime() and the ordering assertion to reference now and
keep the same checks that each entry is a Date and increasing.
In `@components/frontend/src/services/api/scheduled-sessions.ts`:
- Around line 14-21: All functions interpolate raw path parameters causing
malformed URLs; update each function (listScheduledSessions,
createScheduledSession, getScheduledSession, updateScheduledSession,
deleteScheduledSession, suspendScheduledSession, resumeScheduledSession,
triggerScheduledSession, listScheduledSessionRuns) to URL-encode path segments
by replacing direct uses of projectName and name in template strings with
encodeURIComponent(projectName) and encodeURIComponent(name) (or local consts
like encodedProject = encodeURIComponent(projectName), encodedName =
encodeURIComponent(name)) so the API paths become
`/projects/${encodedProject}/...` and
`/projects/${encodedProject}/scheduled-sessions/${encodedName}/...` consistently
across the file.
In
`@components/frontend/src/services/queries/__tests__/use-scheduled-sessions.test.ts`:
- Line 83: The mock for triggerScheduledSession in
use-scheduled-sessions.test.ts returns { message: 'triggered' } but the real API
returns an object shaped { name: string; namespace: string }; change the
vi.fn().mockResolvedValue to return a realistic object (e.g., { name: '...',
namespace: '...' }) and update the test assertion that currently checks the
message field to assert the returned name/namespace values (or their usages) so
the test matches the actual API contract for triggerScheduledSession.
In `@components/frontend/src/services/queries/use-scheduled-sessions.ts`:
- Around line 166-184: The onSuccess handler in useTriggerScheduledSession only
invalidates the runs list but not the scheduled session detail, so update the
onSuccess (in useTriggerScheduledSession) to also invalidate the detail query
(e.g., call queryClient.invalidateQueries with
scheduledSessionKeys.detail(projectName, name) or include that key alongside
scheduledSessionKeys.runs) so the UI fetches the updated
CronJob/LastScheduleTime state after a manual trigger.
In `@components/operator/internal/handlers/projectsettings.go`:
- Around line 145-162: The current flow sets triggerRBACReady=false when
ensureSessionTriggerRBAC(namespace, obj) fails but returns immediately before
persisting that into the CR status; change the control flow so that regardless
of the ensureSessionTriggerRBAC error you call
updateProjectSettingsStatus(namespace, name, statusUpdate) to persist
"scheduledSessionRBACReady": false (use the existing statusUpdate map) and then
return the original error (or wrap it) from the handler; modify the block around
ensureSessionTriggerRBAC, triggerRBACReady, statusUpdate and the return path so
updateProjectSettingsStatus is invoked on error before returning.
In `@components/operator/internal/trigger/trigger.go`:
- Around line 75-80: The Create call uses context.TODO() and can hang; replace
it with a cancellable context with a timeout (e.g., context.WithTimeout) before
calling dynamicClient.Resource(gvr).Namespace(projectNamespace).Create, use that
ctx in place of context.TODO(), defer the cancel() immediately after creation of
the ctx, and keep the existing error handling (log.Fatalf) unchanged; ensure the
surrounding function (e.g., where gvr and dynamicClient are used) imports and
uses the context package and choose a sensible timeout like 30s.
---
Outside diff comments:
In `@components/frontend/src/components/jira-connection-card.tsx`:
- Around line 40-49: The effect currently reruns when `url` or `username`
change, causing cleared fields to be immediately repopulated; revert the
dependency array on the `useEffect` that sets `DEFAULT_JIRA_URL` and pre-fills
`username` back to only `[showForm, currentUser?.email]` so the initial
population runs only when the form is shown, or alternatively implement a
`didPopulateRef` boolean to ensure `useEffect` (the same block using `setUrl`,
`setUsername`, `DEFAULT_JIRA_URL`, and `currentUser?.email`) only populates
fields once per show and resets the ref when `showForm` becomes false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 190159fb-25ff-47d3-921b-0b7e67198f7a
⛔ Files ignored due to path filters (1)
components/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (48)
Makefilecomponents/ambient-sdk/generator/parser.gocomponents/ambient-sdk/go-sdk/client/client.gocomponents/backend/handlers/scheduled_sessions.gocomponents/backend/handlers/scheduled_sessions_test.gocomponents/backend/handlers/sessions_test.gocomponents/backend/handlers/test_helpers_test.gocomponents/backend/main.gocomponents/backend/routes.gocomponents/backend/server/k8s.gocomponents/backend/types/scheduled_session.gocomponents/frontend/package.jsoncomponents/frontend/src/app/api/projects/[name]/scheduled-sessions/[scheduledSessionName]/resume/route.tscomponents/frontend/src/app/api/projects/[name]/scheduled-sessions/[scheduledSessionName]/route.tscomponents/frontend/src/app/api/projects/[name]/scheduled-sessions/[scheduledSessionName]/runs/route.tscomponents/frontend/src/app/api/projects/[name]/scheduled-sessions/[scheduledSessionName]/suspend/route.tscomponents/frontend/src/app/api/projects/[name]/scheduled-sessions/[scheduledSessionName]/trigger/route.tscomponents/frontend/src/app/api/projects/[name]/scheduled-sessions/route.tscomponents/frontend/src/app/projects/[name]/page.tsxcomponents/frontend/src/app/projects/[name]/scheduled-sessions/[scheduledSessionName]/_components/scheduled-session-details-card.tsxcomponents/frontend/src/app/projects/[name]/scheduled-sessions/[scheduledSessionName]/_components/scheduled-session-runs-table.tsxcomponents/frontend/src/app/projects/[name]/scheduled-sessions/[scheduledSessionName]/page.tsxcomponents/frontend/src/components/chat/AttachmentPreview.tsxcomponents/frontend/src/components/jira-connection-card.tsxcomponents/frontend/src/components/workspace-sections/create-scheduled-session-dialog.tsxcomponents/frontend/src/components/workspace-sections/scheduled-sessions-tab.tsxcomponents/frontend/src/lib/__tests__/cron.test.tscomponents/frontend/src/lib/cron.tscomponents/frontend/src/services/api/scheduled-sessions.tscomponents/frontend/src/services/queries/__tests__/use-scheduled-sessions.test.tscomponents/frontend/src/services/queries/index.tscomponents/frontend/src/services/queries/use-scheduled-sessions.tscomponents/frontend/src/types/api/index.tscomponents/frontend/src/types/api/scheduled-sessions.tscomponents/manifests/base/backend-deployment.yamlcomponents/manifests/base/operator-deployment.yamlcomponents/manifests/base/rbac/backend-clusterrole.yamlcomponents/manifests/base/rbac/operator-clusterrole.yamlcomponents/manifests/overlays/e2e/operator-config.yamlcomponents/manifests/overlays/kind/operator-config.yamlcomponents/operator/internal/handlers/projectsettings.gocomponents/operator/internal/handlers/projectsettings_test.gocomponents/operator/internal/trigger/trigger.gocomponents/operator/internal/trigger/trigger_test.gocomponents/operator/main.gocomponents/public-api/handlers/middleware.gocomponents/public-api/main.goscripts/setup-vertex-kind.sh
💤 Files with no reviewable changes (1)
- components/ambient-sdk/go-sdk/client/client.go
...ntend/src/app/api/projects/[name]/scheduled-sessions/[scheduledSessionName]/suspend/route.ts
Show resolved
Hide resolved
components/frontend/src/app/api/projects/[name]/scheduled-sessions/route.ts
Show resolved
Hide resolved
components/frontend/src/services/queries/__tests__/use-scheduled-sessions.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/frontend/src/components/workspace-sections/scheduled-sessions-tab.tsx`:
- Around line 70-79: The handleDelete function currently uses the native
confirm() which provides a generic browser UI; replace it with your app's custom
confirmation dialog (e.g., openConfirmDialog or <ConfirmDialog />) by triggering
the dialog from handleDelete(name) and moving the deleteMutation.mutate call
into the dialog's onConfirm handler so deletion only proceeds after user
confirmation; preserve the same mutation payload ({ projectName, name }) and the
existing onSuccess/onError toast handlers, and ensure the dialog supports
canceling (no-op) and accessible focus management.
In `@components/operator/internal/handlers/projectsettings_test.go`:
- Around line 122-200: Add a negative test that injects API errors via the fake
Kubernetes client to verify ensureSessionTriggerRBAC error handling: copy the
pattern from
TestEnsureSessionTriggerRBAC_Idempotent/TestEnsureSessionTriggerRBAC_MultipleNamespaces
but use setupProjectSettingsTestClient to create a fake client with a reactor
that returns a non-AlreadyExists error (e.g., permission denied) for
ServiceAccount/Role/RoleBinding Create or Get calls, call
ensureSessionTriggerRBAC(namespace, owner) and assert it returns the expected
error, and verify no partial resources were created via
config.K8sClient.CoreV1()/RbacV1() Get calls; reference
ensureSessionTriggerRBAC, setupProjectSettingsTestClient, and config.K8sClient
to locate where to wire the fake reactor and assertions.
In `@components/operator/internal/handlers/projectsettings.go`:
- Around line 153-160: The handler writes a status field
scheduledSessionRBACReady (see statusUpdate in projectsettings.go) that is not
declared in the ProjectSettings CRD schema, which can be dropped or cause
validation errors; update the ProjectSettings CRD schema to add a
status.properties.scheduledSessionRBACReady entry (type: boolean, add a concise
description like "Whether RBAC for scheduled session triggers is ready") and
ensure status is defined as an object so the controller can set that boolean
without schema validation failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a37c9025-22c0-41a9-993f-a9c450a22ff4
📒 Files selected for processing (9)
components/backend/handlers/scheduled_sessions.gocomponents/backend/types/scheduled_session.gocomponents/frontend/src/components/workspace-sections/create-scheduled-session-dialog.tsxcomponents/frontend/src/components/workspace-sections/scheduled-sessions-tab.tsxcomponents/frontend/src/services/api/scheduled-sessions.tscomponents/frontend/src/services/queries/__tests__/use-scheduled-sessions.test.tscomponents/operator/internal/handlers/projectsettings.gocomponents/operator/internal/handlers/projectsettings_test.gocomponents/operator/internal/trigger/trigger.go
components/frontend/src/components/workspace-sections/scheduled-sessions-tab.tsx
Show resolved
Hide resolved
Review Queue — Blockers Found
|
Introduce scheduled sessions that let users create cron-based schedules for automatically launching agentic sessions. The feature spans all layers of the platform: Backend: CRUD handlers for scheduled sessions backed by Kubernetes CronJobs, with suspend/resume, manual trigger, and run-history endpoints. Auth checks delegate to the user's token while CronJob operations use the backend service account. Operator: session-trigger subcommand that runs inside CronJob-spawned pods, reads a session template from env vars, and creates an AgenticSession CR. ProjectSettings reconciler ensures per-namespace RBAC (ServiceAccount, Role, RoleBinding) for the trigger SA. Frontend: Schedules tab in the project workspace, create dialog with cron presets and expression preview, detail page with run history, Next.js API route proxies, React Query hooks, and TypeScript types. Manifests: Backend ClusterRole gains CronJob and Job/create permissions, operator ClusterRole gains AgenticSession/create, backend deployment reads OPERATOR_IMAGE and IMAGE_PULL_POLICY from operator-config ConfigMap, Vertex AI config keys made optional to avoid clobbering. Includes unit tests for backend helpers (sanitizeLabelValue, cronJobToScheduledSession), operator trigger (sanitizeName), operator RBAC (ensureSessionTriggerRBAC), frontend query hooks, and cron utils. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Prpič <mprpic@redhat.com>
The session creation tests that disable Vertex AI only cleared USE_VERTEX but not the deprecated CLAUDE_CODE_USE_VERTEX env var. When the latter is set in the developer's shell, isVertexEnabled() still returns true, skipping the secret validation and causing a false 201 instead of 400. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Prpič <mprpic@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Prpič <mprpic@redhat.com>
- Add SelfSubjectAccessReview RBAC checks to all scheduled session handlers (list, get, create, update, delete, suspend/resume, trigger) - Make displayName optional in CreateScheduledSessionRequest - Fix trigger API response type mismatch (name/namespace vs message) - Stop leaking Gin binding errors to API responses - Propagate ensureSessionTriggerRBAC errors into ProjectSettings status - Add OwnerReferences to SA/Role/RoleBinding created by trigger RBAC - Add loading and error states to SchedulesSection component - Fix sanitizeName trailing-hyphen bug (truncate before trim) - Move create-scheduled-session-dialog to colocated location Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Prpič <mprpic@redhat.com>
mprpic
force-pushed
the
scheduled-sessions
branch
from
7b9f763 to
2548d9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/frontend/src/components/jira-connection-card.tsx (1)
40-49:⚠️ Potential issue | 🟠 MajorAvoid re-initializing form fields during user edits.
The dependency array includes
urlandusername, causing the effect to rerun whenever the user edits these fields. If the user clears either field while the form is open, the effect immediately restores the default URL or email, blocking the user from clearing or retyping values. Use functional state updates instead and remove the input values from dependencies to only trigger initialization when the form opens or the current user changes.Suggested fix
useEffect(() => { - if (showForm) { - if (!url) { - setUrl(DEFAULT_JIRA_URL) - } - if (!username && currentUser?.email) { - setUsername(currentUser.email) - } - } - }, [showForm, currentUser?.email, url, username]) + if (!showForm) return + + setUrl((prev) => prev || DEFAULT_JIRA_URL) + if (currentUser?.email) { + setUsername((prev) => prev || currentUser.email) + } + }, [showForm, currentUser?.email])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/components/jira-connection-card.tsx` around lines 40 - 49, The effect currently re-initializes fields while the user edits because url and username are in the dependency array; change the effect to only depend on showForm and currentUser?.email and use functional state updates so you only initialize when fields are empty. In the useEffect (the one referencing showForm, url, username), remove url and username from the deps (use [showForm, currentUser?.email]) and replace direct setUrl(DEFAULT_JIRA_URL)/setUsername(currentUser.email) with functional updates like setUrl(prev => prev ?? DEFAULT_JIRA_URL) and setUsername(prev => prev ?? currentUser?.email) so existing user edits are not overwritten.
♻️ Duplicate comments (2)
components/frontend/src/services/api/scheduled-sessions.ts (1)
14-95:⚠️ Potential issue | 🟡 MinorURL-encode path parameters to prevent malformed requests.
All functions in this file interpolate
projectNameandnamedirectly into URL paths. If these contain special characters (e.g.,/,?,#, spaces), the URLs will be malformed. ApplyencodeURIComponent()to all path parameters.Example fix pattern
export async function listScheduledSessions( projectName: string ): Promise<ScheduledSession[]> { const response = await apiClient.get<{ items: ScheduledSession[] }>( - `/projects/${projectName}/scheduled-sessions` + `/projects/${encodeURIComponent(projectName)}/scheduled-sessions` ); return response.items; }Apply the same pattern to
projectNameandnamein all 9 functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/services/api/scheduled-sessions.ts` around lines 14 - 95, The path parameters (projectName and name) are interpolated raw into URLs causing malformed requests for special characters; update every function that builds a URL—listScheduledSessions, createScheduledSession, getScheduledSession, updateScheduledSession, deleteScheduledSession, suspendScheduledSession, resumeScheduledSession, triggerScheduledSession, and listScheduledSessionRuns—to wrap projectName and name with encodeURIComponent() before constructing the path so all generated URLs are properly escaped.components/backend/handlers/scheduled_sessions.go (1)
239-301: 🧹 Nitpick | 🔵 TrivialRead-modify-write without conflict retry (existing pattern).
The Get-then-Update pattern can fail with a conflict error if another client modifies the CronJob concurrently. Kubernetes protects against silent data loss via
resourceVersion, so the current behavior is safe but may return 500 errors to users on rare conflicts. Consider adding retry logic forerrors.IsConflict(err)to improve UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/scheduled_sessions.go` around lines 239 - 301, The update handler uses a read-modify-write on the CronJob (cj) via K8sClientScheduled.BatchV1().CronJobs(...).Get and .Update and should retry on Kubernetes conflict errors; modify the update logic (around K8sClientScheduled.BatchV1().CronJobs(...).Update and the preceding cj modifications) to perform a small retry loop (e.g., 3 attempts with brief backoff) that on errors.IsConflict(err) re-fetches the latest cj (using the same Get call), reapplies the requested changes from req (Schedule, Suspend, DisplayName, SessionTemplate) to the fresh cj, and retries Update, returning the original error if non-conflict or retries exhausted; ensure you preserve the annotation and SESSION_TEMPLATE env var logic when reapplying changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/handlers/scheduled_sessions.go`:
- Around line 120-121: The generated schedule name using timestamp (variables
timestamp and name) can collide for concurrent creates; change the name
construction (the fmt.Sprintf("schedule-%d", timestamp) call) to append a short
random suffix (e.g., 4–6 hex or base36 chars) to make it unique—use a secure
random source (crypto/rand) or UnixNano combined with a small math/rand value
(and seed if using math/rand), convert the bytes/int to a short string, and
include it in the fmt.Sprintf (e.g., "schedule-%d-%s"); if using crypto/rand,
handle any error from the reader. Ensure imports are added/updated accordingly.
- Around line 486-489: The code silently ignores JSON unmarshal errors for
SESSION_TEMPLATE; update the block that unmarshals into
types.CreateAgenticSessionRequest so that when json.Unmarshal returns an error
you log the error (including the erroneous env.Value and the error) using the
repository's existing logger (or fmt.Errorf/log.Printf if none), and only assign
to ss.SessionTemplate when unmarshalling succeeds; ensure the log message
includes context like "failed to parse SESSION_TEMPLATE" and the error details
to aid debugging.
In `@components/backend/handlers/sessions_test.go`:
- Around line 443-448: The test saves env vars using os.Getenv which cannot
distinguish unset vs empty; change the capture to use os.LookupEnv for
USE_VERTEX and CLAUDE_CODE_USE_VERTEX (e.g., originalVertexValuePresent,
originalVertexValue, originalClaudeVertexValuePresent,
originalClaudeVertexValue) and update the deferred restore logic in the test
(around the originalVertexValue/originalClaudeVertexValue setup and defers in
sessions_test.go) to call os.Unsetenv when the var was originally absent and
os.Setenv when it was present, so the tests restore the original
presence/absence rather than always setting the variables back.
In `@components/backend/server/k8s.go`:
- Around line 85-95: The code currently falls back to a floating "latest"
operator image which can cause mismatches; change the logic around OperatorImage
and ImagePullPolicy so OPERATOR_IMAGE must be explicitly provided or the service
fails fast: if os.Getenv("OPERATOR_IMAGE") returns empty, return an error (or
log fatal) instead of defaulting to
"quay.io/ambient_code/vteam_operator:latest"; likewise, remove the implicit
ImagePullPolicy default of "IfNotPresent" when using an unspecified tag — either
require IMAGE_PULL_POLICY to be set or set ImagePullPolicy="Always" when a
non-pinned tag is used; update references to OperatorImage and ImagePullPolicy
in k8s setup code to reflect this validation and fail-fast behavior.
In
`@components/frontend/src/app/api/projects/`[name]/scheduled-sessions/[scheduledSessionName]/trigger/route.ts:
- Around line 15-16: The handler currently always calls response.text() and
returns new Response(text, ...) which will throw for null-body statuses (204,
205, 304); update the logic in the route handler to first check response.status
and if it is one of 204/205/304 return new Response(null, { status:
response.status }) (omit Content-Type), otherwise await response.text() and
return new Response(text, { status: response.status, headers: { 'Content-Type':
'application/json' } }); reference the existing variables/expressions response,
response.status, response.text(), text and new Response when locating the fix.
In
`@components/frontend/src/app/projects/`[name]/scheduled-sessions/[scheduledSessionName]/page.tsx:
- Around line 44-48: The page currently treats a missing scheduledSession the
same for all failures because it only checks scheduledSession/undefined; update
the render logic around useScheduledSession and useScheduledSessionRuns to
distinguish network/auth/server errors from a true 404: use the hook's isError
and error (from useScheduledSession) to render an explicit error state for
non-404 errors (e.g., 403/500/network), and only show the "not found" 404-style
UI when the error indicates a 404 status or when scheduledSession is explicitly
null/missing and there is no other error; apply the same distinction for the
runs block (lines 108-114) so useScheduledSessionRuns' isError/error are handled
separately from a genuine not-found case.
In
`@components/frontend/src/components/workspace-sections/create-scheduled-session-dialog.tsx`:
- Around line 116-120: The useEffect currently depends on the whole form object;
change its dependency array to only the specific stable values used:
modelsData?.defaultModel, form.formState.dirtyFields.model, and form.setValue.
Keep the effect body the same (checking modelsData?.defaultModel and
!form.formState.dirtyFields.model, then calling form.setValue("model", ... , {
shouldDirty: false })), but replace the dependency list in the useEffect
declaration so it reads something like useEffect(..., [modelsData?.defaultModel,
form.formState.dirtyFields.model, form.setValue]) to make triggers explicit and
avoid depending on the entire form reference.
In
`@components/frontend/src/components/workspace-sections/scheduled-sessions-tab.tsx`:
- Around line 148-159: The Link href in scheduled-sessions-tab.tsx uses
projectName directly which can break URLs with special characters; update the
href to use encodeURIComponent(projectName) so it matches the runs table
behavior. Locate the Link element that builds the path
`/projects/${projectName}/scheduled-sessions/${ss.name}` (inside the
scheduled-sessions Tab component) and wrap projectName with encodeURIComponent
before interpolation; keep ss.name as-is if already encoded elsewhere or apply
encodeURIComponent there as well for consistency. Ensure the className and inner
JSX remain unchanged and run the app to verify links navigate correctly.
In `@components/manifests/base/rbac/backend-clusterrole.yaml`:
- Around line 67-70: The ClusterRole named backend-api currently grants
cluster-wide management of CronJobs (apiGroups: ["batch"], resources:
["cronjobs"], verbs: ["get","list","watch","create","update","patch","delete"])
which triggers Trivy KSV-0048; either restrict this by replacing the ClusterRole
permission with a namespace-scoped Role bound to the backend service account per
project namespace, or explicitly document/approve the tradeoff. Update the
manifest by removing or narrowing the cronjobs rules from the ClusterRole and
instead create a Role (or Roles) with the same verbs scoped to target namespaces
and bind them via RoleBinding(s) to the backend service account, or add a clear
comment and security-approval note on the ClusterRole if cluster-wide management
is intentional.
In `@components/operator/internal/handlers/projectsettings.go`:
- Around line 257-316: ensureSessionTriggerRBAC currently treats AlreadyExists
as success which can hide drift; update it so when Create returns AlreadyExists
you fetch the existing object (use K8sClient.CoreV1().ServiceAccounts(...).Get,
RbacV1().Roles(...).Get, RbacV1().RoleBindings(...).Get), compare and reconcile
the live object's important fields (for ServiceAccount: labels and
OwnerReferences; for Role: Rules, labels and OwnerReferences; for RoleBinding:
RoleRef, Subjects, labels and OwnerReferences) and perform an Update or Patch to
align the live resource with the desired sa/role/rb (saName, roleName, rbName)
using optimistic retries to handle conflicts; ensure you avoid changing
immutable fields (e.g., namespace/name) and preserve other existing fields not
managed by us.
In `@components/public-api/main.go`:
- Around line 145-149: The CRC origin pattern "https://*.apps-crc.testing" in
the returned origins slice must be matched by enabling wildcard support in the
CORS middleware: set AllowWildcard (or the equivalent option on the CORS config
used with this origins slice) to true where the CORS options are constructed so
that the returned []string including "https://*.apps-crc.testing" is honored;
update the CORS configuration that consumes this origins list (the
middleware/option construction) to include AllowWildcard: true.
---
Outside diff comments:
In `@components/frontend/src/components/jira-connection-card.tsx`:
- Around line 40-49: The effect currently re-initializes fields while the user
edits because url and username are in the dependency array; change the effect to
only depend on showForm and currentUser?.email and use functional state updates
so you only initialize when fields are empty. In the useEffect (the one
referencing showForm, url, username), remove url and username from the deps (use
[showForm, currentUser?.email]) and replace direct
setUrl(DEFAULT_JIRA_URL)/setUsername(currentUser.email) with functional updates
like setUrl(prev => prev ?? DEFAULT_JIRA_URL) and setUsername(prev => prev ??
currentUser?.email) so existing user edits are not overwritten.
---
Duplicate comments:
In `@components/backend/handlers/scheduled_sessions.go`:
- Around line 239-301: The update handler uses a read-modify-write on the
CronJob (cj) via K8sClientScheduled.BatchV1().CronJobs(...).Get and .Update and
should retry on Kubernetes conflict errors; modify the update logic (around
K8sClientScheduled.BatchV1().CronJobs(...).Update and the preceding cj
modifications) to perform a small retry loop (e.g., 3 attempts with brief
backoff) that on errors.IsConflict(err) re-fetches the latest cj (using the same
Get call), reapplies the requested changes from req (Schedule, Suspend,
DisplayName, SessionTemplate) to the fresh cj, and retries Update, returning the
original error if non-conflict or retries exhausted; ensure you preserve the
annotation and SESSION_TEMPLATE env var logic when reapplying changes.
In `@components/frontend/src/services/api/scheduled-sessions.ts`:
- Around line 14-95: The path parameters (projectName and name) are interpolated
raw into URLs causing malformed requests for special characters; update every
function that builds a URL—listScheduledSessions, createScheduledSession,
getScheduledSession, updateScheduledSession, deleteScheduledSession,
suspendScheduledSession, resumeScheduledSession, triggerScheduledSession, and
listScheduledSessionRuns—to wrap projectName and name with encodeURIComponent()
before constructing the path so all generated URLs are properly escaped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0dfd1b92-c87f-4679-aac8-e17a6796c80b
⛔ Files ignored due to path filters (1)
components/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (47)
Makefilecomponents/backend/handlers/scheduled_sessions.gocomponents/backend/handlers/scheduled_sessions_test.gocomponents/backend/handlers/sessions_test.gocomponents/backend/handlers/test_helpers_test.gocomponents/backend/main.gocomponents/backend/routes.gocomponents/backend/server/k8s.gocomponents/backend/types/scheduled_session.gocomponents/frontend/package.jsoncomponents/frontend/src/app/api/projects/[name]/scheduled-sessions/[scheduledSessionName]/resume/route.tscomponents/frontend/src/app/api/projects/[name]/scheduled-sessions/[scheduledSessionName]/route.tscomponents/frontend/src/app/api/projects/[name]/scheduled-sessions/[scheduledSessionName]/runs/route.tscomponents/frontend/src/app/api/projects/[name]/scheduled-sessions/[scheduledSessionName]/suspend/route.tscomponents/frontend/src/app/api/projects/[name]/scheduled-sessions/[scheduledSessionName]/trigger/route.tscomponents/frontend/src/app/api/projects/[name]/scheduled-sessions/route.tscomponents/frontend/src/app/projects/[name]/page.tsxcomponents/frontend/src/app/projects/[name]/scheduled-sessions/[scheduledSessionName]/_components/scheduled-session-details-card.tsxcomponents/frontend/src/app/projects/[name]/scheduled-sessions/[scheduledSessionName]/_components/scheduled-session-runs-table.tsxcomponents/frontend/src/app/projects/[name]/scheduled-sessions/[scheduledSessionName]/page.tsxcomponents/frontend/src/components/chat/AttachmentPreview.tsxcomponents/frontend/src/components/jira-connection-card.tsxcomponents/frontend/src/components/workspace-sections/create-scheduled-session-dialog.tsxcomponents/frontend/src/components/workspace-sections/scheduled-sessions-tab.tsxcomponents/frontend/src/lib/__tests__/cron.test.tscomponents/frontend/src/lib/cron.tscomponents/frontend/src/services/api/scheduled-sessions.tscomponents/frontend/src/services/queries/__tests__/use-scheduled-sessions.test.tscomponents/frontend/src/services/queries/index.tscomponents/frontend/src/services/queries/use-scheduled-sessions.tscomponents/frontend/src/types/api/index.tscomponents/frontend/src/types/api/scheduled-sessions.tscomponents/manifests/base/backend-deployment.yamlcomponents/manifests/base/crds/projectsettings-crd.yamlcomponents/manifests/base/operator-deployment.yamlcomponents/manifests/base/rbac/backend-clusterrole.yamlcomponents/manifests/base/rbac/operator-clusterrole.yamlcomponents/manifests/overlays/e2e/operator-config.yamlcomponents/manifests/overlays/kind/operator-config.yamlcomponents/operator/internal/handlers/projectsettings.gocomponents/operator/internal/handlers/projectsettings_test.gocomponents/operator/internal/trigger/trigger.gocomponents/operator/internal/trigger/trigger_test.gocomponents/operator/main.gocomponents/public-api/handlers/middleware.gocomponents/public-api/main.goscripts/setup-vertex-kind.sh
...ntend/src/app/api/projects/[name]/scheduled-sessions/[scheduledSessionName]/trigger/route.ts
Show resolved
Hide resolved
components/frontend/src/components/workspace-sections/create-scheduled-session-dialog.tsx
Show resolved
Hide resolved
components/frontend/src/components/workspace-sections/scheduled-sessions-tab.tsx
Show resolved
Hide resolved
Merge Queue Status
This pull request spent 5 seconds in the queue, with no time running CI. Required conditions to merge
|
2 participants
Introduce scheduled sessions that let users create cron-based schedules
for automatically launching agentic sessions. The feature spans all
layers of the platform:
Backend: CRUD handlers for scheduled sessions backed by Kubernetes
CronJobs, with suspend/resume, manual trigger, and run-history
endpoints. Auth checks delegate to the user's token while CronJob
operations use the backend service account.
Operator: session-trigger subcommand that runs inside CronJob-spawned
pods, reads a session template from env vars, and creates an
AgenticSession CR. ProjectSettings reconciler ensures per-namespace
RBAC (ServiceAccount, Role, RoleBinding) for the trigger SA.
Frontend: Schedules tab in the project workspace, create dialog with
cron presets and expression preview, detail page with run history,
Next.js API route proxies, React Query hooks, and TypeScript types.
Manifests: Backend ClusterRole gains CronJob and Job/create permissions,
operator ClusterRole gains AgenticSession/create, backend deployment
reads OPERATOR_IMAGE and IMAGE_PULL_POLICY from operator-config
ConfigMap, Vertex AI config keys made optional to avoid clobbering.
Includes unit tests for backend helpers (sanitizeLabelValue,
cronJobToScheduledSession), operator trigger (sanitizeName), operator
RBAC (ensureSessionTriggerRBAC), frontend query hooks, and cron utils.