feat: separate aggregate and no-workspace scopes#2019
feat: separate aggregate and no-workspace scopes#2019
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds workspace-scoped access models and enforcement across backend and UI: new WorkspaceAccess/WorkspaceGrant types, label→workspace extraction, WorkspaceFilter propagation into stores and search cursors, workspace-scoped authorization checks in many API endpoints, OpenAPI/UI schema updates, and extensive frontend workspace-selection plumbing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FrontendAPI as "Frontend API"
participant Auth as "Auth Service"
participant Store as "Persistence/Store"
Note over FrontendAPI: parse workspaceScope/workspace → selection/workspaceFilter
Client->>FrontendAPI: HTTP request (workspaceScope?, workspace?, action)
FrontendAPI->>Auth: validate/authorize workspace access/role
Auth-->>FrontendAPI: allowed / denied
alt allowed
FrontendAPI->>Store: query/list with WorkspaceFilter
Store-->>FrontendAPI: filtered results (items include optional workspace)
FrontendAPI-->>Client: 200 + payload
else denied
FrontendAPI-->>Client: 403 or 404
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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 (16)
ui/src/api/v1/schema.ts (1)
12946-12954:⚠️ Potential issue | 🟠 MajorDon’t expose aggregate scope on document mutations.
These write endpoints now accept
WorkspaceScope, so TypeScript allowsworkspaceScope=accessibleeven though mutations should remain outside aggregate scope. Use a narrower write-scope parameter such asnone | workspace, or keep only the legacyworkspaceselector for mutations.Update the OpenAPI source and regenerate this file instead of editing it directly. As per coding guidelines, “Frontend API types must be generated from OpenAPI spec via
pnpm gen:apito maintain type safety with the backend.”Illustrative generated shape after narrowing mutation scopes
- workspaceScope?: components["parameters"]["WorkspaceScope"]; + workspaceScope?: components["parameters"]["WorkspaceMutationScope"]; workspace?: components["parameters"]["Workspace"];WorkspaceScope: components["schemas"]["WorkspaceScope"]; + /** `@description` Explicit workspace scope for write APIs: no-workspace data or one named workspace */ + WorkspaceMutationScope: components["schemas"]["WorkspaceMutationScope"];export enum WorkspaceScope { accessible = "accessible", none = "none", workspace = "workspace" } + +export enum WorkspaceMutationScope { + none = "none", + workspace = "workspace" +}Also applies to: 13100-13108, 13145-13153, 13198-13206, 13260-13268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/api/v1/schema.ts` around lines 12946 - 12954, The generated API type for createDoc (and other mutation endpoints around the same areas) incorrectly exposes the aggregate WorkspaceScope parameter; update the OpenAPI spec so mutation endpoints use a narrower write-only scope (e.g., an enum limited to "none" | "workspace" or remove WorkspaceScope and keep only the legacy workspace parameter) instead of components["parameters"]["WorkspaceScope"], then regenerate the TS types via the generator (run pnpm gen:api) so createDoc.parameters.query and the similar mutation parameter groups reflect the narrowed write-scope; do not edit ui/src/api/v1/schema.ts directly.internal/persis/filedag/store.go (1)
841-867:⚠️ Potential issue | 🟠 MajorCheck workspace access before reading the DAG body.
SearchMatchescurrently reads the YAML contents before applyingWorkspaceFilter. Move the metadata/workspace check aboveos.ReadFileto avoid unnecessary reads and out-of-scope existence/error side channels.Proposed ordering fix
filePath, err := store.locateDAG(fileName) if err != nil { return nil, exec.ErrDAGNotFound } - dat, err := os.ReadFile(filePath) //nolint:gosec - if err != nil { - if errors.Is(err, os.ErrNotExist) { - return nil, exec.ErrDAGNotFound - } - return nil, err - } if len(opts.Labels) > 0 || opts.WorkspaceFilter != nil { dag, err := spec.Load(ctx, filePath, store.defaultLoadOptions( spec.OnlyMetadata(), spec.WithoutEval(), spec.SkipSchemaValidation(), @@ if !opts.WorkspaceFilter.MatchesLabels(dag.Labels) { return &exec.CursorResult[*exec.Match]{Items: []*exec.Match{}}, nil } } + + dat, err := os.ReadFile(filePath) //nolint:gosec + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil, exec.ErrDAGNotFound + } + return nil, err + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/persis/filedag/store.go` around lines 841 - 867, In SearchMatches, avoid reading the DAG body before checking workspace/label access: after locating the file with store.locateDAG (filePath), perform the metadata-only load via spec.Load (using store.defaultLoadOptions with spec.OnlyMetadata(), spec.WithoutEval(), spec.SkipSchemaValidation(), spec.WithAllowBuildErrors()) and run containsAllLabels(dag.Labels, opts.Labels) and opts.WorkspaceFilter.MatchesLabels(dag.Labels) first; only if those checks pass should you proceed to read the full file with os.ReadFile and continue normal processing. Ensure you keep the same error handling (return exec.ErrDAGNotFound for missing files) and preserve existing behavior for nil WorkspaceFilter or empty Labels.ui/src/menu.tsx (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd the GPL header to this touched TSX source file.
Proposed header
+// Copyright (C) 2026 Yota Hamada +// SPDX-License-Identifier: GPL-3.0-or-later + import {As per coding guidelines, "
**/*.{go,ts,tsx,js}: Apply GPL v3 license headers on source files, managed viamake addlicense."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/menu.tsx` at line 1, Add the GPL v3 license header to the top of the modified TSX file (menu.tsx) so it matches the project's expected format produced by make addlicense; insert the standard GPL header comment block before the first import statement (the initial import { ... } line) so the header sits at the very top of the file and ensure spelling/format and year/owner placeholders match the project's license template.internal/service/frontend/api/v1/queues.go (1)
287-296:⚠️ Potential issue | 🟠 MajorAvoid exposing queues that have no visible items.
Line 295 creates a queue resource for every queue store name even when
count == 0. For DAG-based queues, that can reveal inaccessible DAG/queue names to users who have no visible running or queued items in that queue.Proposed fix
for _, queueName := range queueNames { count, err := a.countVisibleQueuedItems(ctx, queueName) if err != nil { logger.Warn(ctx, "Failed to get queue length", tag.Queue(queueName), tag.Error(err)) continue } + if count == 0 { + if _, exists := queueMap[queueName]; !exists { + continue + } + } queue := getOrCreateQueue(queueMap, queueName, a.config) queue.queuedCount = count }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/queues.go` around lines 287 - 296, The code currently calls getOrCreateQueue and sets queue.queuedCount even when count == 0, which can expose empty/inaccessible DAG-based queues; change the loop in the handler that iterates queueNames so that after calling a.countVisibleQueuedItems(ctx, queueName) you skip creating or adding the queue to queueMap when count == 0 (only call getOrCreateQueue and set queuedCount when count > 0), preserving the existing error handling for non-nil err from countVisibleQueuedItems and using the same queueMap, getOrCreateQueue, and queuedCount fields.ui/src/pages/dags/dag/index.tsx (1)
138-150:⚠️ Potential issue | 🟠 MajorDon’t treat unloaded or empty-label DAG metadata as no-workspace.
Line 138 falls back to
tagsonly whenlabelsis nullish, not when it is an empty array. Combined with Line 143,WorkspaceScope.nonealso matches beforedagDatahas loaded, which can enable DAG-run requests for a workspace DAG under the no-workspace selection.Proposed fix
- const dagWorkspaceName = workspaceNameFromLabels( - dagData?.dag?.labels ?? dagData?.dag?.tags ?? [] - ); + const dagLabels = [ + ...(dagData?.dag?.labels ?? []), + ...(dagData?.dag?.tags ?? []), + ]; + const dagWorkspaceName = workspaceNameFromLabels(dagLabels); + const dagLoaded = Boolean(dagData?.dag); const dagMatchesWorkspace = workspaceSelection.scope === WorkspaceScope.accessible || - (workspaceSelection.scope === WorkspaceScope.none && !dagWorkspaceName) || - (workspaceSelection.scope === WorkspaceScope.workspace && - dagWorkspaceName === workspaceSelection.workspace); + (dagLoaded && + ((workspaceSelection.scope === WorkspaceScope.none && !dagWorkspaceName) || + (workspaceSelection.scope === WorkspaceScope.workspace && + dagWorkspaceName === workspaceSelection.workspace)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/dags/dag/index.tsx` around lines 138 - 150, dagWorkspaceName is computed with dagData?.dag?.labels ?? dagData?.dag?.tags which treats an empty labels array as present and hides tags, and dagMatchesWorkspace allows WorkspaceScope.none to match before dagData is loaded; update the logic so dagWorkspaceName prefers labels only when labels exists and has length (e.g., use labels.length > 0 ? labels : tags) and make dagMatchesWorkspace/ dagRunQueryEnabled require dagData?.dag to be loaded before treating the "none" scope as a match (ensure you reference and gate with dagData?.dag or dagWorkspaceName defined); adjust the expressions around dagWorkspaceName, dagMatchesWorkspace, and dagRunQueryEnabled accordingly to prevent enabling DAG-run requests for unloaded or mis-labeled DAGs.ui/src/pages/docs/index.tsx (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd the GPL license header.
Line 1 starts directly with imports, so this TSX source file is missing the required GPL v3 header. As per coding guidelines,
**/*.{go,ts,tsx,js}: Apply GPL v3 license headers on source files, managed viamake addlicense.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/docs/index.tsx` at line 1, This TSX file (the one that currently begins with "import { ChevronLeft } from 'lucide-react';") is missing the GPL v3 license header; add the standard GPL v3 header comment block at the very top of the file (above all imports) consistent with the project's header format used by make addlicense so the file containing the ChevronLeft import complies with the repository's licensing policy.internal/service/frontend/api/v1/docs.go (1)
282-316:⚠️ Potential issue | 🟠 MajorApply workspace visibility before pagination.
These paths paginate first, then filter hidden docs. Restricted users can see pagination totals/pages for inaccessible docs, and valid visible docs may be pushed onto later pages or omitted from the current page. Filter at the store/query layer, or build pagination metadata from the filtered result set.
Also applies to: 649-669
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/docs.go` around lines 282 - 316, The current branches call a.docStore.ListFlat / a.docStore.List with opts and paginate the raw result, then apply filterDocMetadataByWorkspace or filterDocTreeByWorkspace which causes pagination/counts to include hidden items; change the flow to apply workspace visibility before computing/returning pagination: either (preferred) pass visibility into the store query via opts so ListFlat/List return already-filtered results, or if changing the store API is not possible, filter result.Items immediately after receiving the store response and then rebuild the pagination metadata from that filtered slice (use the filtered length/items when creating the Pagination passed to api.ListDocs200JSONResponse); apply this fix in both the flat branch (affecting a.docStore.ListFlat, toDocMetadataResponse, filterDocMetadataByWorkspace, toPagination) and the tree branch (affecting a.docStore.List, toDocTreeResponse, filterDocTreeByWorkspace, toPagination) so counts/pages reflect only visible docs.internal/service/auth/service.go (1)
313-326:⚠️ Potential issue | 🟠 MajorValidate candidate role and workspace access before mutating the cached user/API key object.
Lines 313-326 and 564-577 mutate the objects loaded from the store before validation. Since the stores use file-based caching that returns live pointers (not clones), validation failures leave the in-memory cached objects in a corrupted state. Subsequent calls to GetByID return the invalid state rather than reloading from disk.
Compute candidate role and workspace access in local variables, validate them together, then assign to the object only after validation succeeds.
Safer pattern for UpdateUser (apply same pattern to UpdateAPIKey)
- if input.Role != nil { + nextRole := user.Role + if input.Role != nil { if !input.Role.Valid() { return nil, fmt.Errorf("invalid role: %s", *input.Role) } - user.Role = *input.Role + nextRole = *input.Role } + nextWorkspaceAccess := user.WorkspaceAccess if input.WorkspaceAccess != nil { - user.WorkspaceAccess = auth.CloneWorkspaceAccess(input.WorkspaceAccess) + nextWorkspaceAccess = auth.CloneWorkspaceAccess(input.WorkspaceAccess) } - if err := auth.ValidateWorkspaceAccess(user.Role, user.WorkspaceAccess, nil); err != nil { + if err := auth.ValidateWorkspaceAccess(nextRole, nextWorkspaceAccess, nil); err != nil { return nil, err } + + user.Role = nextRole + user.WorkspaceAccess = nextWorkspaceAccess🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/auth/service.go` around lines 313 - 326, The code mutates cached user/API key objects directly before validation; instead, in UpdateUser (and symmetrically in UpdateAPIKey) compute candidateRole and candidateWorkspaceAccess into local variables (e.g., candidateRole := user.Role or *input.Role when provided, and candidateWorkspaceAccess := auth.CloneWorkspaceAccess(input.WorkspaceAccess) or user.WorkspaceAccess), call auth.ValidateWorkspaceAccess(candidateRole, candidateWorkspaceAccess, nil) and only after validation succeeds assign user.Role = candidateRole and user.WorkspaceAccess = candidateWorkspaceAccess; apply the same local-variable-then-validate-then-assign pattern wherever the code currently writes to user.Role/user.WorkspaceAccess or apiKey.Role/apiKey.WorkspaceAccess before validation.internal/service/frontend/api/v1/dags.go (3)
895-954:⚠️ Potential issue | 🔴 CriticalAuthorize against the runtime workspace labels before starting or enqueueing.
These endpoints check
dagWorkspaceName(dag)before parsing request labels, butstartDAGRun/enqueueDAGRunlater pass those labels into the run. A caller can supply a workspace label for a workspace they cannot execute and bypass the workspace check.🔒 Proposed fix pattern
- if err := a.requireExecuteForWorkspace(ctx, dagWorkspaceName(dag)); err != nil { - return nil, err - } - if err := buildErrorsToAPIError(dag.BuildErrors); err != nil { return nil, err } @@ labels, err := extractLabelsParam(request.Body.Labels, request.Body.Tags) if err != nil { return nil, err } + if err := a.requireExecuteForWorkspace(ctx, runtimeWorkspaceName(dag, labels)); err != nil { + return nil, err + } if err := a.startDAGRun(ctx, dag, params, dagRunId, nameOverride, labels); err != nil {Apply the same ordering to
ExecuteDAGSyncandEnqueueDAGDAGRun.Also applies to: 999-1047, 1424-1483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/dags.go` around lines 895 - 954, The code checks workspace execution permission with requireExecuteForWorkspace(dagWorkspaceName(dag)) before parsing request labels, which lets a caller supply a different workspace label in request.Body.Labels and bypass authorization when those labels are later used by startDAGRun/enqueueDAGRun; to fix, validate/authorize the effective workspace labels before using them by extracting labels early (using extractLabelsParam or equivalent) and calling requireExecuteForWorkspace for the workspace derived from those labels (or ensure the workspace label cannot be overridden) in ExecuteDAGSync and EnqueueDAGRun (and the other affected handlers), then proceed with generate/validate dagRunId, singleton checks, ensureDAGRunIDUnique, and finally call startDAGRun/enqueueDAGRun with the now-authorized labels.
1693-1708:⚠️ Potential issue | 🔴 CriticalAdd the missing workspace check to DAG history SSE.
GetDAGDAGRunHistoryenforcesrequireWorkspaceVisible, but the SSE data method returns the same history without that check.🔒 Proposed fix
func (a *API) GetDAGHistoryData(ctx context.Context, fileName string) (any, error) { + dag, _ := a.dagStore.GetDetails(ctx, fileName, spec.WithAllowBuildErrors()) + if err := a.requireWorkspaceVisible(ctx, dagWorkspaceName(dag)); err != nil { + return nil, err + } dagName := a.resolveDAGName(ctx, fileName) recentHistory := a.dagRunMgr.ListRecentStatus(ctx, dagName, defaultHistoryLimit) @@ - dag, _ := a.dagStore.GetDetails(ctx, fileName, spec.WithAllowBuildErrors()) - gridData := a.readHistoryData(ctx, dag, recentHistory)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/dags.go` around lines 1693 - 1708, GetDAGHistoryData is missing the same workspace visibility guard used by GetDAGDAGRunHistory; after fetching dag via a.dagStore.GetDetails (or immediately after resolving dagName) call a.requireWorkspaceVisible(ctx, dag) and return the appropriate error if it fails, before calling a.readHistoryData and returning the DAG run payload; ensure you reference the existing requireWorkspaceVisible method so the SSE endpoint enforces workspace access identical to the non-SSE handler.
833-880:⚠️ Potential issue | 🟠 MajorAuthorize the actual DAG-run workspace before returning run details.
This endpoint checks the current DAG metadata workspace, but historical runs can carry their own workspace label. If a DAG was relabeled, users with access to the current workspace can read older runs from a different workspace.
🔒 Proposed fix pattern
attempt, err := a.dagRunStore.FindAttempt(ctx, exec.NewDAGRunRef(dag.Name, dagRunId)) if err != nil { @@ return nil, fmt.Errorf("error getting DAG run attempt: %w", err) } + runWorkspaceName, err := workspaceNameForAttempt(ctx, attempt) + if err != nil { + return nil, err + } + if err := a.requireWorkspaceVisible(ctx, runWorkspaceName); err != nil { + return nil, err + } dagStatus, err := a.dagRunMgr.GetCurrentStatus(ctx, dag, dagRunId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/dags.go` around lines 833 - 880, The handler returns DAG-run details using attempt (from a.dagRunStore.FindAttempt / LatestAttempt) without verifying the run's own workspace label; ensure you authorize access against the actual run workspace before returning results by extracting the workspace label from the retrieved attempt (the DAG run object returned by FindAttempt/LatestAttempt) and calling the same workspace-authorization routine used for the DAG metadata (e.g., the existing authorize / authorizeWorkspace helper) prior to calling a.dagRunMgr.GetCurrentStatus / a.dagRunMgr.GetLatestStatus and before building the response with a.toDAGRunDetailsWithSpecSource so historical runs in a different workspace are denied to unauthorized users.internal/service/frontend/api/v1/dagruns.go (5)
3761-3865:⚠️ Potential issue | 🔴 CriticalApply workspace filtering to the DAG-runs SSE list path.
The HTTP list endpoints add
exec.WithWorkspaceFilter, butGetDAGRunsListDatastill lists from query params without deriving a workspace filter. Scoped users can receive DAG runs outside their workspace through the SSE data path.🔒 Proposed fix
func (a *API) GetDAGRunsListData(ctx context.Context, queryString string) (any, error) { - opts, err := dagRunListOptionsFromQueryString(ctx, queryString) + opts, err := a.dagRunListOptionsFromQueryString(ctx, queryString) if err != nil { return nil, err } @@ -func dagRunListOptionsFromQueryString(ctx context.Context, queryString string) (dagRunListOptions, error) { +func (a *API) dagRunListOptionsFromQueryString(ctx context.Context, queryString string) (dagRunListOptions, error) { params, err := url.ParseQuery(queryString) if err != nil { logger.Warn(ctx, "Failed to parse query string for DAG runs list", @@ - return buildDAGRunListOptions(dagRunListFilterInput{ + opts := buildDAGRunListOptions(dagRunListFilterInput{ statuses: statusValues, fromDate: fromDate, toDate: toDate, name: name, dagRunID: dagRunID, labels: labels, limit: limit, cursor: cursor, - }), nil + }) + scopeParam, workspaceParam := workspaceScopeParamsFromValues(params) + workspaceFilter, err := a.workspaceFilterForParams(ctx, scopeParam, workspaceParam) + if err != nil { + return dagRunListOptions{}, err + } + opts.query = append(opts.query, exec.WithWorkspaceFilter(workspaceFilter)) + return opts, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/dagruns.go` around lines 3761 - 3865, GetDAGRunsListData/dagRunListOptionsFromQueryString currently build list options from query params but never apply the workspace filter, allowing scoped users to receive DAG runs outside their workspace; fix by deriving the workspace filter from ctx (same way HTTP list endpoints do) and include exec.WithWorkspaceFilter in the options passed to a.dagRunStore.ListStatusesPage — update dagRunListOptionsFromQueryString (or the returned dagRunListOptions.query slice built by buildDAGRunListOptions/dagRunListFilterInput) to append the workspace filter obtained from the context before returning so GetDAGRunsListData uses it when calling ListStatusesPage.
1074-1097:⚠️ Potential issue | 🔴 CriticalProtect alternate step-log read paths with workspace visibility checks.
GetDAGRunStepLogchecks workspace visibility, butDownloadDAGRunStepLogand the SSEGetStepLogDatapath read the same run logs without that check. A scoped user can bypass the protected JSON endpoint by using these alternate paths.🔒 Proposed fix
func (a *API) DownloadDAGRunStepLog(ctx context.Context, request api.DownloadDAGRunStepLogRequestObject) (api.DownloadDAGRunStepLogResponseObject, error) { ref := exec.NewDAGRunRef(request.Name, request.DagRunId) dagStatus, err := a.dagRunMgr.GetSavedStatus(ctx, ref) if err != nil { @@ Message: fmt.Sprintf("dag-run ID %s not found for DAG %s", request.DagRunId, request.Name), }, nil } + if err := a.requireWorkspaceVisible(ctx, statusWorkspaceName(dagStatus)); err != nil { + return nil, err + } node, err := dagStatus.NodeByName(request.StepName) @@ func (a *API) GetStepLogData(ctx context.Context, identifier string) (any, error) { @@ dagStatus, err := a.dagRunMgr.GetSavedStatus(ctx, ref) if err != nil { return nil, fmt.Errorf("dag-run ID %s not found for DAG %s", dagRunId, dagName) } + if err := a.requireWorkspaceVisible(ctx, statusWorkspaceName(dagStatus)); err != nil { + return nil, err + } node, err := dagStatus.NodeByName(stepName)Also applies to: 3422-3439
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/dagruns.go` around lines 1074 - 1097, DownloadDAGRunStepLog reads run log files without enforcing the same workspace visibility check used by GetDAGRunStepLog, allowing scoped users to bypass protections; update DownloadDAGRunStepLog to call the same workspace visibility/authorization check used by GetDAGRunStepLog (i.e., ensure the workspace access check that runs before dagRunMgr.GetSavedStatus or immediately after obtaining dagStatus is invoked) before attempting to read the file (before os.ReadFile on logFile), and apply the identical visibility check to the SSE GetStepLogData path so both alternate log-read routes enforce workspace visibility.
1903-2200:⚠️ Potential issue | 🔴 CriticalGate sub-DAG read/download/artifact endpoints by workspace.
The sub-DAG details/spec/log/artifact endpoints return data without a workspace visibility check, while the root DAG-run equivalents are protected. Add
requireDAGRunVisibleor attempt-based workspace checks before reading sub-run contents.🔒 Proposed fix pattern
func (a *API) GetSubDAGRunDetails(ctx context.Context, request api.GetSubDAGRunDetailsRequestObject) (api.GetSubDAGRunDetailsResponseObject, error) { root := exec.NewDAGRunRef(request.Name, request.DagRunId) + if err := a.requireDAGRunVisible(ctx, root); err != nil { + return nil, err + } dagStatus, err := withDAGRunReadTimeout(ctx, dagRunReadRequestInfo{ @@ func (a *API) GetSubDAGRunSpec(ctx context.Context, request api.GetSubDAGRunSpecRequestObject) (api.GetSubDAGRunSpecResponseObject, error) { root := exec.NewDAGRunRef(request.Name, request.DagRunId) + if err := a.requireDAGRunVisible(ctx, root); err != nil { + return nil, err + } attempt, err := a.dagRunStore.FindSubAttempt(ctx, root, request.SubDAGRunId) @@ func (a *API) GetSubDAGRunLog(ctx context.Context, request api.GetSubDAGRunLogRequestObject) (api.GetSubDAGRunLogResponseObject, error) { root := exec.NewDAGRunRef(request.Name, request.DagRunId) + if err := a.requireDAGRunVisible(ctx, root); err != nil { + return nil, err + } dagStatus, err := a.dagRunMgr.FindSubDAGRunStatus(ctx, root, request.SubDAGRunId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/dagruns.go` around lines 1903 - 2200, The sub-DAG endpoints (GetSubDAGRunDetails, GetSubDAGRunSpec, GetSubDAGRunLog, DownloadSubDAGRunLog, GetSubDAGRunArtifacts, GetSubDAGRunArtifactPreview, DownloadSubDAGRunArtifact, GetSubDAGRunStepLog, DownloadSubDAGRunStepLog) currently read sub-run data without enforcing workspace visibility; before any call that reads sub-run status/spec/logs/artifacts (e.g. before a.dagRunMgr.FindSubDAGRunStatus, a.dagRunStore.FindSubAttempt, getSubDAGRunArtifactStatus, openArtifactFile, file reads, or attempt.ReadDAG) call the existing workspace check helpers (requireDAGRunVisible for root-level checks or an attempt-based visibility check like requireAttemptWorkspaceVisible) and return the appropriate 403/NotFound response on failure; ensure you perform this gate as soon as you construct the root exec.NewDAGRunRef and before any disk or store access in each of the functions named above so sub-DAG data is only returned when workspace visibility is satisfied.
2680-2822:⚠️ Potential issue | 🟠 MajorRe-check execute permission for
useCurrentDAGFilereschedules.The endpoint authorizes the source run, then
rescheduleDAGRunmay load the current DAG file and enqueue that version. If the DAG file’s workspace changed since the original run, a user with access only to the old workspace can reschedule into a workspace they cannot execute.🔒 Proposed fix
if opts.useCurrentDAGFile { dag, err = a.loadCurrentRescheduleDAG(ctx, storedSourceFile, nameOverride) if err != nil { return rescheduleDAGRunResult{}, err } + if err := a.requireExecuteForWorkspace(ctx, dagWorkspaceName(dag)); err != nil { + return rescheduleDAGRunResult{}, err + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/dagruns.go` around lines 2680 - 2822, The current reschedule flow authorizes the original DAG run but does not re-check execute permissions when opts.useCurrentDAGFile loads the current DAG file, allowing privilege escalation into another workspace; after loading the current DAG in rescheduleDAGRun (the branch that calls loadCurrentRescheduleDAG) you must enforce execute permission on that DAG/workspace before proceeding (i.e., call the appropriate permission check similar to requireDAGRunExecute — e.g., a.requireDAGExecute or an equivalent that accepts the loaded dag or its workspace/DAG reference) before calling ensureDAGRunIDUnique or enqueueDAGRun; place this check right after the loadCurrentRescheduleDAG return and return an authorization error if it fails.
1321-1390:⚠️ Potential issue | 🔴 CriticalAdd workspace visibility checks before returning step messages.
Both step-message endpoints read node status and persisted messages without checking whether the caller can see the root/sub DAG run workspace.
🔒 Proposed fix
func (a *API) GetDAGRunStepMessages(ctx context.Context, request api.GetDAGRunStepMessagesRequestObject) (api.GetDAGRunStepMessagesResponseObject, error) { ref := exec.NewDAGRunRef(request.Name, request.DagRunId) dagStatus, err := a.dagRunMgr.GetSavedStatus(ctx, ref) if err != nil { @@ Message: fmt.Sprintf("dag-run ID %s not found for DAG %s", request.DagRunId, request.Name), }, nil } + if err := a.requireWorkspaceVisible(ctx, statusWorkspaceName(dagStatus)); err != nil { + return nil, err + } node, err := dagStatus.NodeByName(request.StepName) @@ func (a *API) GetSubDAGRunStepMessages(ctx context.Context, request api.GetSubDAGRunStepMessagesRequestObject) (api.GetSubDAGRunStepMessagesResponseObject, error) { rootRef := exec.NewDAGRunRef(request.Name, request.DagRunId) + if err := a.requireDAGRunVisible(ctx, rootRef); err != nil { + return nil, err + } dagStatus, err := a.dagRunMgr.FindSubDAGRunStatus(ctx, rootRef, request.SubDAGRunId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/dagruns.go` around lines 1321 - 1390, Both endpoints return step messages without verifying the caller can see the DAG-run's workspace; add an explicit workspace visibility check after obtaining dagStatus (in GetDAGRunStepMessages and GetSubDAGRunStepMessages) and before accessing node/attempt/ReadStepMessages. Call your existing workspace/authorization helper (e.g. a.workspaceMgr or a.authz method) to ensure the caller can view dagStatus's workspace ID (use the DAG status' WorkspaceID or equivalent field), and if the check fails return the appropriate not-found/forbidden JSON response instead of proceeding to node.Lookup or attempt.ReadStepMessages.
🧹 Nitpick comments (7)
internal/service/frontend/api/v1/dagruns_edit_retry.go (1)
66-83: Workspace authz ordering — minor consideration.The new
requireExecuteForWorkspace(dagWorkspaceName(plan.editedDAG))check correctly re-authorizes against the edited spec's workspace (in case the submitted spec moves the DAG to a different workspace than the source run). Two minor notes:
buildEditRetryPlanperforms non-trivial work (reading DAG snapshot, resolving runtime params, loading the inline edited spec) before this workspace check. SincerequireDAGRunExecuteat lines 66/124 already gates the source DAG-run's workspace, this is acceptable, but be aware that error messages fromloadEditedRetryDAG/spec validation can still surface to a caller who would ultimately be denied by the edited-workspace check. If that's considered an info-leak, hoist a cheap workspace check earlier once the edited spec's workspace is known.if plan != nilis defensive but unreachable-false here:buildEditRetryPlanonly returnsnilplan alongside a non-nil error, which is already handled. Consider dropping the guard for clarity, or keep it as defense-in-depth — either is fine.Also applies to: 124-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/dagruns_edit_retry.go` around lines 66 - 83, The code calls buildEditRetryPlan (which reads the DAG snapshot and loads the edited spec via loadEditedRetryDAG) before calling requireExecuteForWorkspace(dagWorkspaceName(plan.editedDAG)), which can leak validation/errors from the edited spec for a caller who would later be denied; either hoist a cheap workspace authorization earlier once you can determine the edited spec's workspace (obtain edited workspace from the request/parsed spec and call requireExecuteForWorkspace(...) before performing heavy work in buildEditRetryPlan), or simplify by removing the unreachable defensive guard "if plan != nil" around the requireExecuteForWorkspace call (since buildEditRetryPlan returns nil plan only with an error), choosing one approach and applying the same change in both occurrences that mirror lines 66–83 and 124–144; reference functions: requireDAGRunExecute, buildEditRetryPlan, loadEditedRetryDAG, requireExecuteForWorkspace, dagWorkspaceName, and plan.editedDAG.ui/src/features/cockpit/components/__tests__/TemplateSelector.test.tsx (1)
23-23: Consider assertingworkspaceScopeon/dagsas well.The test now verifies
workspaceScope: WorkspaceScope.accessibleon the/dags/labelsquery (good). SinceappBarValue.workspaceSelectionisaccessible, the/dagsquery on line 95–101 should presumably also includeworkspaceScope. Tightening that assertion would guard against regressions where the/dagsquery silently stops propagating the scope.Also applies to: 106-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/cockpit/components/__tests__/TemplateSelector.test.tsx` at line 23, The test currently asserts workspaceScope on the `/dags/labels` request but not on the `/dags` request(s); update the TemplateSelector.test.tsx assertions so that any mocked request or expectation for the `/dags` endpoint also includes workspaceScope: WorkspaceScope.accessible (matching appBarValue.workspaceSelection). Locate the places where `/dags` is stubbed/checked (the two occurrences near the existing `/dags/labels` assertions) and add an expectation that the request payload/query contains workspaceScope: WorkspaceScope.accessible to prevent regressions where the scope stops being propagated.internal/persis/filedagrun/store_test.go (1)
849-970: Add a non-nil workspace-filter regression case.These updated call sites only verify the new parameter is optional. Please add fast-path and standard-path cases where
WorkspaceFilteracceptsworkspace=opsand rejectsworkspace=prodso the new behavior is covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/persis/filedagrun/store_test.go` around lines 849 - 970, Add tests covering non-nil WorkspaceFilter behavior: in the fast-path test (TestResolveStatus_FastPath) add two assertions using a workspace filter that accepts "workspace=ops" (should return non-nil status) and one that accepts "workspace=prod" (should return nil); similarly, in the standard-path test (TestResolveStatus_StandardPath) add matching cases where resolveStatus is called with a WorkspaceFilter that accepts "workspace=ops" and another that accepts "workspace=prod" and assert resolution and rejection respectively. Locate calls to store.resolveStatus and the test functions TestResolveStatus_FastPath and TestResolveStatus_StandardPath to insert these new checks, constructing the WorkspaceFilter using the same parsing utility used for other filters (e.g., core.ParseLabelFilter or the WorkspaceFilter type) so the parameter position passed to resolveStatus mirrors existing calls. Ensure both fast-path and standard-path variants exercise the new non-nil workspace-filter parameter.ui/src/features/cockpit/components/TemplateSelector.tsx (1)
33-54: Remove unusedselectedWorkspaceprop from the component signature and call sites.
selectedWorkspaceis declared in thePropsinterface and destructured at line 42, but is never used in the component body. The component derives workspace context entirely fromappBarContext.workspaceSelection. Remove the parameter from the interface and all three call sites (CockpitToolbar.tsxand the two test cases) to clean up the API surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/cockpit/components/TemplateSelector.tsx` around lines 33 - 54, The Props interface and TemplateSelector signature include an unused selectedWorkspace prop; remove selectedWorkspace from the Props type and from the TemplateSelector parameter destructuring (function TemplateSelector) and update all call sites (e.g., usages in CockpitToolbar.tsx and the two test files) to stop passing selectedWorkspace so the component API matches its actual usage of appBarContext.workspaceSelection.ui/src/pages/users/UserFormModal.tsx (1)
112-136: Use the generated API client for these user mutations.The new
workspaceAccesspayload is sent through hand-writtenfetchcalls, which bypasses generated request typing and shared API plumbing. Route these through the generated client withremoteNodein query params instead. As per coding guidelines,ui/src/**/*.{ts,tsx}: Frontend API types must be generated from OpenAPI spec viapnpm gen:apito maintain type safety with the backend; andui/**/*.{ts,tsx}: Useclient.GETandclient.POSTcalls with remoteNode in query parameters for API interactions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/users/UserFormModal.tsx` around lines 112 - 136, In UserFormModal replace the hand-written fetch calls inside the submit flow (the isEditing branch that PATCHes `${config.apiURL}/users/${user.id}?remoteNode=${remoteNode}` and the create branch that POSTs to `/users?remoteNode=${remoteNode}`) with the generated API client calls (use client.PATCH or client.POST as provided by the generated client), pass remoteNode as a query param, and send the typed payload (including workspaceAccess and password) directly rather than JSON.stringify and manual headers; update the submit handler in UserFormModal to call the appropriate generated method for updating (using user.id and payload) and creating (using payload + password), so you leverage generated request/response types and shared plumbing instead of raw fetch.internal/service/frontend/api/v1/apikeys.go (1)
103-107: Include workspace access in API key create audit details.Updates log
workspace_access, but creates omit the initial grants. Add it here so audit history captures the key’s permission boundary from creation.Audit detail addition
a.logAudit(ctx, audit.CategoryAPIKey, "api_key_create", map[string]any{ - "key_id": result.APIKey.ID, - "key_name": result.APIKey.Name, - "role": string(result.APIKey.Role), + "key_id": result.APIKey.ID, + "key_name": result.APIKey.Name, + "role": string(result.APIKey.Role), + "workspace_access": toAPIWorkspaceAccess(result.APIKey.WorkspaceAccess), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/apikeys.go` around lines 103 - 107, The audit entry for API key creation is missing the initial workspace grants; update the a.logAudit call in the API key create path (the call that uses audit.CategoryAPIKey and "api_key_create") to include a "workspace_access" field in the map with the initial grants (e.g. use result.WorkspaceAccess or the API key's workspace access field such as result.APIKey.WorkspaceAccess) so the audit record captures the permission boundary at creation.internal/auth/workspace_access.go (1)
59-68: Redundant slice copy inCloneWorkspaceAccess.
NormalizeWorkspaceAccessalready allocates a freshGrantsslice (line 46), so the additionalmake+copyhere is unnecessary work and obscures intent. You can return the normalized value directly.♻️ Proposed simplification
func CloneWorkspaceAccess(access *WorkspaceAccess) *WorkspaceAccess { normalized := NormalizeWorkspaceAccess(access) - grants := make([]WorkspaceGrant, len(normalized.Grants)) - copy(grants, normalized.Grants) - return &WorkspaceAccess{ - All: normalized.All, - Grants: grants, - } + return &normalized }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/workspace_access.go` around lines 59 - 68, The CloneWorkspaceAccess function performs an unnecessary make+copy of normalized.Grants because NormalizeWorkspaceAccess already returns a freshly allocated Grants slice; simplify CloneWorkspaceAccess by returning the result of NormalizeWorkspaceAccess directly (i.e., remove the intermediate make([]WorkspaceGrant, ...) and copy and just return the normalized value from CloneWorkspaceAccess), keeping the function signature and behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1122f52-9925-4d9f-8933-6bd9dcab9259
📒 Files selected for processing (66)
api/v1/api.gen.goapi/v1/api.yamlinternal/auth/apikey.gointernal/auth/user.gointernal/auth/workspace_access.gointernal/auth/workspace_access_test.gointernal/core/exec/context.gointernal/core/exec/dag.gointernal/core/exec/dagrun.gointernal/core/exec/workspace.gointernal/persis/filedag/store.gointernal/persis/filedag/store_test.gointernal/persis/filedagrun/pagination.gointernal/persis/filedagrun/store.gointernal/persis/filedagrun/store_test.gointernal/service/auth/service.gointernal/service/frontend/api/v1/api.gointernal/service/frontend/api/v1/apikeys.gointernal/service/frontend/api/v1/auth.gointernal/service/frontend/api/v1/dagruns.gointernal/service/frontend/api/v1/dagruns_edit_retry.gointernal/service/frontend/api/v1/dags.gointernal/service/frontend/api/v1/docs.gointernal/service/frontend/api/v1/queues.gointernal/service/frontend/api/v1/search.gointernal/service/frontend/api/v1/users.gointernal/service/frontend/api/v1/workspace_access.gointernal/service/frontend/api/v1/workspace_access_test.gointernal/service/frontend/api/v1/workspaces.gointernal/service/frontend/auth/middleware.gointernal/service/oidcprovision/service.goui/src/App.tsxui/src/api/v1/schema.tsui/src/components/WorkspaceAccessEditor.tsxui/src/contexts/AppBarContext.tsui/src/contexts/AuthContext.tsxui/src/features/cockpit/components/DateKanbanList.tsxui/src/features/cockpit/components/TemplateSelector.tsxui/src/features/cockpit/components/WorkspaceSelector.tsxui/src/features/cockpit/components/__tests__/TemplateSelector.test.tsxui/src/features/cockpit/hooks/useCockpitState.tsui/src/features/cockpit/hooks/useDateKanbanData.tsui/src/features/dags/components/common/CreateDAGModal.tsxui/src/features/search/components/SearchResult.tsxui/src/features/search/components/__tests__/SearchResult.test.tsxui/src/hooks/useDAGRunsListSSE.tsui/src/hooks/useDAGsListSSE.tsui/src/hooks/useDocSSE.tsui/src/hooks/useDocTreeSSE.tsui/src/lib/workspace.tsui/src/lib/workspaceAccess.tsui/src/menu.tsxui/src/pages/__tests__/index.test.tsxui/src/pages/api-keys/APIKeyFormModal.tsxui/src/pages/cockpit/index.tsxui/src/pages/dag-runs/index.tsxui/src/pages/dags/dag/index.tsxui/src/pages/dags/index.tsxui/src/pages/design/buildWorkflowDesignPrompt.tsui/src/pages/design/index.tsxui/src/pages/docs/components/DocEditor.tsxui/src/pages/docs/components/DocTreeSidebar.tsxui/src/pages/docs/index.tsxui/src/pages/index.tsxui/src/pages/search/index.tsxui/src/pages/users/UserFormModal.tsx
💤 Files with no reviewable changes (1)
- internal/service/frontend/api/v1/api.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
internal/persis/fileserviceregistry/registry.go (1)
177-188:⚠️ Potential issue | 🟡 MinorOnly remove the instance file after a confirmed heartbeat stop.
Line 188 still runs after the timeout case on Line 180, when
reg.wg.Wait()has not completed. That means the heartbeat may still be alive and can race with deletion/recreation, contradicting the new shutdown ordering guarantee.🐛 Proposed fix
+ stopped := false select { case <-done: // Clean shutdown + stopped = true case <-time.After(5 * time.Second): // Force shutdown after timeout logger.Warn(ctx, "Timeout waiting for registry shutdown", tag.Service(serviceName)) } + if !stopped { + continue + } + // Remove instance file after heartbeat stops so it cannot recreate the // file during shutdown on Windows. if err := removeInstanceFile(reg.fileName); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/persis/fileserviceregistry/registry.go` around lines 177 - 188, The instance file is being removed even if the heartbeat goroutine may still be running when the select times out; ensure removal happens only after the heartbeat goroutine has actually exited by waiting on the registry waitgroup. After the select (which logs the timeout), call reg.wg.Wait() (or otherwise ensure reg.wg has completed) before calling removeInstanceFile(reg.fileName), so that removeInstanceFile only runs once the heartbeat goroutine has fully stopped; reference the done channel, reg.wg.Wait, removeInstanceFile and reg.fileName to locate and update the code.ui/src/api/v1/schema.ts (1)
6082-6086:⚠️ Potential issue | 🟠 MajorAdd explicit mutation scope to DAG creation.
createNewDAGstill only acceptsremoteNode, so the new explicit workspace contract cannot represent “create in no workspace” vs “create in this named workspace.” AddworkspaceScope?: WorkspaceMutationScopeplus legacy-compatibleworkspace?: Workspacehere, matching the document mutation endpoints.Please update the OpenAPI source and regenerate this file instead of editing it directly. As per coding guidelines, Frontend API types must be generated from OpenAPI spec via
pnpm gen:apito maintain type safety with the backend.Suggested generated-shape change
/** `@description` name of the remote node */ remoteNode?: components["parameters"]["RemoteNode"]; + /** `@description` Explicit mutable workspace scope: no-workspace data or one named workspace */ + workspaceScope?: components["parameters"]["WorkspaceMutationScope"]; + /** `@description` workspace name used when workspaceScope=workspace, or legacy selected workspace scope when workspaceScope is omitted */ + workspace?: components["parameters"]["Workspace"];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/api/v1/schema.ts` around lines 6082 - 6086, The createNewDAG request shape currently only accepts remoteNode; update the OpenAPI source so the parameters.query for createNewDAG includes workspaceScope?: WorkspaceMutationScope and legacy-compatible workspace?: Workspace (in addition to remoteNode) to allow explicit "no workspace" vs "named workspace" semantics, then regenerate the frontend types (do not hand-edit ui/src/api/v1/schema.ts) by running the prescribed generator (pnpm gen:api) so the generated type for parameters.query reflects WorkspaceMutationScope and Workspace alongside remoteNode.api/v1/api.yaml (1)
9696-9702:⚠️ Potential issue | 🟠 MajorRequire an explicit workspace access policy on identity creation.
workspaceAccessis optional on create requests but required on returnedUser/APIKeyobjects. That leaves generated clients free to create identities without an explicit workspace policy, making the server default security-sensitive and unclear. Add it torequired, or encode/document a safe backward-compatible default.Contract fix if explicit policy is required
required: - username - password - role + - workspaceAccessrequired: - name - role + - workspaceAccessAlso applies to: 9858-9863
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 9696 - 9702, The create-request schemas for identities omit workspaceAccess from their required arrays while returned User/APIKey objects include it; update the OpenAPI component schemas used for identity creation (the create request schemas that contain the workspaceAccess property) to add "workspaceAccess" to their required list and ensure the property still references "#/components/schemas/WorkspaceAccess"; also update any examples/docs for those schemas to include a valid WorkspaceAccess value (or alternatively document/encode a safe backward-compatible default if you intend not to require it).internal/service/frontend/api/v1/dags.go (1)
376-394:⚠️ Potential issue | 🔴 CriticalDon't ignore
GetDetailserror before gating history visibility.
dag, _ := a.dagStore.GetDetails(...)discards the error and then callsrequireWorkspaceVisible(ctx, dagWorkspaceName(dag)). When loading fails (missing file, I/O error, or any path wheredag == nil),dagWorkspaceName(nil)returns"", which:
- lets callers with no-workspace / admin access sail past the check and receive a 200 with empty history for a non-existent DAG (instead of the 404 every other handler returns), and
- on non-ENOENT failures, can let a user who has access to the "no-workspace" scope see history for a DAG that actually belongs to a workspace they cannot access (the fileName is still used by
ListRecentStatus/readHistoryDatadownstream).Other handlers in this file (
GetDAGSpec,GetDAGDAGRunDetails,getDAGDetailsData) treatGetDetailserrors explicitly — please do the same here so visibility gating always runs against a resolved DAG.🔒 Proposed fix
func (a *API) GetDAGDAGRunHistory(ctx context.Context, request api.GetDAGDAGRunHistoryRequestObject) (api.GetDAGDAGRunHistoryResponseObject, error) { - dag, _ := a.dagStore.GetDetails(ctx, request.FileName, spec.WithAllowBuildErrors()) - if err := a.requireWorkspaceVisible(ctx, dagWorkspaceName(dag)); err != nil { + dag, err := a.dagStore.GetDetails(ctx, request.FileName, spec.WithAllowBuildErrors()) + if err != nil || dag == nil { + return nil, &Error{ + HTTPStatus: http.StatusNotFound, + Code: api.ErrorCodeNotFound, + Message: fmt.Sprintf("DAG %s not found", request.FileName), + } + } + if err := a.requireWorkspaceVisible(ctx, dagWorkspaceName(dag)); err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/dags.go` around lines 376 - 394, GetDAGDAGRunHistory currently ignores the error returned from a.dagStore.GetDetails which allows calling requireWorkspaceVisible with a nil dag; change the logic to check the error from a.dagStore.GetDetails immediately (same pattern as GetDAGSpec/GetDAGDAGRunDetails/getDAGDetailsData): if err != nil return the appropriate error response (propagate the error/translate to a 404 or other API error as the other handlers do) before computing dagWorkspaceName(dag) or calling requireWorkspaceVisible, then continue to call a.resolveDAGName, a.dagRunMgr.ListRecentStatus and readHistoryData only when dag is non-nil and GetDetails succeeded.internal/service/frontend/api/v1/search.go (1)
213-238:⚠️ Potential issue | 🟡 MinorPost-query visibility filter can return pages much smaller than the requested
Limit.
docStore.SearchCursoris called withLimitand returns up to that many raw items; the in-place filter at lines 228-234 then drops all items the caller cannot see. When most matching docs live outside the caller's accessible set (e.g. the global search page on a multi-workspace instance where the caller only has access to a few), the response may contain only a handful of items even thoughHasMoreandNextCursorare set. Clients will need many round-trips to fill a page, and users will see sparse results.If the underlying store doesn't support workspace-aware filtering yet, consider at minimum over-fetching (e.g. request
Limit * Nand stop onceLimitvisible items are accumulated while still returning the store's next cursor) or looping the cursor server-side until the page is full or the cursor is exhausted. Same concern applies toSearchDagMatchesstyle pagination if its underlying store doesn't push the workspace filter down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/search.go` around lines 213 - 238, The current post-query filtering after a.docStore.SearchCursor can yield far fewer visible items than the requested Limit because result.Items is trimmed via visibility.visible, so change SearchCursor handling in the search handler to ensure the returned page contains up to the requested visible Limit: call a.docStore.SearchCursor with an increased fetch window (e.g. Limit * N) or loop repeatedly using the returned cursor to accumulate visible items into result.Items until either you have populated the requested visible Limit or the store cursor is exhausted, preserving the store's HasMore/NextCursor semantics for the response; ensure the logic references docStore.SearchCursor, the request.Params.Limit (normalized by normalizeSearchLimit/searchDefaultLimit), visibility.visible, and returns via toDocSearchFeedResponse with adjusted result.Items and the original/updated cursor/HasMore values.
♻️ Duplicate comments (1)
api/v1/api.yaml (1)
1659-1662:⚠️ Potential issue | 🟠 MajorDisallow aggregate scope for single-resource lookups.
These endpoints still accept
workspaceScope=accessiblewhile targeting only onefileName/path. With duplicate resources across workspaces, the request is ambiguous andSearchMatchesResponsecannot identify which workspace each snippet came from. Use a non-aggregate read scope (none | workspace) here, or requireworkspaceScope=workspaceplusworkspacefor follow-up calls.Contract-level fix option
- - $ref: "#/components/parameters/WorkspaceScope" + - $ref: "#/components/parameters/WorkspaceLookupScope" - $ref: "#/components/parameters/Workspace"+ WorkspaceLookupScope: + name: workspaceScope + in: query + description: "Explicit target scope for single-resource reads: no-workspace data or one named workspace" + required: false + schema: + $ref: "#/components/schemas/WorkspaceLookupScope" + WorkspaceMutationScope: name: workspaceScope in: query description: "Explicit mutable workspace scope: no-workspace data or one named workspace"+ WorkspaceLookupScope: + type: string + description: "Workspace scope selector for single-resource read APIs" + enum: + - none + - workspace + WorkspaceMutationScope: type: string description: "Workspace scope selector for mutation APIs" enum: - none - workspaceAlso applies to: 1711-1719, 6451-6459
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 1659 - 1662, The endpoints referencing the parameter set ($ref: "#/components/parameters/DAGFileName", "#/components/parameters/RemoteNode", "#/components/parameters/WorkspaceScope", "#/components/parameters/Workspace") are wrongly allowing an aggregate workspaceScope for single-resource lookups (fileName/path) which makes results ambiguous; update the API spec for those operations to disallow the aggregate scope by replacing or constraining the WorkspaceScope parameter to a non-aggregate read scope (e.g., only allow "none" | "workspace") or require workspaceScope="workspace" together with the Workspace parameter for these single-resource endpoints (apply the same change for the other occurrences referenced at lines ~1711-1719 and ~6451-6459).
🧹 Nitpick comments (2)
internal/runtime/manager_test.go (1)
239-248: Assert the node-level error field here.Line 243 checks
latest.Error, but stale-local-run repair records the stale-process message onlatest.Nodes[0].Errorin this same test file’s stale-repair case. Add the node-level assertion so this test verifies the exact field that would expose an accidental repair.Test assertion tightening
require.Equal(t, core.Running, latest.Status) require.Equal(t, core.NodeRunning, latest.Nodes[0].Status) require.Empty(t, latest.Error) + require.Empty(t, latest.Nodes[0].Error) persisted, err := att.ReadStatus(ctx) require.NoError(t, err) require.Equal(t, core.Running, persisted.Status) require.Equal(t, core.NodeRunning, persisted.Nodes[0].Status) + require.Empty(t, persisted.Nodes[0].Error)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runtime/manager_test.go` around lines 239 - 248, The test asserts that latest.Error is empty but the stale-local-run repair writes its message to the node-level field; update the assertions after calling th.DAGRunMgr.GetLatestStatus and att.ReadStatus to also check latest.Nodes[0].Error and persisted.Nodes[0].Error (or the appropriate node index) is empty (or equals the expected value) so the test verifies the node-level error field that stale-repair would set; locate the checks around latest, persisted and their Nodes slices in manager_test.go and add the node-level assertions referencing latest.Nodes[0].Error and persisted.Nodes[0].Error.ui/src/pages/dags/index.tsx (1)
82-120: Tab-closing effect appears redundant withTabProviderkeyremount.In the outer
DAGscomponent (lines 447-456),dagTabStorageKeyincorporatesworkspaceScopeKeyand is passed as bothkeyandstorageKeytoTabProvider. When the workspace selection changes,keychange causesTabProviderto unmount/remount with fresh state, sotabsis already empty by the timeDAGsContentre-renders. The effect at lines 112-120 then sees the ref mismatch on the new mount... exceptpreviousWorkspaceScopeKeyRefis (re)initialized to the currentworkspaceScopeKeyon mount, so the early return fires and no tabs are iterated anyway.Net effect: the loop body effectively never runs. If this is intentional defensive code, fine; otherwise consider removing the effect and ref to reduce noise, since the
key-based remount is the actual mechanism clearing tabs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/dags/index.tsx` around lines 82 - 120, The effect using previousWorkspaceScopeKeyRef to close tabs is redundant because DAGs uses dagTabStorageKey (which includes workspaceScopeKey) as the key/storageKey for TabProvider causing a remount that clears tabs; remove the redundant logic by deleting the previousWorkspaceScopeKeyRef declaration and the React.useEffect block that compares previousWorkspaceScopeKeyRef.current to workspaceScopeKey and iterates over tabs calling closeTab (also remove any now-unused imports or variables if flagged by the compiler).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/src/api/v1/schema.ts`:
- Around line 7038-7041: The workspaceScope field on the generated API types
(workspaceScope?: components["parameters"]["WorkspaceScope"] and workspace?:
components["parameters"]["Workspace"]) allows aggregate values like
"accessible", which is unsafe for path-addressed reads such as getDoc and
searchDocMatches; update the OpenAPI spec to restrict the WorkspaceScope used by
those endpoints to non-aggregate types (e.g., disallow "accessible" or use a
dedicated enum for single-document reads), then regenerate the TypeScript types
(run pnpm gen:api) so the generated schema.ts reflects the tighter type and
prevents aggregate scopes for getDoc and searchDocMatches.
In `@ui/src/contexts/AuthContext.tsx`:
- Around line 173-201: The guard in useCanWrite and useCanExecute incorrectly
treats an undefined appBarContext.workspaceSelection as permissive; change the
check so undefined selection is treated as aggregate (fail-closed) by returning
false when workspaceSelection is missing or its scope equals
WorkspaceScope.accessible (i.e., replace the single condition
appBarContext.workspaceSelection?.scope === WorkspaceScope.accessible with a
check like !appBarContext.workspaceSelection ||
appBarContext.workspaceSelection.scope === WorkspaceScope.accessible), keeping
the rest of the logic (useAuth user check, config.authMode handling, and calls
to effectiveWorkspaceRole and roleAtLeast) intact.
---
Outside diff comments:
In `@api/v1/api.yaml`:
- Around line 9696-9702: The create-request schemas for identities omit
workspaceAccess from their required arrays while returned User/APIKey objects
include it; update the OpenAPI component schemas used for identity creation (the
create request schemas that contain the workspaceAccess property) to add
"workspaceAccess" to their required list and ensure the property still
references "#/components/schemas/WorkspaceAccess"; also update any examples/docs
for those schemas to include a valid WorkspaceAccess value (or alternatively
document/encode a safe backward-compatible default if you intend not to require
it).
In `@internal/persis/fileserviceregistry/registry.go`:
- Around line 177-188: The instance file is being removed even if the heartbeat
goroutine may still be running when the select times out; ensure removal happens
only after the heartbeat goroutine has actually exited by waiting on the
registry waitgroup. After the select (which logs the timeout), call
reg.wg.Wait() (or otherwise ensure reg.wg has completed) before calling
removeInstanceFile(reg.fileName), so that removeInstanceFile only runs once the
heartbeat goroutine has fully stopped; reference the done channel, reg.wg.Wait,
removeInstanceFile and reg.fileName to locate and update the code.
In `@internal/service/frontend/api/v1/dags.go`:
- Around line 376-394: GetDAGDAGRunHistory currently ignores the error returned
from a.dagStore.GetDetails which allows calling requireWorkspaceVisible with a
nil dag; change the logic to check the error from a.dagStore.GetDetails
immediately (same pattern as GetDAGSpec/GetDAGDAGRunDetails/getDAGDetailsData):
if err != nil return the appropriate error response (propagate the
error/translate to a 404 or other API error as the other handlers do) before
computing dagWorkspaceName(dag) or calling requireWorkspaceVisible, then
continue to call a.resolveDAGName, a.dagRunMgr.ListRecentStatus and
readHistoryData only when dag is non-nil and GetDetails succeeded.
In `@internal/service/frontend/api/v1/search.go`:
- Around line 213-238: The current post-query filtering after
a.docStore.SearchCursor can yield far fewer visible items than the requested
Limit because result.Items is trimmed via visibility.visible, so change
SearchCursor handling in the search handler to ensure the returned page contains
up to the requested visible Limit: call a.docStore.SearchCursor with an
increased fetch window (e.g. Limit * N) or loop repeatedly using the returned
cursor to accumulate visible items into result.Items until either you have
populated the requested visible Limit or the store cursor is exhausted,
preserving the store's HasMore/NextCursor semantics for the response; ensure the
logic references docStore.SearchCursor, the request.Params.Limit (normalized by
normalizeSearchLimit/searchDefaultLimit), visibility.visible, and returns via
toDocSearchFeedResponse with adjusted result.Items and the original/updated
cursor/HasMore values.
In `@ui/src/api/v1/schema.ts`:
- Around line 6082-6086: The createNewDAG request shape currently only accepts
remoteNode; update the OpenAPI source so the parameters.query for createNewDAG
includes workspaceScope?: WorkspaceMutationScope and legacy-compatible
workspace?: Workspace (in addition to remoteNode) to allow explicit "no
workspace" vs "named workspace" semantics, then regenerate the frontend types
(do not hand-edit ui/src/api/v1/schema.ts) by running the prescribed generator
(pnpm gen:api) so the generated type for parameters.query reflects
WorkspaceMutationScope and Workspace alongside remoteNode.
---
Duplicate comments:
In `@api/v1/api.yaml`:
- Around line 1659-1662: The endpoints referencing the parameter set ($ref:
"#/components/parameters/DAGFileName", "#/components/parameters/RemoteNode",
"#/components/parameters/WorkspaceScope", "#/components/parameters/Workspace")
are wrongly allowing an aggregate workspaceScope for single-resource lookups
(fileName/path) which makes results ambiguous; update the API spec for those
operations to disallow the aggregate scope by replacing or constraining the
WorkspaceScope parameter to a non-aggregate read scope (e.g., only allow "none"
| "workspace") or require workspaceScope="workspace" together with the Workspace
parameter for these single-resource endpoints (apply the same change for the
other occurrences referenced at lines ~1711-1719 and ~6451-6459).
---
Nitpick comments:
In `@internal/runtime/manager_test.go`:
- Around line 239-248: The test asserts that latest.Error is empty but the
stale-local-run repair writes its message to the node-level field; update the
assertions after calling th.DAGRunMgr.GetLatestStatus and att.ReadStatus to also
check latest.Nodes[0].Error and persisted.Nodes[0].Error (or the appropriate
node index) is empty (or equals the expected value) so the test verifies the
node-level error field that stale-repair would set; locate the checks around
latest, persisted and their Nodes slices in manager_test.go and add the
node-level assertions referencing latest.Nodes[0].Error and
persisted.Nodes[0].Error.
In `@ui/src/pages/dags/index.tsx`:
- Around line 82-120: The effect using previousWorkspaceScopeKeyRef to close
tabs is redundant because DAGs uses dagTabStorageKey (which includes
workspaceScopeKey) as the key/storageKey for TabProvider causing a remount that
clears tabs; remove the redundant logic by deleting the
previousWorkspaceScopeKeyRef declaration and the React.useEffect block that
compares previousWorkspaceScopeKeyRef.current to workspaceScopeKey and iterates
over tabs calling closeTab (also remove any now-unused imports or variables if
flagged by the compiler).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc9ad8ac-b903-4fcf-afbb-080cd7cd1c1a
📒 Files selected for processing (30)
api/v1/api.gen.goapi/v1/api.yamlinternal/core/exec/dag.gointernal/core/exec/workspace.gointernal/core/exec/workspace_test.gointernal/intg/distr/execution_test.gointernal/persis/filedag/store.gointernal/persis/filedag/store_test.gointernal/persis/fileserviceregistry/registry.gointernal/persis/fileserviceregistry/types.gointernal/runtime/manager.gointernal/runtime/manager_test.gointernal/service/frontend/api/v1/dags.gointernal/service/frontend/api/v1/docs.gointernal/service/frontend/api/v1/queues.gointernal/service/frontend/api/v1/search.gointernal/service/frontend/api/v1/transformer.gointernal/service/frontend/api/v1/workspace_access.gointernal/service/frontend/api/v1/workspaces.goui/src/api/v1/schema.tsui/src/components/WorkspaceAccessEditor.tsxui/src/contexts/AppBarContext.tsui/src/contexts/AuthContext.tsxui/src/lib/workspace.tsui/src/lib/workspaceAccess.tsui/src/menu.tsxui/src/pages/dags/index.tsxui/src/pages/docs/components/DocEditor.tsxui/src/pages/docs/index.tsxui/src/pages/users/UserFormModal.tsx
✅ Files skipped from review due to trivial changes (2)
- internal/intg/distr/execution_test.go
- ui/src/pages/docs/index.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- ui/src/menu.tsx
- ui/src/pages/users/UserFormModal.tsx
- internal/persis/filedag/store.go
- ui/src/components/WorkspaceAccessEditor.tsx
- internal/service/frontend/api/v1/queues.go
- internal/service/frontend/api/v1/docs.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
api/v1/api.yaml (1)
1661-1662:⚠️ Potential issue | 🟠 MajorConstrain DAG match lookups to a single workspace.
/search/dags/{fileName}/matchesis keyed only byfileName; allowingworkspaceScope=accessiblecan make duplicate file names across accessible workspaces ambiguous. Mirror the docs match endpoint and use the narrower scope here.Proposed contract fix
- - $ref: "#/components/parameters/WorkspaceScope" + - $ref: "#/components/parameters/WorkspaceMutationScope" - $ref: "#/components/parameters/Workspace"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 1661 - 1662, The DAG matches endpoint /search/dags/{fileName}/matches currently includes both parameters $ref: "#/components/parameters/WorkspaceScope" and $ref: "#/components/parameters/Workspace", which allows ambiguous lookups across multiple workspaces; remove the WorkspaceScope parameter and keep only $ref: "#/components/parameters/Workspace" so the endpoint is constrained to a single workspace (mirror the docs match endpoint behavior) to avoid duplicate fileName ambiguity.ui/src/api/v1/schema.ts (1)
6973-6976:⚠️ Potential issue | 🟠 MajorUse a non-aggregate scope for DAG match lookups.
searchDagMatchestargets onefileName, butWorkspaceScopestill allowsaccessible. If two workspaces contain the same DAG file name, aggregate scope can fetch or merge snippets from the wrong DAG. Use a non-aggregate scope here, or explicitly rejectaccessiblein the OpenAPI contract and generated type.Suggested generated-shape change
- /** `@description` Explicit workspace scope: accessible data, no-workspace data, or one named workspace */ - workspaceScope?: components["parameters"]["WorkspaceScope"]; + /** `@description` Explicit DAG match scope: no-workspace data or one named workspace */ + workspaceScope?: components["parameters"]["WorkspaceMutationScope"];Please update the OpenAPI source and regenerate this file instead of editing
schema.tsdirectly. As per coding guidelines, Frontend API types must be generated from OpenAPI spec viapnpm gen:apito maintain type safety with the backend.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/api/v1/schema.ts` around lines 6973 - 6976, The OpenAPI-generated types for searchDagMatches currently allow an aggregate WorkspaceScope (accessible), which can mix DAGs with the same fileName across workspaces; update the OpenAPI spec to restrict the searchDagMatches operation's workspaceScope parameter to a non-aggregate scope (e.g., exclude or disallow "accessible") or explicitly require a single-workspace scope, leaving the workspace parameter as the selected workspace, then re-run the generator (pnpm gen:api) so the generated schema.ts (affecting workspaceScope and workspace) reflects the non-aggregate constraint; do not edit schema.ts directly.
🧹 Nitpick comments (1)
internal/service/frontend/api/v1/docs.go (1)
776-791: Minor: avoid the redundant initialdocPointReadScopeForParamscall.When
hasQueryis true, the result of the unconditional call on line 777 is immediately discarded and replaced on line 787. This still performs a workspace list lookup vianoWorkspaceDocVisibilityfor nothing. Consider structuring as an if/else to only compute visibility once.♻️ Proposed refactor
path, queryString, hasQuery := strings.Cut(docID, "?") - workspaceName, visibility, err := a.docPointReadScopeForParams(ctx, nil, nil) - if err != nil { - return nil, err - } + var ( + workspaceName string + visibility docWorkspaceVisibility + err error + ) if hasQuery { params, err := url.ParseQuery(queryString) if err != nil { return nil, err } scopeParam, workspaceParam := workspaceMutationScopeParamsFromValues(params) workspaceName, visibility, err = a.docPointReadScopeForParams(ctx, scopeParam, workspaceParam) if err != nil { return nil, err } + } else { + workspaceName, visibility, err = a.docPointReadScopeForParams(ctx, nil, nil) + if err != nil { + return nil, err + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/docs.go` around lines 776 - 791, The current code calls a.docPointReadScopeForParams(ctx, nil, nil) unconditionally and then overwrites its result if hasQuery is true; remove the redundant initial call and instead branch: if hasQuery, parse queryString, call workspaceMutationScopeParamsFromValues(params) and then call a.docPointReadScopeForParams(ctx, scopeParam, workspaceParam); else call a.docPointReadScopeForParams(ctx, nil, nil). Use the existing path, queryString, hasQuery variables and preserve error handling for url.ParseQuery and both docPointReadScopeForParams calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1/api.yaml`:
- Around line 6999-7006: The API accepts different workspace-name formats across
endpoints: the query parameter "Workspace" and selector/grant paths use pattern
"^[A-Za-z0-9_-]+$", but create/update workspace request schemas allow any
string; unify them by introducing a single reusable workspace-name schema (e.g.,
components/schemas/WorkspaceName) that sets type: string and pattern:
"^[A-Za-z0-9_-]+$", then replace inline definitions by $ref to that schema for
the "Workspace" parameter, all createWorkspace/updateWorkspace request body
schemas, and any WorkspaceAccess/selector/grant path schemas so every place uses
the same validated pattern.
---
Duplicate comments:
In `@api/v1/api.yaml`:
- Around line 1661-1662: The DAG matches endpoint
/search/dags/{fileName}/matches currently includes both parameters $ref:
"#/components/parameters/WorkspaceScope" and $ref:
"#/components/parameters/Workspace", which allows ambiguous lookups across
multiple workspaces; remove the WorkspaceScope parameter and keep only $ref:
"#/components/parameters/Workspace" so the endpoint is constrained to a single
workspace (mirror the docs match endpoint behavior) to avoid duplicate fileName
ambiguity.
In `@ui/src/api/v1/schema.ts`:
- Around line 6973-6976: The OpenAPI-generated types for searchDagMatches
currently allow an aggregate WorkspaceScope (accessible), which can mix DAGs
with the same fileName across workspaces; update the OpenAPI spec to restrict
the searchDagMatches operation's workspaceScope parameter to a non-aggregate
scope (e.g., exclude or disallow "accessible") or explicitly require a
single-workspace scope, leaving the workspace parameter as the selected
workspace, then re-run the generator (pnpm gen:api) so the generated schema.ts
(affecting workspaceScope and workspace) reflects the non-aggregate constraint;
do not edit schema.ts directly.
---
Nitpick comments:
In `@internal/service/frontend/api/v1/docs.go`:
- Around line 776-791: The current code calls a.docPointReadScopeForParams(ctx,
nil, nil) unconditionally and then overwrites its result if hasQuery is true;
remove the redundant initial call and instead branch: if hasQuery, parse
queryString, call workspaceMutationScopeParamsFromValues(params) and then call
a.docPointReadScopeForParams(ctx, scopeParam, workspaceParam); else call
a.docPointReadScopeForParams(ctx, nil, nil). Use the existing path, queryString,
hasQuery variables and preserve error handling for url.ParseQuery and both
docPointReadScopeForParams calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d43bd40-c996-459b-a44c-a5f83ae729e8
📒 Files selected for processing (17)
api/v1/api.gen.goapi/v1/api.yamlinternal/runtime/manager.gointernal/runtime/manager_test.gointernal/service/frontend/api/v1/docs.gointernal/service/frontend/api/v1/search.gointernal/service/frontend/api/v1/workspace_access.goui/src/api/v1/schema.tsui/src/contexts/AuthContext.tsxui/src/contexts/DocTabContext.tsxui/src/features/search/components/SearchResult.tsxui/src/hooks/useDocSSE.tsui/src/lib/workspace.tsui/src/pages/docs/components/DocEditor.tsxui/src/pages/docs/components/DocTabEditorPanel.tsxui/src/pages/docs/components/DocTreeSidebar.tsxui/src/pages/docs/index.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/runtime/manager_test.go
- ui/src/pages/docs/components/DocEditor.tsx
- internal/runtime/manager.go
- ui/src/hooks/useDocSSE.ts
- ui/src/pages/docs/components/DocTreeSidebar.tsx
- ui/src/lib/workspace.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/service/frontend/api/v1/dags.go (2)
376-394:⚠️ Potential issue | 🟠 MajorVisibility check can pass on non-existent DAG when
GetDetailsfails.At line 377, the error from
a.dagStore.GetDetailsis dropped with_. If the DAG doesn't exist (or the store fails),dagisnil, sodagWorkspaceName(dag)returns""andrequireWorkspaceVisible(ctx, "")will succeed for any caller with no-workspace visibility. The handler then continues withresolveDAGName(ctx, request.FileName)(which falls back tofileName) and returns history data, potentially leaking existence/metadata for arbitrary file names to users who only have no-workspace access.For consistency with
getDAGDetailsData(lines 421-427) which properly propagates the load error, consider:🔒 Proposed fix
- dag, _ := a.dagStore.GetDetails(ctx, request.FileName, spec.WithAllowBuildErrors()) - if err := a.requireWorkspaceVisible(ctx, dagWorkspaceName(dag)); err != nil { - return nil, err - } + dag, err := a.dagStore.GetDetails(ctx, request.FileName, spec.WithAllowBuildErrors()) + if err != nil { + return nil, &Error{ + HTTPStatus: http.StatusNotFound, + Code: api.ErrorCodeNotFound, + Message: fmt.Sprintf("DAG %s not found", request.FileName), + } + } + if err := a.requireWorkspaceVisible(ctx, dagWorkspaceName(dag)); err != nil { + return nil, err + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/dags.go` around lines 376 - 394, GetDAGDAGRunHistory drops the error from a.dagStore.GetDetails causing nil dag to bypass visibility checks; change the call to capture the error (dag, err := a.dagStore.GetDetails(...)) and if err != nil return nil, err (same behavior as getDAGDetailsData), so you only call dagWorkspaceName(dag), requireWorkspaceVisible, resolveDAGName and readHistoryData when dag was successfully loaded; ensure you keep the subsequent uses of dag (dagWorkspaceName, readHistoryData) and dagName resolution unchanged.
734-810:⚠️ Potential issue | 🟠 Major
GetAllDAGTagsduplicatesGetAllDAGLabelswith multiple issues.
The bodies of
GetAllDAGLabels(734-771) andGetAllDAGTags(773-810) are byte-for-byte identical except for the response field name (LabelsvsTags). Both iteratedag.Labels.Strings()in the filtered path and calla.dagStore.LabelList(ctx)in the fallback. Extract a shared helper (e.g.,collectLabels(ctx, params)) to eliminate drift risk.Inside
GetAllDAGTags, the local variable is namedlabels(lines 786, 802) and error messages read"error getting labels"(lines 783, 804), which is confusing in a tags endpoint.
dag.Labels.Strings()anda.dagStore.LabelList(ctx)both include the internalworkspace=<name>label without filtering. This leaks workspace names through the tags/labels endpoints and pollutes the tag picker. Add filtering to exclude labels with keyWorkspaceLabelKeyin both the scoped and fallback paths.Replace
int(^uint(0)>>1)(lines 738, 777) with the named constantmath.MaxInt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/dags.go` around lines 734 - 810, GetAllDAGTags is a copy of GetAllDAGLabels and leaks workspace labels and uses confusing names/constants; extract a shared helper (e.g., collectLabels(ctx, workspaceScope, workspace) used by GetAllDAGLabels and GetAllDAGTags) that does the List path (using exec.NewPaginator(1, math.MaxInt)) and fallback to a.dagStore.LabelList(ctx), deduplicate values, and filter out any label whose key equals WorkspaceLabelKey before returning; update GetAllDAGTags to use the helper, rename local variables/errors from "labels" to "tags" where appropriate, and replace int(^uint(0)>>1) with math.MaxInt everywhere.api/v1/api.yaml (1)
1530-1538:⚠️ Potential issue | 🟠 MajorAdd workspace scoping to
/dags/search.This legacy DAG search endpoint still cannot request
accessible,none, or a named workspace, so it remains inconsistent with the explicit scope model used by the other DAG/search endpoints.Contract fix
parameters: - $ref: "#/components/parameters/RemoteNode" + - $ref: "#/components/parameters/WorkspaceScope" + - $ref: "#/components/parameters/Workspace" - name: "q" in: "query" required: trueAs per coding guidelines, Generate REST API server code from OpenAPI spec at
api/v1/api.yamlusingoapi-codegenviamake api.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 1530 - 1538, The /dags/search operation is missing workspace scoping like other DAG search endpoints; update its parameter block to include the workspace scoping parameter (same name and schema used by the other DAG/search endpoints — e.g., the "workspace" query parameter that accepts "accessible", "none", or a workspace name), ensure it appears alongside the existing RemoteNode and "q" parameters, and then regenerate server code with oapi-codegen by running make api; locate the /dags/search operation in api/v1/api.yaml and mirror the workspace parameter definition and allowed values used by the other DAG/search endpoints.
♻️ Duplicate comments (1)
api/v1/api.yaml (1)
775-776:⚠️ Potential issue | 🟠 MajorMake aggregate list results targetable by follow-up APIs.
These endpoints can now return resources across workspaces, but the path-based DAG/DAG-run follow-ups in this spec still generally cannot accept the returned workspace to disambiguate duplicates. Add
WorkspaceMutationScope+Workspaceto single-resource DAG/DAG-run detail/action endpoints, or constrain aggregate listing where follow-up targeting is unsupported.Example target-scope pattern
parameters: - $ref: "#/components/parameters/RemoteNode" + - $ref: "#/components/parameters/WorkspaceMutationScope" + - $ref: "#/components/parameters/Workspace" - $ref: "#/components/parameters/DAGFileName"parameters: - $ref: "#/components/parameters/RemoteNode" + - $ref: "#/components/parameters/WorkspaceMutationScope" + - $ref: "#/components/parameters/Workspace" - $ref: "#/components/parameters/DAGName" - $ref: "#/components/parameters/DAGRunId"As per coding guidelines, Generate REST API server code from OpenAPI spec at
api/v1/api.yamlusingoapi-codegenviamake api.Also applies to: 1820-1821, 2031-2032
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 775 - 776, The aggregate list endpoints currently return resources across workspaces but downstream DAG/DAG-run single-resource detail/action endpoints lack a way to accept the returned workspace; update the OpenAPI spec to add the WorkspaceMutationScope and Workspace parameters to all single-resource DAG and DAG-run detail/action paths (e.g., the operations referencing DAG, DAG-run, and their detail/action operations) so callers can disambiguate targets, or alternatively restrict aggregate listing endpoints to include Workspace/WorkspaceScope when follow-up targeting is unsupported; ensure you add the $ref entries for "#/components/parameters/WorkspaceMutationScope" and "#/components/parameters/Workspace" to those single-resource operations (and remove or adjust aggregate list params if you choose to constrain listings).
🧹 Nitpick comments (1)
ui/src/features/search/components/SearchResult.tsx (1)
175-254: Optional: extract the per-resultloadMoreclosure into a helper.The DAG and Doc branches both build a structurally identical item (
key,kind,title,link,initialMatches,initialHasMoreMatches,initialNextCursor,loadMore) and theirloadMorecallbacks only differ in theclient.GETendpoint, path params, and the extra workspace query spread. Extracting a small factory (e.g.,buildDagItem(result, query, remoteNode, client)/buildDocItem(...)) — or a single generic helper parameterized by endpoint and query-builder — would remove the ~40-line duplication of the response-unwrapping block and keep theitemsexpression compact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/search/components/SearchResult.tsx` around lines 175 - 254, The items map duplicates construction and response-unwrapping logic; extract the per-result loadMore closure and surrounding item builder into a helper (e.g., buildDagItem and buildDocItem or a single buildItem factory) that returns the object with key, kind, title, link, initialMatches, initialHasMoreMatches, initialNextCursor and a loadMore implementation. Use existing helpers workspaceMutationQueryForWorkspace, visibleDocumentPathForWorkspace, and workspaceDocumentQueryForWorkspace to compute the per-result params and pass endpoint, path params (if any), query fields (remoteNode, q, cursor, workspace spreads) and client into the factory so loadMore only calls client.GET once and returns the standardized { error, matches, hasMore, nextCursor } shape. Replace the inline maps for the DAG and Doc branches to call the new factory (e.g., buildDagItem(result, query, remoteNode, client) / buildDocItem(result, query, remoteNode, client)) to remove the duplicated response-unwrapping block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/frontend/api/v1/dags.go`:
- Around line 292-303: Handle and surface fatal errors from a.dagStore.LoadSpec
instead of dropping them: capture the returned err from
a.dagStore.LoadSpec(nextDAG, err := a.dagStore.LoadSpec(...)), and if err != nil
and it's not a build-validation error (i.e., not a core.ErrorList), return that
err immediately (or at minimum log and return) rather than proceeding to
a.dagStore.UpdateSpec; if err != nil but nextDAG != nil (allowed build errors),
allow the workspace check via
requireDAGWriteForWorkspace(dagWorkspaceName(nextDAG)) and continue, and
consider avoiding re-parse by passing the already-loaded nextDAG to an
UpdateSpec variant or short-circuiting UpdateSpec parsing when nextDAG is
available.
In `@ui/src/features/search/components/SearchResult.tsx`:
- Around line 185-229: The DAG result link doesn't include the result's
workspace query like Doc links do, causing the design page to open in the
current workspace instead of the result's workspace; update the DAG result block
(where link: `/dags/${encodeURI(result.fileName)}/spec` and dagWorkspaceQuery is
used) to build a linkSearch similar to the Doc branch (e.g., const linkSearch =
result.workspace ? `?workspace=${encodeURIComponent(result.workspace)}` : '')
and append it to the DAG link so the link becomes
`/dags/${encodeURI(result.fileName)}/spec${linkSearch}`; keep the existing
loadMore usage of dagWorkspaceQuery unchanged.
In `@ui/src/lib/workspace.ts`:
- Around line 252-265: The code currently returns defaultWorkspaceSelection()
immediately when WORKSPACE_STORAGE_KEY exists but sanitizeWorkspaceName(legacy)
is falsy, preventing a possible migration from
LEGACY_COCKPIT_WORKSPACE_STORAGE_KEY; change the logic in the block that reads
WORKSPACE_STORAGE_KEY so that you still removeItem(WORKSPACE_STORAGE_KEY) and
only persist and return when sanitized is truthy (using sanitizeWorkspaceName
and persistWorkspaceSelection), but if sanitized is falsy do NOT return—let the
function fall through to the subsequent legacy cockpit branch (which checks
LEGACY_COCKPIT_WORKSPACE_STORAGE_KEY) and only call defaultWorkspaceSelection()
at the very end if no valid legacy values are found.
---
Outside diff comments:
In `@api/v1/api.yaml`:
- Around line 1530-1538: The /dags/search operation is missing workspace scoping
like other DAG search endpoints; update its parameter block to include the
workspace scoping parameter (same name and schema used by the other DAG/search
endpoints — e.g., the "workspace" query parameter that accepts "accessible",
"none", or a workspace name), ensure it appears alongside the existing
RemoteNode and "q" parameters, and then regenerate server code with oapi-codegen
by running make api; locate the /dags/search operation in api/v1/api.yaml and
mirror the workspace parameter definition and allowed values used by the other
DAG/search endpoints.
In `@internal/service/frontend/api/v1/dags.go`:
- Around line 376-394: GetDAGDAGRunHistory drops the error from
a.dagStore.GetDetails causing nil dag to bypass visibility checks; change the
call to capture the error (dag, err := a.dagStore.GetDetails(...)) and if err !=
nil return nil, err (same behavior as getDAGDetailsData), so you only call
dagWorkspaceName(dag), requireWorkspaceVisible, resolveDAGName and
readHistoryData when dag was successfully loaded; ensure you keep the subsequent
uses of dag (dagWorkspaceName, readHistoryData) and dagName resolution
unchanged.
- Around line 734-810: GetAllDAGTags is a copy of GetAllDAGLabels and leaks
workspace labels and uses confusing names/constants; extract a shared helper
(e.g., collectLabels(ctx, workspaceScope, workspace) used by GetAllDAGLabels and
GetAllDAGTags) that does the List path (using exec.NewPaginator(1, math.MaxInt))
and fallback to a.dagStore.LabelList(ctx), deduplicate values, and filter out
any label whose key equals WorkspaceLabelKey before returning; update
GetAllDAGTags to use the helper, rename local variables/errors from "labels" to
"tags" where appropriate, and replace int(^uint(0)>>1) with math.MaxInt
everywhere.
---
Duplicate comments:
In `@api/v1/api.yaml`:
- Around line 775-776: The aggregate list endpoints currently return resources
across workspaces but downstream DAG/DAG-run single-resource detail/action
endpoints lack a way to accept the returned workspace; update the OpenAPI spec
to add the WorkspaceMutationScope and Workspace parameters to all
single-resource DAG and DAG-run detail/action paths (e.g., the operations
referencing DAG, DAG-run, and their detail/action operations) so callers can
disambiguate targets, or alternatively restrict aggregate listing endpoints to
include Workspace/WorkspaceScope when follow-up targeting is unsupported; ensure
you add the $ref entries for "#/components/parameters/WorkspaceMutationScope"
and "#/components/parameters/Workspace" to those single-resource operations (and
remove or adjust aggregate list params if you choose to constrain listings).
---
Nitpick comments:
In `@ui/src/features/search/components/SearchResult.tsx`:
- Around line 175-254: The items map duplicates construction and
response-unwrapping logic; extract the per-result loadMore closure and
surrounding item builder into a helper (e.g., buildDagItem and buildDocItem or a
single buildItem factory) that returns the object with key, kind, title, link,
initialMatches, initialHasMoreMatches, initialNextCursor and a loadMore
implementation. Use existing helpers workspaceMutationQueryForWorkspace,
visibleDocumentPathForWorkspace, and workspaceDocumentQueryForWorkspace to
compute the per-result params and pass endpoint, path params (if any), query
fields (remoteNode, q, cursor, workspace spreads) and client into the factory so
loadMore only calls client.GET once and returns the standardized { error,
matches, hasMore, nextCursor } shape. Replace the inline maps for the DAG and
Doc branches to call the new factory (e.g., buildDagItem(result, query,
remoteNode, client) / buildDocItem(result, query, remoteNode, client)) to remove
the duplicated response-unwrapping block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10fa43c8-6f68-46d0-9963-ae48f0c6b7db
📒 Files selected for processing (10)
api/v1/api.gen.goapi/v1/api.yamlinternal/service/frontend/api/v1/dags.gointernal/service/frontend/api/v1/docs.gointernal/service/frontend/api/v1/search.gointernal/service/frontend/api/v1/search_test.goui/src/api/v1/schema.tsui/src/features/search/components/SearchResult.tsxui/src/features/search/components/__tests__/SearchResult.test.tsxui/src/lib/workspace.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/src/features/search/components/tests/SearchResult.test.tsx
- internal/service/frontend/api/v1/search.go
- internal/service/frontend/api/v1/docs.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1/api.yaml (1)
9683-9704:⚠️ Potential issue | 🟠 MajorMake the
workspaceAccesscreate default explicit.
workspaceAccessis optional on create requests, but the API contract does not define whether omission means all workspaces, no workspaces, or role-derived access. For an authorization field, make it required or document a safe server default directly on the create request properties.Contract fix option
CreateUserRequest: type: object description: "Request body for creating a new user" properties: @@ workspaceAccess: + description: "Workspace access policy. If omitted, the server applies the legacy default of <documented default>." $ref: "#/components/schemas/WorkspaceAccess"CreateAPIKeyRequest: type: object description: "Create API key request" properties: @@ workspaceAccess: + description: "Workspace access policy. If omitted, the server applies the legacy default of <documented default>." $ref: "#/components/schemas/WorkspaceAccess"Or, if callers must always choose explicitly:
required: - name - role + - workspaceAccessAlso applies to: 9845-9865
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 9683 - 9704, The CreateUserRequest schema leaves workspaceAccess ambiguous; update the CreateUserRequest definition to make workspaceAccess explicit: either add "workspaceAccess" to the required array if callers must always supply it, or keep it optional but add a "default" value and a clear "description" explaining the server-side default behavior (e.g., "default: no access" or "default: same as role") so consumers know the semantics; adjust the CreateUserRequest schema (and the same CreateUserRequest occurrence referenced elsewhere) and ensure the WorkspaceAccess schema is referenced consistently in the description.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/v1/api.yaml`:
- Around line 9683-9704: The CreateUserRequest schema leaves workspaceAccess
ambiguous; update the CreateUserRequest definition to make workspaceAccess
explicit: either add "workspaceAccess" to the required array if callers must
always supply it, or keep it optional but add a "default" value and a clear
"description" explaining the server-side default behavior (e.g., "default: no
access" or "default: same as role") so consumers know the semantics; adjust the
CreateUserRequest schema (and the same CreateUserRequest occurrence referenced
elsewhere) and ensure the WorkspaceAccess schema is referenced consistently in
the description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c9d0c4c-e2ab-4259-acc5-51cf2e816f08
📒 Files selected for processing (3)
api/v1/api.gen.goapi/v1/api.yamlui/src/api/v1/schema.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/api/v1/schema.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1/api.yaml`:
- Around line 775-776: The exact DAG and DAG-run endpoints lack the
WorkspaceScope/Workspace parameters, so generated clients cannot target a
specific workspace; update each exact operation (e.g., the operations for
/dags/{fileName}, /dags/{fileName}/spec, /dags/{fileName}/start and
/dag-runs/{name}/{dagRunId}) to include the two parameter refs ($ref:
"#/components/parameters/WorkspaceScope" and $ref:
"#/components/parameters/Workspace") in the operation's parameters array (same
pattern used by the aggregate endpoints), or alternatively mark the aggregate
endpoints as single-workspace only if cross-workspace follow-ups aren’t
supported — locate the operations in api.yaml by the path strings and add the
WorkspaceScope and Workspace $ref entries to each operation’s parameters list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7853480-da1d-47e9-87cb-570953572ef8
📒 Files selected for processing (10)
api/v1/api.gen.goapi/v1/api.yamlinternal/service/frontend/api/v1/dags.gointernal/service/frontend/api/v1/dags_internal_test.goui/src/api/v1/schema.tsui/src/features/search/components/SearchResult.tsxui/src/features/search/components/__tests__/SearchResult.test.tsxui/src/lib/__tests__/workspace.test.tsui/src/lib/workspace.tsui/src/pages/dags/dag/index.tsx
✅ Files skipped from review due to trivial changes (1)
- ui/src/lib/tests/workspace.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- ui/src/features/search/components/tests/SearchResult.test.tsx
- ui/src/pages/dags/dag/index.tsx
- internal/service/frontend/api/v1/dags.go
- ui/src/api/v1/schema.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/e2e/helpers/e2e.ts (1)
174-187: Optional: document/guard the navigation precondition.
page.evaluatetoucheslocalStorage, which requires the page to already be on an app origin. All current callers invoke this afterloginViaUI(...)(which navigates to/loginand then away), so this works today. If a future caller invokesuseNoWorkspaceScopeon a freshpage(still atabout:blank), the evaluate will throw aSecurityError. Consider either a short JSDoc noting the precondition or defensively callingawait page.goto('/')first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/e2e/helpers/e2e.ts` around lines 174 - 187, The helper useNoWorkspaceScope currently calls page.evaluate to access localStorage (using WORKSPACE_SCOPE_STORAGE_KEY, LEGACY_WORKSPACE_STORAGE_KEY, LEGACY_COCKPIT_WORKSPACE_STORAGE_KEY) which will throw a SecurityError if the page is still at about:blank; update useNoWorkspaceScope to ensure the page is on the app origin before touching localStorage — either add a defensive await page.goto('/') (or the app root) when the page.url() is about:blank, or add JSDoc describing the precondition that callers must have navigated (e.g., via loginViaUI) before calling useNoWorkspaceScope so future callers won’t hit the SecurityError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ui/e2e/helpers/e2e.ts`:
- Around line 174-187: The helper useNoWorkspaceScope currently calls
page.evaluate to access localStorage (using WORKSPACE_SCOPE_STORAGE_KEY,
LEGACY_WORKSPACE_STORAGE_KEY, LEGACY_COCKPIT_WORKSPACE_STORAGE_KEY) which will
throw a SecurityError if the page is still at about:blank; update
useNoWorkspaceScope to ensure the page is on the app origin before touching
localStorage — either add a defensive await page.goto('/') (or the app root)
when the page.url() is about:blank, or add JSDoc describing the precondition
that callers must have navigated (e.g., via loginViaUI) before calling
useNoWorkspaceScope so future callers won’t hit the SecurityError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8f685f8-a48f-4a40-b436-3ed8572c87f7
📒 Files selected for processing (3)
ui/e2e/auth-flows.spec.tsui/e2e/dag-crud.spec.tsui/e2e/helpers/e2e.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/coordinator/handler.go (1)
946-953:⚠️ Potential issue | 🟠 MajorValidate persisted lease ownership before touching or repairing.
RunHeartbeatchecks the request’s owner ID, but the persisted lease can still belong to another coordinator. SinceTouchruns before the lease is read and the repair helper only checksWorkerID, an old coordinator can refresh/repair a lease it no longer owns.🐛 Proposed fix
for _, task := range req.RunningTasks { if task == nil || task.AttemptKey == "" { continue } + lease, err := h.dagRunLeaseStore.Get(ctx, task.AttemptKey) + if err != nil { + if errors.Is(err, exec.ErrDAGRunLeaseNotFound) { + cancelledRuns = appendCancelledRunIfMissing(cancelledRuns, task.AttemptKey) + continue + } + return nil, status.Error(codes.Internal, "failed to read run lease: "+err.Error()) + } + if h.owner.ID != "" && lease.Owner.ID != "" && lease.Owner.ID != h.owner.ID { + cancelledRuns = appendCancelledRunIfMissing(cancelledRuns, task.AttemptKey) + continue + } if err := h.dagRunLeaseStore.Touch(ctx, task.AttemptKey, observedAt); err != nil { if errors.Is(err, exec.ErrDAGRunLeaseNotFound) { cancelledRuns = appendCancelledRunIfMissing(cancelledRuns, task.AttemptKey) continue } return nil, status.Error(codes.Internal, "failed to refresh run lease: "+err.Error()) } h.repairStaleLeaseFailureFromRunHeartbeat(ctx, req.WorkerId, task, observedAt) }And keep the same guard in the helper to protect direct/internal call paths:
- if lease == nil || lease.AttemptID == "" || lease.WorkerID != workerID { + if lease == nil || lease.AttemptID == "" || lease.WorkerID != workerID { + return + } + if h.owner.ID != "" && lease.Owner.ID != "" && lease.Owner.ID != h.owner.ID { return }Also applies to: 982-984
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/coordinator/handler.go` around lines 946 - 953, Before calling dagRunLeaseStore.Touch and before invoking repairStaleLeaseFailureFromRunHeartbeat, first load the persisted lease for task.AttemptKey and verify its Owner/WorkerID matches req.WorkerId; if it does not, treat the run as not owned (append via appendCancelledRunIfMissing and continue) so we never refresh or repair a lease we don't own. Add the same ownership guard at the other call-site that mirrors lines 982-984. Also keep the existing WorkerID check inside repairStaleLeaseFailureFromRunHeartbeat so internal/direct callers are protected as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/service/coordinator/handler.go`:
- Around line 946-953: Before calling dagRunLeaseStore.Touch and before invoking
repairStaleLeaseFailureFromRunHeartbeat, first load the persisted lease for
task.AttemptKey and verify its Owner/WorkerID matches req.WorkerId; if it does
not, treat the run as not owned (append via appendCancelledRunIfMissing and
continue) so we never refresh or repair a lease we don't own. Add the same
ownership guard at the other call-site that mirrors lines 982-984. Also keep the
existing WorkerID check inside repairStaleLeaseFailureFromRunHeartbeat so
internal/direct callers are protected as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f09e3ad-9b3a-4a83-95ce-b63fac4140bc
📒 Files selected for processing (2)
internal/service/coordinator/handler.gointernal/service/coordinator/handler_test.go
1 participant
Summary
Testing
Summary by CodeRabbit
New Features
Behavior Changes
Tests