feat(core): waiting status#1554
Conversation
📝 WalkthroughWalkthroughIntroduces a human-in-the-loop (HITL) approval workflow system for DAGs. Adds Waiting and Rejected status values, a HITL executor for pause points, approval/rejection API endpoints with metadata persistence, wait handlers and notifications, and UI approval modals. Enables DAG runs to pause at designated steps pending human approval or rejection with optional inputs and reasons. Changes
Sequence DiagramsequenceDiagram
participant User
participant Client as Client/UI
participant API as API Server
participant Runner
participant Executor as HITL Executor
participant Storage
User->>Client: Navigate to DAG Run
Client->>API: GET /dag-runs/{id}
API->>Storage: Fetch DAG Run Status
Storage-->>API: DAG Run (status: Waiting)
API-->>Client: DAG Run Details
Client->>Client: Show Approve/Reject Buttons
User->>Client: Click Approve with Inputs
Client->>API: POST /dag-runs/{id}/steps/{name}/approve
rect rgb(200, 220, 240)
note right of API: Approval Handler
API->>Storage: Get DAG Run Status
Storage-->>API: Current Status
API->>Storage: Validate Step is Waiting
API->>API: Validate Required Inputs
API->>Storage: Update Step Status to Succeeded<br/>Set ApprovedAt, ApprovedBy
API->>Storage: Store Approval Inputs
end
rect rgb(200, 240, 200)
note right of API: Resume Logic
API->>Runner: Check for Remaining Waiting Nodes
alt Any Waiting Nodes Remain
Runner-->>API: HasWaiting = true
API-->>Client: ApproveStepResponse (Resumed: false)
else All Approved
Runner-->>API: HasWaiting = false
API->>Runner: Enqueue/Resume DAG Run
rect rgb(240, 220, 200)
note right of Executor: Execution Resumes
Runner->>Executor: Execute Next Steps
Executor->>Storage: Update Node Statuses
end
API-->>Client: ApproveStepResponse (Resumed: true)
end
end
Client->>Client: Display Success
Client->>API: Poll /dag-runs/{id}
API->>Storage: Fetch Updated Status
Storage-->>API: DAG Run (status: Succeeded/Waiting)
API-->>Client: Updated Status
Client->>User: Display Final Status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/src/features/dags/components/visualization/TimelineChart.tsx (1)
35-54: Add missing label case for NodeStatus.Waiting.The
getStatusLabelfunction is missing a case forNodeStatus.Waiting, which will cause the tooltip (line 265) to display "Unknown" instead of a proper label like "Waiting" or "Waiting for Approval".🔎 Proposed fix
function getStatusLabel(status: NodeStatus): string { switch (status) { case NodeStatus.NotStarted: return 'Not Started'; case NodeStatus.Running: return 'Running'; case NodeStatus.Success: return 'Success'; case NodeStatus.Failed: return 'Failed'; case NodeStatus.Aborted: return 'Aborted'; case NodeStatus.Skipped: return 'Skipped'; case NodeStatus.PartialSuccess: return 'Partial Success'; + case NodeStatus.Waiting: + return 'Waiting'; default: return 'Unknown'; } }api/v2/api.yaml (1)
3170-3192: Fix x-enum-varnames inconsistency.There's an inconsistency in the x-enum-varnames for the new status:
- Line 3145:
Statususes "Wait"- Line 3181:
NodeStatususes "Waiting"These should be consistent. Recommend using "Waiting" for both since it better matches "WaitingForApproval" naming patterns and is more descriptive.
🔎 Proposed fix for consistency
Update line 3145 in the Status enum:
- "Queued" - "PartialSuccess" - - "Wait" + - "Waiting"
🧹 Nitpick comments (10)
ui/src/ui/StatusChip.tsx (1)
52-56: Color styling is correct, but consider adding pulse animation for consistency.The amber/yellow color scheme for
Status.WaitmatchesNodeStatusChip.tsxperfectly and provides good visual distinction for the waiting state. However,NodeStatusChip.tsxapplies pulse animation to the Waiting status (line 84 in that file), while this component does not.🔎 Suggested enhancement for visual consistency
Consider adding pulse animation to the Wait status for consistency with NodeStatusChip:
function StatusChip({ status, children, size = 'md' }: Props) { // Determine the colors and icon based on status let bgColorClass = ''; let textColorClass = ''; let borderColorClass = ''; + let pulseAnimation = ''; switch (status) { case Status.Success: // done -> muted green (sepia-compatible) bgColorClass = 'bg-[rgba(107,168,107,0.12)]'; borderColorClass = 'border-[#6ba86b]'; textColorClass = 'text-[#5a8a5a]'; break; case Status.Failed: // error -> warm coral red (sepia-compatible) bgColorClass = 'bg-[rgba(196,114,106,0.12)]'; borderColorClass = 'border-[#c4726a]'; textColorClass = 'text-[#b05a52]'; break; case Status.Running: // running -> accent green (sepia-compatible) bgColorClass = 'bg-[rgba(125,168,125,0.15)]'; borderColorClass = 'border-[#7da87d]'; textColorClass = 'text-[#6b9a6b]'; + pulseAnimation = 'animate-pulse'; break; case Status.Aborted: // aborted -> muted coral/pink (sepia-compatible) bgColorClass = 'bg-[rgba(212,132,122,0.12)]'; borderColorClass = 'border-[#d4847a]'; textColorClass = 'text-[#c06a62]'; break; case Status.NotStarted: // none -> slate blue (sepia-compatible) bgColorClass = 'bg-[rgba(138,159,196,0.12)]'; borderColorClass = 'border-[#8a9fc4]'; textColorClass = 'text-[#6a7fa4]'; break; case Status.Queued: // queued -> muted purple (sepia-compatible) bgColorClass = 'bg-[rgba(154,122,196,0.12)]'; borderColorClass = 'border-[#9a7ac4]'; textColorClass = 'text-[#7a5aa4]'; break; case Status.PartialSuccess: // partial success -> warm amber (sepia-compatible) bgColorClass = 'bg-[rgba(212,148,106,0.12)]'; borderColorClass = 'border-[#d4946a]'; textColorClass = 'text-[#c47a4a]'; break; case Status.Wait: // waiting for approval -> amber/yellow (attention-grabbing) bgColorClass = 'bg-[rgba(245,158,11,0.15)]'; borderColorClass = 'border-[#f59e0b]'; textColorClass = 'text-[#d97706]'; + pulseAnimation = 'animate-pulse'; break; default: // Fallback to warm gray (sepia-compatible) bgColorClass = 'bg-[rgba(168,160,152,0.12)]'; borderColorClass = 'border-[#a8a098]'; textColorClass = 'text-[#6b635a]'; } // Size classes const sizeClasses = { xs: 'text-[10px] py-0 px-1.5', sm: 'text-xs py-0.5 px-2', md: 'text-sm py-1 px-3', lg: 'text-base py-1.5 px-4', }; // Render a pill-shaped badge with icon and text return ( <div className={cn( 'inline-flex items-center rounded-full border', bgColorClass, borderColorClass, textColorClass, + pulseAnimation, sizeClasses[size] )} > <span className={cn( 'font-normal break-keep text-nowrap whitespace-nowrap', textColorClass )} > {children} </span> </div> ); }Note: This also adds pulse to
Status.Runningfor consistency, which currently lacks it in this component.ui/src/features/dags/components/dag-execution/ParallelExecutionModal.tsx (1)
127-135: Consider addingStatusLabel.waitingto the status order.The
statusOrderarray determines which status filters appear in the UI. Since this PR introduces the Waiting state, sub-runs in a waiting state won't have a dedicated filter button and will only be visible in the "All" view.🔎 Suggested addition
const statusOrder: StatusLabel[] = [ StatusLabel.running, + StatusLabel.waiting, StatusLabel.queued, StatusLabel.succeeded, StatusLabel.partially_succeeded, StatusLabel.failed, StatusLabel.aborted, StatusLabel.not_started, ];ui/src/features/dags/components/dag-details/SubDAGRunsList.tsx (1)
167-175: Consider addingStatusLabel.waitingto the status order.Similar to
ParallelExecutionModal.tsx, thestatusOrderarray is missing the new Waiting state. Sub-runs waiting for approval won't have a dedicated filter button and will only appear in the "All" view.🔎 Suggested addition
const statusOrder: StatusLabel[] = [ StatusLabel.running, + StatusLabel.waiting, StatusLabel.queued, StatusLabel.succeeded, StatusLabel.partially_succeeded, StatusLabel.failed, StatusLabel.aborted, StatusLabel.not_started, ];internal/runtime/builtin/wait/config.go (1)
15-22: Consider validating required fields are in input list.The
decodeConfigfunction decodes the configuration but doesn't validate that all fields inRequiredare present inInput. This validation is performed elsewhere (in wait.go's validateConfig), but centralizing it here would improve cohesion.🔎 Optional: Add validation in decodeConfig
func decodeConfig(dat map[string]any, cfg *Config) error { md, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ WeaklyTypedInput: true, ErrorUnused: false, Result: cfg, }) + if err != nil { + return err + } - return md.Decode(dat) + if err := md.Decode(dat); err != nil { + return err + } + + // Validate that required fields are in input list + if len(cfg.Required) > 0 { + inputSet := make(map[string]bool) + for _, input := range cfg.Input { + inputSet[input] = true + } + for _, req := range cfg.Required { + if !inputSet[req] { + return fmt.Errorf("required field %q not in input list", req) + } + } + } + return nil }internal/runtime/builtin/wait/wait.go (1)
26-40: Wait executor wiring is coherent; minor config/Kill nits onlyThe executor cleanly reports
NodeWaiting(Line 77) and leaves actual blocking to the runner, which is the right separation. Config decoding and validation (Lines 26–40, 81–97) look correct and guard against invalidrequiredentries.If you ever touch this again, two small cleanups you might consider:
- Factor shared config decoding between
newWaitandvalidateConfigto avoid double decode of the same map.- Optionally log or document in
Kill(Lines 50–52) that it is intentionally a no-op for the wait executor, so future readers don’t wonder if termination handling was forgotten.Functionally this is solid as‑is.
Also applies to: 50-52, 81-97, 99-101
internal/service/frontend/api/v2/dagruns.go (1)
484-598: Approval endpoint and input validation are correct; consider tightening coupling to the wait configThe
ApproveDAGRunStepflow (Lines 484–598) is well structured:
- Permission +
requireExecutechecks mirror other mutating endpoints.- Step lookup and
NodeWaitingguard (Lines 503–523) prevent approving non-waiting steps.- You persist
ApprovedAtandApprovalInputs(Lines 533–538) and then, iff no other nodes remain inNodeWaiting, resume viaRetry(Lines 545–587), returning a clearResumedflag to the caller.
nodeStatusMappingincludingapi.NodeStatusWaiting: core.NodeWaiting(Lines 832–840) makes manual status updates consistent with the new state.
validateRequiredInputs(Lines 1154–1202) correctly enforces that all config‑declaredrequiredfields have corresponding entries inbody.Inputs, while safely no‑oping when config is absent or malformed.Two small improvements you might consider:
- In
validateRequiredInputs, short‑circuit on non‑wait executors (e.g.,if step.ExecutorConfig.Type != "wait" { return nil }) to make it explicit that only the wait builtin’s semantics are being applied here.- For
ApprovedAt, align with the rest of your timestamps by using UTC explicitly (time.Now().UTC().Format(time.RFC3339)), if other codepaths already standardize on UTC.Otherwise the endpoint and helper look correct and in line with the rest of the API.
Also applies to: 832-840, 1154-1202
api/v2/api.yaml (2)
2012-2055: Clarify input validation and environment variable naming convention.The endpoint description mentions that inputs will be available as environment variables, but the specification doesn't document:
- The naming convention for environment variables (e.g., are keys uppercased? prefixed?)
- How input validation works (the 400 response mentions "required inputs missing" but there's no schema-level definition of which inputs are required)
- Whether there are any restrictions on input key names or values
Consider adding this information to the endpoint description or creating a separate section in the API docs to explain the HITL input parameter conventions.
2821-2838: Clarify the semantics of theresumedfield.The description states "Whether the DAG run was re-enqueued for execution" but doesn't explain:
- Under what conditions would
resumedbetruevsfalse?- If
resumedisfalse, what state is the DAG run in after approval?- Does "re-enqueued" mean the DAG was previously dequeued/paused, or is this just the mechanism for continuing execution?
Consider expanding the description to clarify the approval workflow and what callers should do when
resumedisfalse.api/v2/api.gen.go (2)
202-219: Approve request/response and params are coherent; consider type alias consistency*The new
ApproveStepRequest,ApproveStepResponse,ApproveDAGRunStepParams, andApproveDAGRunStepJSONRequestBodyshape a clear, minimal API for HITL approval and match the rest of the codegen patterns.One small consistency nit: elsewhere run IDs and step names usually use the alias types
DAGRunId/StepName, whileApproveStepResponseuses plainstringfor both fields. If you want to keep type usage uniform across the API, you could adjust the OpenAPI schema to$refthose aliases so codegen emits:Example of more consistent field types (change via api.yaml, then re‑generate)
type ApproveStepResponse struct { - // DagRunId The DAG run ID - DagRunId string `json:"dagRunId"` + // DagRunId The DAG run ID + DagRunId DAGRunId `json:"dagRunId"` @@ - // StepName The approved step name - StepName string `json:"stepName"` + // StepName The approved step name + StepName StepName `json:"stepName"` }Not a blocker; just a polish suggestion at the spec level.
Also applies to: 1452-1457, 1871-1873
6778-6827: Strict types and handler for ApproveDAGRunStep are consistent with other strict endpointsThe strict API surface and implementation for
ApproveDAGRunSteplook correct:
ApproveDAGRunStepRequestObject/ResponseObjectmirror other operations.ApproveDAGRunStep200/400/404/defaultresponse types follow the standard visitor pattern.StrictServerInterfacegains a single, well‑typedApproveDAGRunStepmethod.strictHandler.ApproveDAGRunStep:
- Populates
Name,DagRunId,StepName, andParams.- Decodes the JSON body into
ApproveDAGRunStepJSONRequestBody(ApproveStepRequest) with the same error handling used elsewhere.- Threads through strict middlewares with operation name
"ApproveDAGRunStep"and invokes the response visitor.Only behavioral note: as with other strict handlers, an entirely empty request body will cause JSON decode to fail; clients should send at least
{}when no inputs are needed. This matches existing semantics for other JSON‑body operations.Also applies to: 8722-8724, 9443-9477
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
api/v2/api.gen.goapi/v2/api.yamlinternal/common/telemetry/collector.gointernal/core/execution/node.gointernal/core/status.gointernal/output/tree.gointernal/persistence/filedagrun/dataroot_test.gointernal/runtime/builtin/builtin.gointernal/runtime/builtin/wait/config.gointernal/runtime/builtin/wait/wait.gointernal/runtime/builtin/wait/wait_test.gointernal/runtime/node.gointernal/runtime/runner.gointernal/runtime/runner_helper_test.gointernal/runtime/runner_test.gointernal/service/frontend/api/v2/dagruns.gointernal/service/frontend/api/v2/validation_test.goui/src/api/v2/schema.tsui/src/features/dags/components/common/NodeStatusChip.tsxui/src/features/dags/components/common/StatusDot.tsxui/src/features/dags/components/common/index.tsui/src/features/dags/components/common/statusLabels.tsui/src/features/dags/components/dag-details/SubDAGRunsList.tsxui/src/features/dags/components/dag-execution/ParallelExecutionModal.tsxui/src/features/dags/components/visualization/Graph.tsxui/src/features/dags/components/visualization/TimelineChart.tsxui/src/ui/StatusChip.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/output/tree.gointernal/core/execution/node.gointernal/runtime/builtin/builtin.gointernal/core/status.gointernal/runtime/runner_test.gointernal/runtime/node.gointernal/runtime/builtin/wait/config.gointernal/runtime/runner.gointernal/runtime/runner_helper_test.gointernal/runtime/builtin/wait/wait.gointernal/persistence/filedagrun/dataroot_test.gointernal/common/telemetry/collector.gointernal/service/frontend/api/v2/validation_test.gointernal/runtime/builtin/wait/wait_test.gointernal/service/frontend/api/v2/dagruns.goapi/v2/api.gen.go
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/features/dags/components/common/StatusDot.tsxui/src/features/dags/components/common/index.tsui/src/features/dags/components/common/statusLabels.tsui/src/features/dags/components/common/NodeStatusChip.tsxui/src/features/dags/components/dag-details/SubDAGRunsList.tsxui/src/ui/StatusChip.tsxui/src/features/dags/components/visualization/Graph.tsxui/src/features/dags/components/visualization/TimelineChart.tsxui/src/api/v2/schema.tsui/src/features/dags/components/dag-execution/ParallelExecutionModal.tsx
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/features/dags/components/common/StatusDot.tsxui/src/features/dags/components/common/index.tsui/src/features/dags/components/common/statusLabels.tsui/src/features/dags/components/common/NodeStatusChip.tsxui/src/features/dags/components/dag-details/SubDAGRunsList.tsxui/src/ui/StatusChip.tsxui/src/features/dags/components/visualization/Graph.tsxui/src/features/dags/components/visualization/TimelineChart.tsxui/src/api/v2/schema.tsui/src/features/dags/components/dag-execution/ParallelExecutionModal.tsx
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/runtime/runner_test.gointernal/runtime/runner_helper_test.gointernal/persistence/filedagrun/dataroot_test.gointernal/service/frontend/api/v2/validation_test.gointernal/runtime/builtin/wait/wait_test.go
{api/v1,api/v2}/**/*.{proto,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
API definitions live in
api/v1andapi/v2; generated server stubs land ininternal/service, while matching TypeScript clients flow intoui/src/api
Files:
api/v2/api.yaml
🧠 Learnings (3)
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Use consistent metadata styling with `bg-slate-200 dark:bg-slate-700` backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Applied to files:
ui/src/features/dags/components/common/StatusDot.tsxui/src/ui/StatusChip.tsxui/src/features/dags/components/visualization/TimelineChart.tsx
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/service/frontend/api/v2/validation_test.go
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/service/frontend/api/v2/validation_test.go
🧬 Code graph analysis (15)
internal/output/tree.go (1)
internal/core/status.go (2)
NodeWaiting(69-69)Wait(14-14)
ui/src/features/dags/components/common/StatusDot.tsx (2)
internal/core/status.go (1)
Status(4-4)api/v2/api.gen.go (1)
Status(977-977)
ui/src/features/dags/components/common/statusLabels.ts (2)
ui/src/features/dags/components/common/index.ts (1)
STATUS_DISPLAY_LABELS(15-15)api/v2/api.gen.go (1)
StatusLabel(980-980)
internal/core/status.go (3)
api/v2/api.gen.go (2)
Status(977-977)NodeStatus(738-738)internal/persistence/legacy/model/status.go (1)
Status(30-46)api/v1/api.gen.go (1)
NodeStatus(444-444)
internal/runtime/runner_test.go (2)
internal/runtime/data.go (1)
Parallel(69-73)internal/core/status.go (4)
Wait(14-14)NodeSucceeded(66-66)NodeWaiting(69-69)NodeNotStarted(62-62)
internal/runtime/node.go (1)
internal/core/status.go (1)
NodeWaiting(69-69)
ui/src/features/dags/components/common/NodeStatusChip.tsx (3)
internal/core/status.go (1)
NodeStatus(59-59)api/v2/api.gen.go (1)
NodeStatus(738-738)api/v1/api.gen.go (1)
NodeStatus(444-444)
internal/runtime/runner.go (3)
internal/core/status.go (8)
Wait(14-14)Status(4-4)NodeRunning(63-63)NodeNotStarted(62-62)NodeWaiting(69-69)NodeSucceeded(66-66)NodeAborted(65-65)NodeSkipped(67-67)internal/runtime/plan.go (1)
Plan(22-36)internal/core/execution/node.go (1)
Node(9-28)
internal/runtime/runner_helper_test.go (2)
internal/core/step.go (2)
Step(13-97)ExecutorConfig(189-197)internal/core/status.go (3)
Succeeded(11-11)Aborted(10-10)Wait(14-14)
internal/runtime/builtin/wait/wait.go (4)
internal/runtime/executor/executor.go (1)
Executor(15-20)internal/runtime/builtin/wait/config.go (1)
Config(6-13)internal/core/step.go (1)
ExecutorConfig(189-197)internal/core/status.go (2)
NodeStatus(59-59)NodeWaiting(69-69)
internal/persistence/filedagrun/dataroot_test.go (3)
internal/persistence/filedagrun/dagrun_test.go (1)
DAGRunTest(41-45)internal/core/status.go (3)
Status(4-4)Succeeded(11-11)Wait(14-14)internal/common/fileutil/fileutil.go (1)
FileExists(44-47)
internal/common/telemetry/collector.go (1)
internal/core/status.go (4)
NotStarted(7-7)Running(8-8)Queued(12-12)Wait(14-14)
ui/src/features/dags/components/visualization/TimelineChart.tsx (2)
internal/core/status.go (1)
NodeStatus(59-59)api/v2/api.gen.go (1)
NodeStatus(738-738)
internal/service/frontend/api/v2/validation_test.go (2)
api/v2/api.gen.go (1)
ApproveStepRequest(203-206)internal/core/step.go (1)
ExecutorConfig(189-197)
internal/runtime/builtin/wait/wait_test.go (2)
internal/core/step.go (1)
ExecutorConfig(189-197)internal/core/status.go (1)
NodeWaiting(69-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test on ubuntu-latest
- GitHub Check: Build
🔇 Additional comments (34)
internal/core/execution/node.go (1)
24-27: LGTM! Clean addition of HITL approval fields.The new fields are well-structured:
- Proper JSON serialization tags with
omitempty- Clear documentation comments
- Appropriate types for timestamp and key-value parameters
internal/persistence/filedagrun/dataroot_test.go (1)
292-338: LGTM! Excellent test coverage for Wait status preservation.The test properly validates that DAG runs in Wait status are treated as active and preserved during cleanup, which is critical for HITL workflows. The test setup is clean and uses proper helpers.
internal/output/tree.go (1)
662-663: LGTM! Correct status mapping for duration calculation.The NodeWaiting → Wait mapping is consistent with the other status conversions in this function and enables proper duration calculation for nodes in the waiting state.
internal/runtime/node.go (1)
116-118: LGTM! Appropriate handling of NodeWaiting state.The fallthrough to the error path is correct—a node in waiting state should not reach the continuation check logic, as it's awaiting human approval. The error logging will help identify any unexpected state transitions.
internal/common/telemetry/collector.go (1)
431-431: LGTM! Correct classification of Wait as non-completed status.The Wait status represents an active but paused state awaiting approval, so it's correctly grouped with other non-terminal statuses (NotStarted, Running, Queued). This ensures metrics properly track waiting DAG runs as in-progress rather than completed.
ui/src/features/dags/components/common/NodeStatusChip.tsx (1)
80-86: LGTM! Well-integrated waiting state.The new
NodeStatus.Waitingcase is properly implemented with appropriate amber/yellow styling, pulse animation, and a pause icon that effectively communicates the waiting-for-approval state.internal/runtime/builtin/builtin.go (1)
13-13: LGTM! Proper side-effect import pattern.The import follows the standard Go pattern for init-based package registration and maintains alphabetical ordering with other builtin executors.
ui/src/features/dags/components/common/index.ts (1)
15-15: LGTM! Clean public API extension.The export of
STATUS_DISPLAY_LABELSproperly extends the module's public API for status label rendering across components.ui/src/features/dags/components/visualization/TimelineChart.tsx (1)
88-88: Color mapping added correctly.The amber color (#f59e0b) for
NodeStatus.Waitingis consistent with other UI components and appropriate for an attention-grabbing waiting state.ui/src/features/dags/components/common/statusLabels.ts (1)
1-16: LGTM! Excellent centralization of status labels.This centralized mapping eliminates duplication and provides type safety through
Record<StatusLabel, string>, ensuring all enum values are covered. The human-readable labels are clear and appropriate.ui/src/features/dags/components/common/StatusDot.tsx (1)
36-39: LGTM! Appropriate visual treatment for the Waiting state.The warm orange color (
#f59e0b) and pulse animation effectively communicate an active waiting state, distinguishing it from both Running (green) and error states while maintaining consistency with the existing animation pattern.internal/runtime/runner_helper_test.go (3)
26-28: LGTM! Test helper follows established patterns.The
waitStephelper is consistent with existingsuccessStepandfailStephelpers, making it easy to test wait/approval workflows in the test suite.
109-116: LGTM! Useful abstraction for testing custom executors.This helper provides a clean way to configure steps with different executor types in tests, supporting the new wait executor and future executor types.
249-249: LGTM! Treating Wait as a valid terminal status aligns with HITL design.The test assertion correctly recognizes that reaching a Wait state (waiting for approval) is not an error condition but an intentional pause in the workflow, similar to Succeeded or Aborted.
ui/src/features/dags/components/dag-execution/ParallelExecutionModal.tsx (1)
13-13: LGTM! Proper refactoring to centralized status labels.The import correctly uses the shared
STATUS_DISPLAY_LABELSconstant, eliminating duplication and ensuring consistency across components.ui/src/features/dags/components/dag-details/SubDAGRunsList.tsx (1)
7-7: LGTM! Correct refactoring to shared constants.The import properly uses the centralized
STATUS_DISPLAY_LABELS, maintaining consistency with the broader refactoring across UI components.internal/service/frontend/api/v2/validation_test.go (1)
11-161: LGTM! Comprehensive test coverage.The test suite thoroughly covers the validation logic with clear scenarios:
- Edge cases (nil/empty config)
- Success paths (all required fields provided)
- Failure paths (missing required fields with descriptive error messages)
- Extra fields handling
The table-driven approach and use of
testify/assertalign with the coding guidelines.internal/core/status.go (2)
14-14: LGTM! Consistent status model extension.The new
Waitstatus andIsWaiting()method are correctly integrated:
- String representation follows the existing lowercase convention ("waiting")
IsActive()properly treatsWaitas an active state alongsideRunningandQueued- The implementation mirrors the existing patterns for other status values
Also applies to: 35-36, 42-46, 53-56
69-69: LGTM! Node-level waiting status properly added.The
NodeWaitingstatus variant and itsIsWaiting()helper method are consistent with the DAG-levelWaitstatus, providing a complete status model for the HITL workflow.Also applies to: 77-80, 99-100
internal/runtime/runner_test.go (1)
2810-2889: LGTM! Thorough wait step test coverage.The
TestWaitStepsuite provides comprehensive validation of the wait/approval workflow:
- WaitStepResultsInWaitStatus: Verifies DAG transitions to
Waitstatus when a wait step completes- WaitStepBlocksDependentNodes: Confirms downstream nodes remain
NotStartedwhile waiting- ParallelBranchWithWaitStep: Tests that independent branches proceed while wait branch blocks
- WaitStepAtStart: Validates wait-first scenarios
All tests follow the established patterns, use
t.Parallel()appropriately, and provide clear assertions.internal/runtime/builtin/wait/wait_test.go (3)
13-82: LGTM! Comprehensive executor behavior tests.The
TestWaitExecutor_Runtest suite validates the wait executor output for various configurations:
- Basic wait with no config
- Wait with custom prompt
- Wait with input fields and required validation
Stdout capture and assertion approach is appropriate for verifying user-facing messages.
84-105: LGTM! Proper interface verification.The test correctly verifies that the wait executor implements the
NodeStatusDeterminerinterface and returnsNodeWaitingstatus, ensuring runtime integration works as expected.
107-160: LGTM! Thorough config validation tests.The validation test suite covers:
- Nil and empty configs (valid)
- Required fields present in input list (valid)
- Required fields missing from input list (invalid)
This ensures the wait executor properly validates its configuration before execution.
ui/src/features/dags/components/visualization/Graph.tsx (1)
245-272: Waiting node visual state is correctly integratedThe new
classDef waiting(Lines 270–272) andgraphStatusMapentries forNodeStatus.Waiting(Lines 471–481) line up with existing status mappings and Mermaid usage; no issues from a runtime or typing perspective. Color choice also fits the existing palette.Also applies to: 471-481
internal/runtime/runner.go (1)
176-189: Wait-state handling across the runner is consistent and well-isolatedThe new logic to:
- Break the main loop when
running == 0 && isWaitingForApproval(plan)(Lines 176–181),- Surface
core.WaitfromStatusbefore other terminal states (Lines 594–599),- Define
isWaitingForApproval/canNodeRunas “no running nodes, at least oneNodeWaiting, and allNodeNotStartednodes are transitively blocked by a waiting dep” (Lines 611–677),- Treat
NodeWaitingas a hard dependency blocker inisReady(Lines 702–771),- Exclude
NodeWaitingfrom partial-success computation (Lines 878–913), and- Avoid counting waiting nodes as completed in
finishNode(Lines 1019–1032),fits together cleanly: a DAG that’s paused for approval won’t be marked deadlocked, won’t run handlers prematurely, and will report a dedicated Wait status. The DFS with a
visitedset incanNodeRunHelperis a reasonable balance between correctness and simplicity for detecting nodes blocked by waiting dependencies.I’d just ensure tests cover:
- Multiple
NodeWaitingsteps (parallel and sequential),- Graphs with both waiting and failed/aborted branches, and
- Cycles (to confirm we still end up in the deadlock path, not Wait, when no waiting nodes exist).
Otherwise this looks solid.
Also applies to: 261-281, 583-609, 611-677, 702-777, 878-913, 1019-1032
ui/src/api/v2/schema.ts (1)
799-818: Generated schema for approval + waiting status looks consistent with backendThe additions for:
ApproveStepRequest/ApproveStepResponse(Lines 1122–1137),- The new
/dag-runs/{name}/{dagRunId}/steps/{stepName}/approvepath (Lines 799–818) andoperations["approveDAGRunStep"]wiring (Lines 4395–4455), and- The extended
Status/NodeStatusenums and labels including Wait/Waiting and PartialSuccess (Lines 1302–1338, 5218–5256),all align with the Go handler and status model in
internal/service/frontend/api/v2/dagruns.goandinternal/core/status.go. The shapes and enum values match what the server expects.Given this file is auto‑generated, the only ask is to keep it in sync by regenerating from the OpenAPI spec rather than hand‑editing; structurally the current diff looks good.
Also applies to: 1122-1137, 1302-1338, 4395-4455, 5218-5256
api/v2/api.yaml (4)
2811-2820: LGTM!The
ApproveStepRequestschema is well-defined with an appropriate structure for optional key-value parameters.
3134-3156: LGTM!The
Statusenum extension is correctly implemented with value 7 for "Waiting for approval", including proper x-enum-varnames and comprehensive descriptions.
3157-3169: LGTM!The
StatusLabelenum extension correctly adds "waiting" following the existing naming convention.
3193-3205: LGTM!The
NodeStatusLabelenum extension correctly adds "waiting" consistent with theStatusLabelenum.api/v2/api.gen.go (4)
61-71: Waiting status enum and documentation look consistentThe new
NodeStatusWaiting/StatusWaitvalue7and the corresponding"waiting"labels and"Waiting for approval"doc comments are wired consistently across all status enums and the variousStatus/NodeStatusfields. No issues from the API surface point of view.Also applies to: 73-83, 104-114, 116-126, 488-497, 549-558, 699-708, 729-738, 968-977, 1079-1088, 1481-1490, 1561-1570
2099-2101: ServerInterface and Unimplemented stubs for ApproveDAGRunStep are in line with existing endpointsThe new
ApproveDAGRunStepmethod onServerInterfaceand itsUnimplementedstub mirror the existing step‑level endpoints (log/status) in signature and comment style. This ensures the new route is exposed uniformly and will integrate cleanly with existing implementations.Also applies to: 2354-2358
3458-3518: Wrapper and routing for ApproveDAGRunStep follow established middleware pattern
ServerInterfaceWrapper.ApproveDAGRunStepcorrectly:
- Binds
name,dagRunId, andstepNamefrom the path.- Binds optional
remoteNodefrom the query string.- Applies
ApiTokenScopes/BasicAuthScopesto the context, matching other DAG‑run endpoints.- Wraps the handler with
HandlerMiddlewaresand registers viaHandlerWithOptionsatPOST /dag-runs/{name}/{dagRunId}/steps/{stepName}/approve.Body decoding is intentionally left to the handler (or strict wrapper), consistent with other non‑strict endpoints.
Also applies to: 5752-5754
10558-11000: Embedded swaggerSpec appears correctly regenerated alongside codeThe
swaggerSpecbase64+gzip blob has been updated in tandem with the new statuses and theApproveDAGRunStependpoint glue. Assuming it was produced from the same updated OpenAPI document, this keepsGetSwagger()consistent with the generated types and routes.As usual for generated code, treat this blob as output of the spec/codegen pipeline and avoid manual edits; changes should go through the OpenAPI spec and re‑generation.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (14)
ui/src/features/dags/components/dag-execution/ApprovalModal.tsx (1)
99-117: Improve input accessibility and compact styling.A few improvements for accessibility and guideline compliance:
- Associate
<label>with<input>usinghtmlFor/id- Use compact input padding per guidelines (
py-1instead ofpy-2)- Consider adding Enter key support to submit the form
🔎 Suggested improvements
- <div key={field}> - <label className="block text-sm font-medium mb-1"> + <div key={field}> + <label + htmlFor={`approval-input-${field}`} + className="block text-sm font-medium mb-1" + > {field} {isRequired && ( <span className="text-error ml-1">*</span> )} </label> <input + id={`approval-input-${field}`} type="text" value={inputs[field] || ''} onChange={(e) => setInputs((prev) => ({ ...prev, [field]: e.target.value, })) } - className="w-full px-3 py-2 text-sm border border-border rounded bg-background focus:outline-none focus:ring-2 focus:ring-primary/50" + className="w-full px-3 py-1 text-sm border border-border rounded bg-background focus:outline-none focus:ring-2 focus:ring-primary/50" placeholder={`Enter ${field}`} + onKeyDown={(e) => { + if (e.key === 'Enter' && !e.shiftKey) { + e.preventDefault(); + handleApprove(); + } + }} /> </div>Based on learnings and coding guidelines requiring compact padding and keyboard navigation support.
ui/src/features/dags/components/dag-details/NodeStatusTableRow.tsx (1)
466-502: LGTM!The
handleApproveimplementation correctly mirrors the existinghandleStatusUpdatepattern. The error is properly thrown soApprovalModalcan catch and display it.One minor observation: the
isSubDAGRuncheck is computed in multiple places (here,handleStatusUpdate, andInlineLogViewer). Consider extracting it to a shared computed value if this pattern expands further.internal/runtime/data.go (1)
65-67: Consider adding thread-safe accessor methods for ApprovalInputs.The
ApprovalInputsfield is added toNodeState, but unlike other fields (e.g.,OutputVariables), there are no corresponding thread-safe getter/setter methods in theDatawrapper. SinceDatauses a mutex for thread-safe access toNodeState, consider adding accessor methods likeGetApprovalInputs()andSetApprovalInputs(map[string]string)to maintain consistency with the existing pattern and ensure thread-safe access to this field.🔎 Proposed accessor methods
+func (d *Data) GetApprovalInputs() map[string]string { + d.mu.RLock() + defer d.mu.RUnlock() + + if d.inner.State.ApprovalInputs == nil { + return nil + } + // Return a copy to prevent external modifications + result := make(map[string]string, len(d.inner.State.ApprovalInputs)) + for k, v := range d.inner.State.ApprovalInputs { + result[k] = v + } + return result +} + +func (d *Data) SetApprovalInputs(inputs map[string]string) { + d.mu.Lock() + defer d.mu.Unlock() + + if inputs == nil { + d.inner.State.ApprovalInputs = nil + return + } + d.inner.State.ApprovalInputs = make(map[string]string, len(inputs)) + for k, v := range inputs { + d.inner.State.ApprovalInputs[k] = v + } +}internal/runtime/agent/reporter_test.go (1)
47-51: Consider testing edge cases for WaitMail configuration.The WaitMail configuration is properly added to the test DAG setup. However, consider adding test scenarios for edge cases such as:
- WaitMail being nil when MailOn.Wait is enabled
- Missing required fields (From, To) in WaitMail config
These edge cases would help ensure robust error handling in the reporter.
🔎 Example edge case tests
Add these test scenarios to the TestReporter function:
"wait mail with nil config": testWaitMailNilConfig, "wait mail with missing fields": testWaitMailMissingFields,And implement the helper functions:
func testWaitMailNilConfig(t *testing.T, rp *reporter, mock *mockSender, dag *core.DAG, nodes []*execution.Node) { dag.MailOn.Wait = true dag.WaitMail = nil err := rp.send(context.Background(), dag, execution.DAGRunStatus{ Status: core.Wait, Nodes: nodes, }, nil) // Verify graceful handling (no panic) require.NoError(t, err) require.Equal(t, 0, mock.count) }internal/core/spec/loader_test.go (1)
208-214: Add assertion for AttachLogs inheritance.The test verifies prefix override and inheritance of From and To fields for WaitMail, but doesn't check the AttachLogs field. For consistency with the ErrorMail assertions (line 200), add a check to verify that AttachLogs is not inherited when the base config doesn't set it.
🔎 Proposed additional assertion
// Other fields should be inherited from base assert.Equal(t, "base@example.com", dag.WaitMail.From, "waitMail from should be inherited from base") assert.Equal(t, []string{"wait@example.com"}, dag.WaitMail.To, "waitMail to should be inherited from base") + assert.False(t, dag.WaitMail.AttachLogs, "waitMail attachLogs should be default (false) when not set in either config")api/v2/api.gen.go (1)
10951-11195: Gitleaks “generic-api-key” finding inside embedded swaggerSpecThe Gitleaks alert is anchored in the base64-encoded
swaggerSpecblob. Since this is a compressed OpenAPI document generated fromapi.yaml, the most likely cause is an example value (e.g., the documented'Bearer dagu_wh_...'webhook token pattern) rather than a real secret.Still, it’s worth double-checking that:
api/v2/api.yamldoes not contain any real API keys, tokens, or environment-specific secrets in examples.- All tokens in examples are clearly dummy/test strings.
If everything in the spec is synthetic, this is a false positive and no changes are needed.
If you’d like, I can suggest a small script to decode
swaggerSpec, pretty-print the JSON, and grep for suspicious-looking tokens so you can quickly confirm there are no real secrets embedded.internal/core/dag_test.go (1)
881-934: Consider using executor type constants for better maintainability.The test correctly validates
HasWaitSteps()behavior. However, using string literals like"command","dag", and"wait"instead of the exported constants (e.g.,core.ExecutorTypeWait) reduces maintainability. If constant values change, tests using string literals won't catch the inconsistency.🔎 Proposed refactor to use constants
func TestDAGHasWaitSteps(t *testing.T) { t.Parallel() tests := []struct { name string steps []core.Step expected bool }{ { name: "Empty", steps: []core.Step{}, expected: false, }, { name: "NoWait", steps: []core.Step{ - {Name: "step1", ExecutorConfig: core.ExecutorConfig{Type: "command"}}, - {Name: "step2", ExecutorConfig: core.ExecutorConfig{Type: "dag"}}, + {Name: "step1", ExecutorConfig: core.ExecutorConfig{Type: "command"}}, // or use a constant if available + {Name: "step2", ExecutorConfig: core.ExecutorConfig{Type: core.ExecutorTypeDAG}}, }, expected: false, }, { name: "HasWait", steps: []core.Step{ {Name: "step1", ExecutorConfig: core.ExecutorConfig{Type: "command"}}, - {Name: "step2", ExecutorConfig: core.ExecutorConfig{Type: "wait"}}, + {Name: "step2", ExecutorConfig: core.ExecutorConfig{Type: core.ExecutorTypeWait}}, }, expected: true, }, { name: "OnlyWait", steps: []core.Step{ - {Name: "step1", ExecutorConfig: core.ExecutorConfig{Type: "wait"}}, + {Name: "step1", ExecutorConfig: core.ExecutorConfig{Type: core.ExecutorTypeWait}}, }, expected: true, }, { name: "EmptyType", steps: []core.Step{ {Name: "step1", ExecutorConfig: core.ExecutorConfig{Type: ""}}, }, expected: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() dag := &core.DAG{Steps: tt.steps} result := dag.HasWaitSteps() assert.Equal(t, tt.expected, result) }) } }internal/core/spec/dag_test.go (1)
1963-2035: Good test coverage with minor maintainability suggestion.The test comprehensively validates the restriction against dispatching wait-step DAGs to workers. However, consider using
core.ExecutorTypeWaitinstead of the string literal"wait"for better maintainability.🔎 Optional refactor to use constant
{ name: "NoWorkerSelector", dag: &dag{ Name: "test-dag", Steps: []any{ - map[string]any{"name": "step1", "type": "wait"}, + map[string]any{"name": "step1", "type": core.ExecutorTypeWait}, }, }, expectErr: false, }, { name: "Conflict", dag: &dag{ Name: "test-dag", WorkerSelector: map[string]string{"region": "us-west"}, Steps: []any{ - map[string]any{"name": "step1", "type": "wait"}, + map[string]any{"name": "step1", "type": core.ExecutorTypeWait}, }, }, expectErr: true, errContains: "DAG with wait steps cannot be dispatched to workers", },internal/integration/approval_test.go (1)
36-40: Consider making node assertions more robust against ordering changes.The tests access nodes by index (
dagRunStatus.Nodes[0],Nodes[1], etc.), which assumes a specific ordering. If the implementation changes node ordering, these tests could fail unexpectedly. Consider iterating over nodes and matching by name instead.🔎 Example of name-based node lookup
// Helper function to find node by name func findNodeByName(nodes []*execution.Node, name string) *execution.Node { for _, n := range nodes { if n.Step.Name == name { return n } } return nil } // Usage in test: waitNode := findNodeByName(dagRunStatus.Nodes, "wait-step") require.NotNil(t, waitNode) require.Equal(t, core.NodeWaiting, waitNode.Status)ui/src/api/v2/schema.ts (1)
1142-1157: Clarify "required inputs" validation logic.The
ApproveStepRequest.inputsfield is marked optional in the schema, but the error response at line 4456 mentions "required inputs missing". This suggests the backend determines required inputs dynamically based on the step's executor configuration.Consider documenting this behavior in the schema description to clarify that while the
inputsfield itself is optional, specific executor configurations may require certain input keys.📝 Suggested documentation enhancement
/** @description Request body for approving a waiting step */ ApproveStepRequest: { - /** @description Key-value parameters to provide. These will be available as environment variables in subsequent steps. */ + /** @description Key-value parameters to provide. These will be available as environment variables in subsequent steps. While this field is optional, specific executor configurations may require certain input keys to be present. */ inputs?: { [key: string]: string; }; };api/v2/api.yaml (4)
2012-2054: Clarify required inputs validation mechanism.The error response mentions "required inputs missing" (line 2038), but the OpenAPI spec doesn't document how required inputs are determined. Consider adding documentation that explains:
- How required inputs are defined (presumably in the step's
executorConfig)- What validation occurs when inputs are provided vs. when they're omitted
- An example of the
executorConfigstructure for a 'wait' executor that defines required inputsThis will help API consumers understand when and why a 400 error with "required inputs missing" might occur.
📝 Example documentation enhancement
Add to the endpoint description:
/dag-runs/{name}/{dagRunId}/steps/{stepName}/approve: post: summary: "Approve a waiting step for HITL" - description: "Approves a step that is in Waiting status, optionally providing input parameters that will be available as environment variables in subsequent steps" + description: | + Approves a step that is in Waiting status, optionally providing input parameters that will be available as environment variables in subsequent steps. + + Required inputs are defined in the step's executorConfig. If the step's wait executor configuration specifies required inputs, + all of them must be provided in the request body, otherwise a 400 error is returned.
2871-2888: Clarify theresumedfield semantics.The
resumedfield description "Whether the DAG run was re-enqueued for execution" (line 2883) could be more specific about the conditions under which this would befalse.Consider documenting scenarios such as:
- Was the DAG-run already completed when approval was granted?
- Are there other waiting steps that prevent immediate resumption?
- Could there be system conditions that prevent re-enqueueing?
This will help API consumers understand the approval lifecycle and handle edge cases appropriately.
📝 Suggested enhancement
resumed: type: boolean - description: "Whether the DAG run was re-enqueued for execution" + description: | + Whether the DAG run was successfully re-enqueued for execution after approval. + Returns false if the DAG-run cannot be resumed (e.g., already completed, + has other blocking conditions, or system constraints prevent enqueueing).
3186-3186: Naming inconsistency in x-enum-varnames between Status and NodeStatus.The x-enum-varnames differ between the two enums for value 7:
- Status (line 3195):
"Wait"- NodeStatus (line 3231):
"Waiting"While both use
"waiting"for the label (lines 3218, 3254), the inconsistent varnames may cause confusion when working with generated code or when mapping between DAG-run status and node status.Consider standardizing to either "Wait" or "Waiting" for both enums. "Waiting" might be clearer as it's more descriptive.
🔎 Proposed fix for consistency
Status: type: integer enum: [0, 1, 2, 3, 4, 5, 6, 7] x-enum-varnames: - "NotStarted" - "Running" - "Failed" - "Aborted" - "Success" - "Queued" - "PartialSuccess" - - "Wait" + - "Waiting"Also applies to: 3195-3195, 3222-3222, 3231-3231
3747-3757: Enhance documentation for the 'wait' executor configuration.The
executorConfigfield supports the new wait/approval functionality, but the schema usesadditionalProperties: truefor theconfigobject, which provides no type information about the expected structure for different executor types.Consider adding:
- Documentation or examples showing the structure of a 'wait' executor config
- Reference to how
requiredInputsare defined in the config (to connect with the approval endpoint error messages)- Examples of valid executorConfig for the 'wait' executor type
This would significantly improve the API's discoverability and reduce integration errors.
📝 Example documentation enhancement
Add to the executorConfig description:
executorConfig: type: object - description: "Executor configuration for this step" + description: | + Executor configuration for this step. Different executor types require different configuration structures. + + Example for 'wait' executor (human approval): + ```json + { + "type": "wait", + "config": { + "requiredInputs": ["approval_reason", "approved_by"], + "optionalInputs": ["comments"] + } + } + ``` + + The requiredInputs list determines which parameters must be provided when approving the step.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
api/v2/api.gen.goapi/v2/api.yamlinternal/core/dag.gointernal/core/dag_test.gointernal/core/execution/runstatus.gointernal/core/spec/dag.gointernal/core/spec/dag_test.gointernal/core/spec/loader_test.gointernal/core/step.gointernal/integration/approval_test.gointernal/integration/handler_on_test.gointernal/runtime/agent/agent.gointernal/runtime/agent/reporter.gointernal/runtime/agent/reporter_test.gointernal/runtime/builtin/dag/dag.gointernal/runtime/builtin/dag/parallel.gointernal/runtime/builtin/wait/wait_test.gointernal/runtime/data.gointernal/runtime/node.gointernal/runtime/runner.gointernal/runtime/runner_test.gointernal/runtime/transform/node.gointernal/runtime/transform/status.gointernal/service/frontend/api/v2/dagruns.gointernal/service/frontend/api/v2/dagruns_test.goui/src/api/v2/schema.tsui/src/features/dags/components/dag-details/NodeStatusTableRow.tsxui/src/features/dags/components/dag-execution/ApprovalModal.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/runtime/node.go
- internal/runtime/runner_test.go
- internal/service/frontend/api/v2/dagruns.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/runtime/data.gointernal/core/dag_test.gointernal/core/execution/runstatus.gointernal/core/step.gointernal/integration/approval_test.gointernal/core/spec/loader_test.gointernal/runtime/builtin/dag/dag.gointernal/runtime/builtin/dag/parallel.gointernal/integration/handler_on_test.gointernal/runtime/agent/agent.gointernal/core/spec/dag.gointernal/runtime/transform/status.gointernal/core/dag.gointernal/core/spec/dag_test.goapi/v2/api.gen.gointernal/runtime/transform/node.gointernal/runtime/agent/reporter.gointernal/service/frontend/api/v2/dagruns_test.gointernal/runtime/runner.gointernal/runtime/builtin/wait/wait_test.gointernal/runtime/agent/reporter_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/core/dag_test.gointernal/integration/approval_test.gointernal/core/spec/loader_test.gointernal/integration/handler_on_test.gointernal/core/spec/dag_test.gointernal/service/frontend/api/v2/dagruns_test.gointernal/runtime/builtin/wait/wait_test.gointernal/runtime/agent/reporter_test.go
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/features/dags/components/dag-execution/ApprovalModal.tsxui/src/features/dags/components/dag-details/NodeStatusTableRow.tsxui/src/api/v2/schema.ts
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/features/dags/components/dag-execution/ApprovalModal.tsxui/src/features/dags/components/dag-details/NodeStatusTableRow.tsxui/src/api/v2/schema.ts
{api/v1,api/v2}/**/*.{proto,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
API definitions live in
api/v1andapi/v2; generated server stubs land ininternal/service, while matching TypeScript clients flow intoui/src/api
Files:
api/v2/api.yaml
🧠 Learnings (2)
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Applied to files:
ui/src/features/dags/components/dag-execution/ApprovalModal.tsx
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/core/spec/dag_test.gointernal/runtime/builtin/wait/wait_test.go
🧬 Code graph analysis (16)
internal/core/execution/runstatus.go (2)
internal/core/execution/node.go (2)
NewNodeOrNil(56-61)Node(9-28)internal/core/dag.go (1)
HandlerOn(543-550)
internal/integration/approval_test.go (1)
internal/core/status.go (4)
Wait(14-14)NodeWaiting(69-69)NodeNotStarted(62-62)NodeSucceeded(66-66)
internal/core/spec/loader_test.go (2)
internal/core/spec/loader.go (1)
Load(150-171)internal/core/dag.go (1)
MailOn(553-557)
ui/src/features/dags/components/dag-execution/ApprovalModal.tsx (3)
api/v2/api.gen.go (1)
Step(983-1049)ui/src/api/v2/schema.ts (1)
components(1140-1982)ui/src/components/ui/button.tsx (1)
Button(57-57)
internal/runtime/builtin/dag/parallel.go (3)
internal/core/dag.go (1)
DAG(63-175)internal/core/step.go (1)
SubDAG(121-124)internal/core/status.go (4)
Status(4-4)Wait(14-14)Succeeded(11-11)PartiallySucceeded(13-13)
internal/integration/handler_on_test.go (3)
internal/core/dag.go (2)
DAG(63-175)HandlerOn(543-550)internal/core/status.go (3)
Wait(14-14)NodeSucceeded(66-66)NodeFailed(64-64)internal/core/execution/runstatus.go (1)
DAGRunStatus(36-61)
internal/runtime/agent/agent.go (5)
internal/runtime/transform/status.go (1)
WithOnWaitNode(119-125)internal/core/dag.go (1)
HandlerOn(543-550)api/v2/api.gen.go (1)
HandlerOn(583-595)api/v1/api.gen.go (1)
HandlerOn(358-370)internal/core/status.go (1)
Wait(14-14)
internal/core/spec/dag.go (2)
internal/core/status.go (1)
Wait(14-14)internal/core/dag.go (4)
MailOn(553-557)HandlerOn(543-550)HandlerOnWait(594-594)MailConfig(568-573)
ui/src/features/dags/components/dag-details/NodeStatusTableRow.tsx (1)
ui/src/features/dags/components/dag-execution/ApprovalModal.tsx (1)
ApprovalModal(31-162)
internal/runtime/transform/status.go (2)
internal/core/execution/runstatus.go (1)
DAGRunStatus(36-61)internal/runtime/data.go (1)
NodeData(23-26)
internal/core/dag.go (1)
internal/core/step.go (3)
ExecutorConfig(189-197)ExecutorTypeWait(324-324)Step(13-97)
internal/core/spec/dag_test.go (1)
internal/core/dag.go (2)
MailOn(553-557)HandlerOn(543-550)
api/v2/api.gen.go (3)
internal/core/status.go (2)
NodeStatus(59-59)Status(4-4)internal/core/step.go (1)
ExecutorConfig(189-197)internal/core/params.go (1)
Params(36-45)
internal/runtime/runner.go (4)
internal/core/status.go (7)
Wait(14-14)Status(4-4)NodeWaiting(69-69)NodeNotStarted(62-62)NodeRunning(63-63)NodeAborted(65-65)NodeSkipped(67-67)internal/core/dag.go (1)
HandlerOnWait(594-594)internal/runtime/env.go (2)
GetEnv(380-386)WithEnv(375-377)internal/core/execution/node.go (1)
Node(9-28)
internal/runtime/builtin/wait/wait_test.go (2)
internal/core/step.go (1)
ExecutorConfig(189-197)internal/core/status.go (1)
NodeWaiting(69-69)
internal/runtime/agent/reporter_test.go (2)
internal/core/dag.go (2)
MailConfig(568-573)MailOn(553-557)internal/core/status.go (1)
Wait(14-14)
🪛 Gitleaks (8.30.0)
api/v2/api.gen.go
[high] 10989-10990: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test on ubuntu-latest
🔇 Additional comments (86)
ui/src/features/dags/components/dag-execution/ApprovalModal.tsx (3)
1-26: LGTM!Type definitions and imports are well-structured. The
WaitConfigtype correctly models the optional configuration fields.
36-40: Config extraction handles edge cases well.The type assertion on line 37 is safe in practice due to the fallback defaults on lines 38-40. If
confighas an unexpected shape, the empty array/object fallbacks prevent runtime errors.
51-74: LGTM!Validation and error handling are well-implemented. The
isValidcheck correctly returnstruefor emptyrequiredFields, and thefinallyblock ensures loading state is always reset.ui/src/features/dags/components/dag-details/NodeStatusTableRow.tsx (5)
28-28: LGTM!Imports are correctly added for the new approval functionality.
Also applies to: 47-48
260-261: LGTM!State for the approval modal follows the existing pattern used for other modals in this component.
781-809: LGTM!The desktop approve button is well-implemented:
- Correctly renders only for
Waitingstatus- Uses
stopPropagationto prevent row expansion- Consistent styling with other action buttons
- Appropriate title for accessibility
934-941: LGTM!The
ApprovalModalis correctly placed alongside the existingStatusUpdateModal, maintaining the established modal management pattern.
1157-1217: LGTM!The mobile implementation is internally consistent:
- Uses plain
<button>elements matching the existing retry button style- Approve button correctly renders only for
WaitingstatusApprovalModalis properly scoped within thedagRunIdconditionalinternal/runtime/agent/reporter_test.go (2)
25-26: LGTM! Good test coverage for wait mail scenarios.The addition of "create wait mail" and "no wait mail" test scenarios provides comprehensive coverage for the new wait mail functionality, following the existing pattern for error and success mail tests.
228-255: LGTM! Wait mail test functions are well-structured.Both
testWaitMailandtestNoWaitMailfunctions follow the established testing pattern and correctly verify:
- Mail sending behavior based on MailOn.Wait flag
- Correct subject prefix ("Waiting")
- Proper mail count tracking
The tests provide good coverage for the wait mail feature.
internal/core/spec/loader_test.go (3)
173-186: LGTM! WaitMail configuration properly added to inheritance test.The WaitMail configuration is correctly added to both the base config and child DAG, following the same pattern as ErrorMail and InfoMail. This ensures that WaitMail inheritance and override behavior is tested alongside other mail configurations.
245-276: LGTM! Comprehensive test for WaitMail configuration.The test thoroughly validates:
- Full WaitMail configuration with all fields (From, To, Prefix, AttachLogs)
- Multiple recipients in list format
- MailOn.Wait flag integration
- Proper field values after loading
This provides strong coverage for the independent WaitMail loading scenario.
277-298: LGTM! Good edge case coverage for single recipient.The test correctly verifies that a single recipient specified as a string (rather than a list) is properly converted to a slice format. This ensures the WaitMail configuration handles both common YAML patterns for specifying recipients.
api/v2/api.gen.go (14)
61-71: WAIT status/value and label extension for nodes
NodeStatusWaiting = 7andNodeStatusLabelWaiting = "waiting"cleanly extend the existing status enum and label set without disturbing existing numeric values. This is consistent and safe for clients that already handle status codes as ints/strings.Also applies to: 73-83
104-114: WAIT status/value and label extension for DAG-runs
StatusWait = 7plusStatusLabelWaiting = "waiting"mirror the node status addition and keep the numeric/status-label mapping coherent across DAG runs and nodes. No issues from an API-surface perspective.Also applies to: 116-126
202-219: ApproveStepRequest/ApproveStepResponse shapes look correctThe request/response types for approving a waiting step are clear and minimal: an optional
inputsmap for env-like parameters, and a response containingdagRunId,stepName, and aresumedflag. This aligns well with the described HITL flow.
488-497: Status docs consistently extended to include “7: Waiting for approval”All affected status doc blocks (DAGRunDetails, DAGRunSummary, Node, NodeStatus, Status, SubDAGRunDetail, UpdateDAGRunStepStatusJSONBody, UpdateSubDAGRunStepStatusJSONBody) consistently document
7as “Waiting for approval”. This keeps the generated comments in sync with the new enum values and should help client generators.Also applies to: 549-558, 699-708, 729-738, 968-977, 1088-1097, 1490-1499, 1576-1585
999-1007: Step.ExecutorConfig API field matches executor conceptAdding
executorConfigwithtypeandconfigmatches the internalExecutorConfigconcept while omitting internal-only metadata. JSON naming and optionality (omitempty) look correct for the public API.
1461-1466: Approve*StepParams follow existing RemoteNode params pattern
ApproveDAGRunStepParamsandApproveSubDAGRunStepParamsonly carry optionalremoteNode, consistent with other*Paramstypes. This keeps the approve endpoints flexible for remote-node scenarios without overloading the path.Also applies to: 1547-1552
1886-1888: JSON body aliases for approve endpoints are wired correctlyThe
ApproveDAGRunStepJSONRequestBodyandApproveSubDAGRunStepJSONRequestBodyaliases toApproveStepRequesthook the shared request shape into the generated strict handlers as expected, reusing the same schema for both DAG and sub-DAG approvals.Also applies to: 1892-1894
2117-2119: ServerInterface extended with approve handlers in the right place
ApproveDAGRunStepandApproveSubDAGRunStepare added alongside the existing step-level and sub-DAG step-level operations, with consistent signatures and parameter ordering. This keeps the interface predictable for implementers.Also applies to: 2138-2140
2375-2379: Unimplemented stubs for approve endpoints are consistentThe new
Unimplemented.ApproveDAGRunStepandUnimplemented.ApproveSubDAGRunStepmethods simply return 501, matching the pattern used by all other unimplemented endpoints in this file.Also applies to: 2417-2421
3485-3545: Non-strict ServerInterfaceWrapper correctly binds approve requestsFor both approve endpoints:
- Path params (
name,dagRunId,stepName,subDAGRunId) are bound with the usualBindStyledParameterWithOptions.- Optional
remoteNodeis bound as a query param, same as other DAG-run operations.- Auth scope context keys (
ApiTokenScopes,BasicAuthScopes) are set in the same way as neighboring handlers.Body decoding is intentionally left to the
ServerInterfaceimplementation, consistent with the rest of the non-strict wrappers.If you want extra safety, you can add/extend an API test hitting these wrappers directly (through chi routing) to confirm all approve-path params bind as expected.
Also applies to: 3981-4050
5850-5852: Chi routes registered for approve endpoints in the expected locationsThe new
POSTroutes for:
/dag-runs/{name}/{dagRunId}/steps/{stepName}/approve/dag-runs/{name}/{dagRunId}/sub-dag-runs/{subDAGRunId}/steps/{stepName}/approveare registered immediately next to the other step/sub-step routes, using the correct wrappers. This keeps the routing table coherent and discoverable.
Also applies to: 5871-5873
6879-6928: Approve*Step strict request/response types look soundThe strict-mode request/response wiring for both DAG-run and sub-DAG-run approvals:
- Captures all relevant path params plus
Paramsand JSONBody.- Provides concrete
200,400,404, anddefaultJSON response variants, aligned with other step-level operations.- Implements the
Visit*Responsemethods following the existing pattern.Nothing stands out as inconsistent or error-prone in these shapes.
Also applies to: 7178-7227
8875-8877: StrictServerInterface extended cleanly with approve operations
ApproveDAGRunStepandApproveSubDAGRunStepare added toStrictServerInterfaceright beside related step-level methods, matching request/response object types and comments. This is what strict implementations will expect.Also applies to: 8896-8898
9599-9633: Strict handler implementations for approve endpoints follow established patternBoth strict handlers:
- Populate the corresponding
*RequestObjectwith path params,Params, and decoded JSONBody.- Run through the configured strict middlewares with a distinct operation name.
- Enforce response-type correctness by requiring an
Approve*StepResponseObjectand delegating toVisit*Response.This mirrors how other POST-with-body endpoints are implemented; no functional issues apparent.
Also applies to: 9815-9848
internal/runtime/builtin/dag/dag.go (3)
37-38: LGTM!The exported error variable follows Go conventions and provides a clear message about the restriction.
51-54: LGTM!The validation correctly prevents sub-DAGs with wait steps from being dispatched to workers, which is necessary since wait steps require human approval and cannot be executed remotely.
117-120: LGTM!The status mapping correctly propagates the waiting state from sub-DAGs to parent DAGs, enabling the human-in-the-loop approval flow across the DAG hierarchy.
internal/runtime/transform/status.go (1)
118-125: LGTM!The
WithOnWaitNodefunction correctly follows the established functional options pattern and is consistent with other handler options in this file.internal/core/spec/dag_test.go (2)
479-488: LGTM!The new test cases for
mailOn.Waitprovide good coverage, testing both isolated and combined scenarios.
739-762: LGTM!The wait handler test coverage is comprehensive, including:
- Handler presence and command validation
- Correct naming (
onWait)- Nil handling
- Error cases with empty commands
Also applies to: 805-805, 1868-1894
internal/core/step.go (1)
322-324: No issues found. The code contains only a singleExecutorTypeWaitconstant declaration at line 324.internal/runtime/transform/node.go (2)
42-42: LGTM!The
ApprovalInputsfield is correctly propagated from the execution layer to the runtime layer, consistent with other fields inToNode.
75-75: LGTM!The
ApprovalInputsfield is correctly propagated from the runtime state to the execution node, completing the bidirectional mapping for approval data.internal/service/frontend/api/v2/dagruns_test.go (4)
108-171: Well-structured test for the basic approval flow.The test properly validates the end-to-end approval workflow: starting a DAG, waiting for the Wait status, approving, and verifying completion. The assertions on
DagRunId,StepName, andResumedfields provide good coverage of the API response contract.
173-243: LGTM!Good coverage of the approval-with-inputs scenario. The test validates that approval inputs are properly passed through the API.
245-292: LGTM!Correctly tests the validation of required inputs during approval. Expecting
StatusBadRequestwhen required fields are missing is the appropriate behavior.
294-330: LGTM!Good negative test case - attempting to approve a step that's not in a waiting state should return a bad request. This validates proper state checking in the approval endpoint.
internal/runtime/agent/agent.go (2)
800-800: LGTM!The
OnWaithandler node is correctly included in the status options, following the same pattern as other handlers (OnInit,OnExit,OnSuccess,OnFailure,OnCancel).
951-953: LGTM!The
OnWaithandler is correctly wired in the runner configuration. The nil-check pattern is consistent with how other handlers (OnInit,OnExit,OnSuccess,OnFailure,OnCancel) are configured in this function.internal/core/execution/runstatus.go (4)
25-25: LGTM!The
OnWaitfield is correctly initialized fromdag.HandlerOn.WaitusingNewNodeOrNil, consistent with how other handler nodes are initialized inInitialStatus.
51-51: LGTM!The
OnWaitfield is correctly added to theDAGRunStatusstruct with appropriate JSON tagging (omitempty), consistent with other handler node fields.
94-96: LGTM!Error collection for
OnWaitfollows the established pattern used by other handlers, ensuring wait handler errors are properly surfaced.
122-124: LGTM!The
OnWaitnode lookup inNodeByNamefollows the same pattern as other handler nodes, enabling lookups by the wait handler's step name.internal/integration/handler_on_test.go (4)
198-221: LGTM!Well-structured test case validating that the wait handler executes when a DAG enters the Wait status. The setup verification (checking
dag.HandlerOn.Waitand handler name) and validation ofstatus.OnWaitare comprehensive.
222-242: LGTM!Important test case ensuring that a failing wait handler doesn't prevent the DAG from entering the Wait status. This correctly validates that the wait status takes precedence over handler failures, which is the expected behavior for HITL workflows.
706-745: LGTM!Good test coverage for the
DAG_WAITING_STEPSenvironment variable. This validates that the wait handler receives the names of steps that are waiting for approval.
747-785: LGTM!The test validates proper formatting of
DAG_WAITING_STEPSwith hyphenated step names, ensuring the environment variable propagation works correctly with various naming conventions.internal/runtime/builtin/dag/parallel.go (3)
61-64: Good validation to prevent invalid configurations.This correctly blocks sub-DAGs with wait steps from being dispatched to workers. Since wait steps require local storage access for approval handling, remote execution would fail. The early validation with a descriptive error improves user experience.
167-171: LGTM!Correctly excludes
core.Waitfrom the failure count. Wait is an expected intermediate state requiring human action, not a failure condition. The comment clearly explains the rationale.
208-232: LGTM!The wait status propagation logic is correct:
- Detects when any sub-DAG is in the Wait state
- Prioritizes
NodeWaitingoverpartialSuccesssince human action is required- Returns
NodeWaitingto bubble up the waiting state to the parent DAGThis ensures the HITL workflow properly propagates through nested DAG structures.
internal/core/dag.go (5)
128-129: LGTM!The
WaitMailconfiguration field follows the established pattern ofErrorMailandInfoMail, enabling email notifications when DAGs enter the wait status.
200-210: LGTM!The
HasWaitStepsmethod correctly scans top-level steps for the wait executor type. The documentation clearly explains why DAGs with wait steps cannot be dispatched to workers.
549-549: LGTM!The
Waithandler is correctly added to theHandlerOnstruct, following the pattern of other event handlers.
556-556: LGTM!The
Waitflag is correctly added toMailOn, enabling configuration of email notifications on wait status alongside existing failure and success triggers.
594-594: LGTM!The
HandlerOnWaitconstant follows the naming convention of existing handler types.internal/runtime/builtin/wait/wait_test.go (3)
13-81: Well-structured table-driven tests with proper parallel execution.Good use of table-driven tests following Go best practices. The test cases cover the basic, prompt, and inputs scenarios. Based on coding guidelines, the use of
stretchr/testify/requireandassertis appropriate.
84-105: Clean interface assertion pattern for DetermineNodeStatus.The type assertion approach to verify the executor implements the
NodeStatusDeterminerinterface is a solid pattern for testing optional interfaces.
107-159: Good coverage of validation edge cases including failure path.The test covers nil config, empty config, valid required fields, and invalid required fields (where required field is not in input list). This properly tests the error path as recommended by coding guidelines.
internal/integration/approval_test.go (2)
11-41: Good integration test coverage for wait step behavior.The test properly verifies that a wait step enters the
Waitstatus and blocks dependent nodes. The YAML-based DAG definition is clear and readable.
112-115: Helpful documentation about missing SubDAG test.Good practice to document why the SubDAGWithWaitStep test is not included and the current limitation. This helps future developers understand the feature's current state.
internal/runtime/agent/reporter.go (4)
76-86: Wait mail handling follows established patterns.The wait status mail handling mirrors the existing success and failure mail logic consistently. The conditional checks for
dag.MailOn.Waitanddag.WaitMailfollow the same pattern as other mail configurations.
199-199: Consistent CSS styling for waiting status.The amber/brown color scheme (
#92400e) for the waiting status is visually appropriate and distinguishable from other status colors.
376-380: Wait badge styling consistent with other status badges.The status badge for wait uses an appropriate amber color scheme that visually indicates a "pending action required" state.
596-601: Status handling in renderHTMLWithDAGInfo is complete.Both
partially_succeededandwaitingnode statuses are properly handled with appropriate CSS classes.internal/runtime/runner.go (7)
45-46: OnWait handler field properly integrated.The
onWaitfield is consistently added to both theRunnerstruct and wired through fromConfigin the constructor.
179-183: Wait condition detection is correctly positioned before deadlock check.The wait check correctly breaks the loop when there are no running nodes, no ready nodes, and waiting nodes exist. This check is appropriately placed before the deadlock detection to distinguish between a legitimate wait state and an actual deadlock.
279-311: OnWait handler execution with proper error handling.Good design choice to log errors from the onWait handler but not fail the overall DAG status. This ensures notification failures don't block the wait state. The
DAG_WAITING_STEPSenvironment variable provides useful context for the handler.
652-656: Wait status check correctly positioned before terminal state checks.Checking for waiting nodes before
isPartialSuccessandisErrorensures the Wait status takes precedence when nodes are waiting for approval.
764-770: NodeWaiting correctly blocks dependent nodes.The
isReadyfunction properly treatsNodeWaitingas a blocking state for dependent nodes, with appropriate debug logging.
1034-1036: NodeWaiting metric handling deferred to post-approval.The comment clarifies that waiting nodes are counted when they complete after approval, which is the correct behavior.
517-525: The approval input format is correct and consistent with the codebase.The
key+"="+valueformat matches the standard environment variable storage pattern used throughout the runtime. Line 548 explicitly documents this as the intended"KEY=evaluatedValue"format, andLoadOutputVariablesexpects variables in this format. No changes needed.internal/core/spec/dag.go (5)
68-69: WaitMail configuration field follows established pattern.The
WaitMailfield mirrors the existingErrorMailandInfoMailfields, maintaining consistency in the spec structure.
124-124: Wait handler added to handlerOn struct.Consistent with other event handlers (Init, Failure, Success, Cancel, Exit).
1227-1232: Wait handler building follows established handler pattern.The Wait handler construction mirrors the pattern used for Init, Exit, Success, Failure, and Cancel handlers.
1258-1260: DRY approach reusing buildMailConfigInternal.Good reuse of the existing
buildMailConfigInternalfunction rather than duplicating logic.
394-401: Good validation preventing worker dispatch with wait steps.This validation ensures DAGs with wait steps requiring human interaction cannot be dispatched to remote workers, which aligns with the PR objective to "prevent remote DAG waits." The
HasWaitSteps()method is properly implemented ininternal/core/dag.goand correctly checks for any wait executor steps. The error message is clear and actionable.ui/src/api/v2/schema.ts (5)
799-818: LGTM! Approval endpoints follow consistent API patterns.The new approval endpoints are well-structured and consistent with existing step-level operations. Documentation clearly describes HITL purpose and parameter passing via environment variables.
Also applies to: 899-918
1616-1624: Flexible executor configuration with intentional loose typing.The
executorConfigusesunknownfor config values, trading type safety for flexibility across different executor types (wait, http, docker, command). This is appropriate for a plugin-style architecture where each executor has unique configuration requirements.
5316-5317: Consistent Wait status added across all status enums.The Wait status (value=7, label="waiting") is properly added to all four status-related enums:
StatusandStatusLabelfor DAG-run levelNodeStatusandNodeStatusLabelfor step levelDocumentation at lines 1330 and 1349 correctly describes the new status as "Waiting for approval".
Also applies to: 5326-5327, 5336-5337, 5346-5347
4424-4484: Approval operations properly defined with appropriate error handling.The
approveDAGRunStepandapproveSubDAGRunStepoperations are well-structured:
- Optional request body accommodates approvals with or without input parameters
- Response includes
resumedflag indicating execution state- Error responses appropriately cover invalid state (400) and missing resources (404)
- Consistent with existing step operation patterns
Also applies to: 4714-4776
1-4: Auto-generated schema - source changes in OpenAPI spec.This file is auto-generated from
openapi-typescript. Any modifications to the API schema should be made in the source OpenAPI specification file (likelyapi/v2/api.yamlbased on PR context), not directly in this TypeScript file.The generated schema properly supports the new HITL approval workflow feature with:
- Approval endpoints for DAG-run and sub-DAG-run steps
- Request/response types for approval operations
- Wait status additions to all relevant enums
- Executor configuration support in Step schema
api/v2/api.yaml (2)
2233-2282: LGTM - Sub DAG-run approval endpoint is well-structured.The sub DAG-run approval endpoint correctly mirrors the parent DAG-run approval endpoint with the addition of the
subDAGRunIdpath parameter. The consistency between the two endpoints is good for API usability.
3184-3218: Status enum properly extended for wait status.The addition of value 7 for the waiting/approval state is correctly integrated into both the Status enum and StatusLabel. The naming is clear and the description accurately conveys the purpose.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1554 +/- ##
==========================================
+ Coverage 63.61% 63.69% +0.08%
==========================================
Files 224 226 +2
Lines 24928 25187 +259
==========================================
+ Hits 15857 16043 +186
- Misses 7595 7661 +66
- Partials 1476 1483 +7
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
internal/runtime/builtin/hitl/config.go (1)
15-22: Handle error frommapstructure.NewDecoder.The error returned by
NewDecoderon line 16 is silently ignored using the blank identifier. While construction errors are rare in typical usage, they should be checked to prevent unexpected runtime behavior.This issue was previously flagged in an earlier review. Please address it before merging:
🔎 Proposed fix
func decodeConfig(dat map[string]any, cfg *Config) error { - md, _ := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + md, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ WeaklyTypedInput: true, ErrorUnused: false, Result: cfg, }) + if err != nil { + return err + } return md.Decode(dat) }
🧹 Nitpick comments (14)
internal/runtime/plan_test.go (1)
364-405: Consider adding a test case for ordering consistency.The test validates correct names are returned but doesn't assert ordering. If
WaitingStepNames()guarantees a specific order (e.g., insertion order or alphabetical), consider adding a test case with multiple waiting nodes to verify ordering behavior.ui/src/features/dags/components/dag-execution/RejectionModal.tsx (2)
80-88: Add dark mode support to textarea per coding guidelines.The textarea styling lacks dark mode variants. Per coding guidelines, all UI components should support both light and dark modes using Tailwind CSS class pairs.
🔎 Proposed fix for dark mode support
<textarea id="rejection-reason" value={reason} onChange={(e) => setReason(e.target.value)} onKeyDown={handleKeyDown} - className="w-full px-3 py-2 text-sm border border-border rounded bg-background focus:outline-none focus:ring-2 focus:ring-primary/50 resize-none" + className="w-full px-3 py-2 text-sm border border-border rounded bg-background dark:bg-slate-800 focus:outline-none focus:ring-2 focus:ring-primary/50 resize-none" placeholder="Enter a reason for rejection..." rows={3} />
54-59: Consider accessibility: Enter in textarea should allow newlines.Currently, pressing Enter (without Shift) triggers rejection. This prevents users from entering multi-line reasons. Since the reason field is a textarea (implying multi-line input), consider removing the Enter-to-submit behavior or using a different trigger.
🔎 Option: Remove Enter key handling from textarea
- const handleKeyDown = (e: React.KeyboardEvent) => { - if (e.key === 'Enter' && !e.shiftKey && !loading) { - e.preventDefault(); - handleReject(); - } - }; // In textarea: - onKeyDown={handleKeyDown}Alternatively, if Enter-to-submit is desired, document that Shift+Enter creates newlines, and consider adding a hint in the placeholder.
internal/runtime/builtin/hitl/hitl_test.go (2)
84-105: Consider using the imported interface type directly.Instead of declaring an inline interface, you can use
executor.NodeStatusDeterminersince it's already defined and would make the test more maintainable.🔎 Proposed refactor
+import "github.com/dagu-org/dagu/internal/runtime/executor" - determiner, ok := exec.(interface { - DetermineNodeStatus() (core.NodeStatus, error) - }) + determiner, ok := exec.(executor.NodeStatusDeterminer) require.True(t, ok, "executor should implement NodeStatusDeterminer")
107-160: Consider adding a test case for config decode errors.The tests cover nil, empty, valid, and invalid required field scenarios. However, there's no test for malformed config that would cause
decodeConfigto fail. Adding such a case would improve coverage of the error path.🔎 Suggested additional test case
{ name: "InvalidConfigType", config: map[string]any{ "input": "not-a-slice", // should be []string }, expectErr: true, },ui/src/features/dags/components/dag-execution/ApprovalModal.tsx (1)
114-127: Consider adding dark mode support to input styling.The input field uses
border-borderandbg-backgroundwhich should work for both modes, but verify the focus ring color works well in dark mode.Per coding guidelines, ensure dark mode support with explicit class pairs if needed:
<input id={fieldId} type="text" value={inputs[field] || ''} onChange={(e) => setInputs((prev) => ({ ...prev, [field]: e.target.value, })) } onKeyDown={handleKeyDown} - className="w-full px-3 py-1 text-sm border border-border rounded bg-background focus:outline-none focus:ring-2 focus:ring-primary/50" + className="w-full px-3 py-1 text-sm border border-border rounded bg-background dark:bg-slate-800 focus:outline-none focus:ring-2 focus:ring-primary/50" placeholder={`Enter ${field}`} />ui/src/features/dags/components/dag-details/NodeStatusTableRow.tsx (1)
1029-1044: Consider extracting modal rendering to reduce duplication.The
ApprovalModalandRejectionModalare rendered identically in both desktop (lines 1031-1044) and mobile (lines 1364-1377) views. Since they're controlled by the same state and handlers, they could be rendered once outside the conditional view blocks.🔎 Potential refactor to reduce duplication
Move the modal rendering outside the view-specific blocks:
// At the end of the component, before the final return const modals = ( <> <ApprovalModal visible={showApprovalModal} dismissModal={() => setShowApprovalModal(false)} step={node.step} onApprove={handleApprove} /> <RejectionModal visible={showRejectionModal} dismissModal={() => setShowRejectionModal(false)} step={node.step} onReject={handleReject} /> </> ); // Then render once at component level instead of in both viewsThis reduces code duplication and ensures consistent behavior across views.
Also applies to: 1362-1377
api/v2/api.gen.go (3)
206-223: Approve/Reject step request/response shapes align with API goals
ApproveStepRequest/ResponseandRejectStepRequest/Response, plus their*DAGRunStepJSONRequestBodyaliases, match the described HITL behavior and follow existing style (pointer fields withomitempty, simple response payloads).One design consideration: for sub‑DAG variants you might also want to include
subDagRunIdin the response for easier client correlation, but that’s purely optional.Also applies to: 856-870, 1943-1948, 1952-1957
1024-1091: Step.ExecutorConfig wiring looks correctThe new
ExecutorConfignested struct onStepmirrors how other union-ish config blobs are modeled here (pointer to anonymous struct,map[string]interface{}for config,typediscriminator as string). JSON tags andomitemptyusage are correct.Follow‑up is mainly on validation: enforce allowed executor
typevalues and config schema in your service layer, not here in the generated types.
11551-11603: Swagger blob re-embed is expected; ensure it’s fully regenerated from specThe updated
swaggerSpecbase64/gzip payload is opaque here but is the canonical place the generator writes the OpenAPI spec. Just make sure this file was regenerated viaoapi-codegenfrom the updatedapi.yamlrather than hand‑edited so future regenerations remain consistent.api/v2/api.yaml (4)
2012-2055: Consider adding 409 Conflict response for idempotency.The approve endpoint lacks a 409 response code for cases where a step has already been approved or rejected. This would help clients distinguish between:
- 400: Invalid state (step not in Waiting status, or missing required inputs)
- 409: Idempotent conflict (step already processed)
Adding this would improve API clarity and client error handling.
Suggested addition
"404": description: "DAG-run or step not found" content: application/json: schema: $ref: "#/components/schemas/Error" + "409": + description: "Step already approved or rejected" + content: + application/json: + schema: + $ref: "#/components/schemas/Error" default: description: "Generic error response"
2056-2099: Consider adding 409 Conflict response for idempotency.Similar to the approve endpoint, the reject endpoint lacks a 409 response for cases where a step has already been approved or rejected, which would improve API clarity for idempotent operations.
2991-3003: RejectStepResponse missing execution state indicator.
RejectStepResponselacks a field indicating the execution state after rejection, unlikeApproveStepResponsewhich includes aresumedboolean. Consider adding a field (e.g.,terminated: booleanorstatus: string) to inform clients whether the DAG run was terminated, continued, or entered a failed state after rejection.Suggested addition
RejectStepResponse: type: object description: "Response after rejecting a waiting step" properties: dagRunId: type: string description: "The DAG run ID" stepName: type: string description: "The rejected step name" + terminated: + type: boolean + description: "Whether the DAG run was terminated after rejection" required: - dagRunId - stepName + - terminated
3889-3899: Consider constraining executor type values.The
executorConfig.typefield is a free-form string, which may lead to typos or invalid executor types being specified. If the set of supported executor types is finite and known, consider using an enum to provide validation and improve API documentation.If extensibility is required, document that custom executor types are supported and how they should be registered.
Example with enum constraint
executorConfig: type: object description: "Executor configuration for this step" properties: type: type: string + enum: ["wait", "http", "docker", "command"] description: "Type of executor (e.g., 'wait', 'http', 'docker', 'command')" config: type: object description: "Executor-specific configuration" additionalProperties: true
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
api/v2/api.gen.goapi/v2/api.yamlinternal/common/telemetry/collector.gointernal/core/dag.gointernal/core/dag_test.gointernal/core/execution/node.gointernal/core/execution/runstatus.gointernal/core/spec/dag.gointernal/core/spec/dag_test.gointernal/core/spec/loader_test.gointernal/core/status.gointernal/core/step.gointernal/integration/approval_test.gointernal/integration/handler_on_test.gointernal/output/tree.gointernal/persistence/filedagrun/dataroot_test.gointernal/runtime/agent/agent.gointernal/runtime/agent/reporter.gointernal/runtime/agent/reporter_test.gointernal/runtime/builtin/builtin.gointernal/runtime/builtin/dag/dag.gointernal/runtime/builtin/dag/parallel.gointernal/runtime/builtin/hitl/config.gointernal/runtime/builtin/hitl/hitl.gointernal/runtime/builtin/hitl/hitl_test.gointernal/runtime/data.gointernal/runtime/node.gointernal/runtime/plan.gointernal/runtime/plan_test.gointernal/runtime/runner.gointernal/runtime/runner_helper_test.gointernal/runtime/runner_test.gointernal/runtime/transform/node.gointernal/runtime/transform/node_test.gointernal/runtime/transform/status.gointernal/service/frontend/api/v2/dagruns.gointernal/service/frontend/api/v2/dagruns_test.gointernal/service/frontend/api/v2/transformer.gointernal/service/frontend/api/v2/validation_test.gointernal/service/frontend/auth/middleware.gointernal/service/frontend/auth/oidc.goschemas/dag.schema.jsonui/src/api/v2/schema.tsui/src/consts.tsui/src/features/dags/components/common/NodeStatusChip.tsxui/src/features/dags/components/common/StatusDot.tsxui/src/features/dags/components/common/index.tsui/src/features/dags/components/common/statusLabels.tsui/src/features/dags/components/dag-details/DAGStatusOverview.tsxui/src/features/dags/components/dag-details/NodeStatusTableRow.tsxui/src/features/dags/components/dag-details/SubDAGRunsList.tsxui/src/features/dags/components/dag-execution/ApprovalModal.tsxui/src/features/dags/components/dag-execution/ParallelExecutionModal.tsxui/src/features/dags/components/dag-execution/RejectionModal.tsxui/src/features/dags/components/visualization/Graph.tsxui/src/features/dags/components/visualization/TimelineChart.tsxui/src/pages/dag-runs/index.tsxui/src/pages/index.tsxui/src/ui/StatusChip.tsx
🚧 Files skipped from review as they are similar to previous changes (16)
- internal/core/execution/runstatus.go
- internal/output/tree.go
- internal/runtime/transform/status.go
- internal/runtime/agent/agent.go
- ui/src/features/dags/components/common/StatusDot.tsx
- ui/src/features/dags/components/dag-execution/ParallelExecutionModal.tsx
- internal/common/telemetry/collector.go
- internal/runtime/builtin/builtin.go
- internal/core/dag_test.go
- internal/core/execution/node.go
- internal/core/spec/loader_test.go
- ui/src/features/dags/components/dag-details/SubDAGRunsList.tsx
- internal/core/spec/dag.go
- internal/integration/approval_test.go
- ui/src/features/dags/components/visualization/Graph.tsx
- ui/src/features/dags/components/visualization/TimelineChart.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/core/spec/dag_test.gointernal/service/frontend/auth/middleware.gointernal/service/frontend/api/v2/dagruns_test.gointernal/core/step.gointernal/integration/handler_on_test.gointernal/runtime/data.gointernal/runtime/builtin/dag/parallel.gointernal/runtime/builtin/hitl/hitl_test.gointernal/service/frontend/api/v2/validation_test.gointernal/runtime/agent/reporter_test.gointernal/runtime/agent/reporter.gointernal/runtime/transform/node.gointernal/runtime/transform/node_test.gointernal/runtime/runner.gointernal/runtime/builtin/hitl/config.gointernal/core/dag.gointernal/runtime/runner_helper_test.gointernal/core/status.gointernal/runtime/plan_test.gointernal/runtime/builtin/dag/dag.gointernal/service/frontend/auth/oidc.gointernal/persistence/filedagrun/dataroot_test.gointernal/runtime/node.gointernal/service/frontend/api/v2/dagruns.gointernal/runtime/plan.goapi/v2/api.gen.gointernal/service/frontend/api/v2/transformer.gointernal/runtime/builtin/hitl/hitl.gointernal/runtime/runner_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/core/spec/dag_test.gointernal/service/frontend/api/v2/dagruns_test.gointernal/integration/handler_on_test.gointernal/runtime/builtin/hitl/hitl_test.gointernal/service/frontend/api/v2/validation_test.gointernal/runtime/agent/reporter_test.gointernal/runtime/transform/node_test.gointernal/runtime/runner_helper_test.gointernal/runtime/plan_test.gointernal/persistence/filedagrun/dataroot_test.gointernal/runtime/runner_test.go
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/features/dags/components/dag-execution/RejectionModal.tsxui/src/features/dags/components/common/index.tsui/src/features/dags/components/common/statusLabels.tsui/src/consts.tsui/src/features/dags/components/common/NodeStatusChip.tsxui/src/pages/dag-runs/index.tsxui/src/pages/index.tsxui/src/features/dags/components/dag-details/NodeStatusTableRow.tsxui/src/features/dags/components/dag-execution/ApprovalModal.tsxui/src/features/dags/components/dag-details/DAGStatusOverview.tsxui/src/api/v2/schema.tsui/src/ui/StatusChip.tsx
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/features/dags/components/dag-execution/RejectionModal.tsxui/src/features/dags/components/common/index.tsui/src/features/dags/components/common/statusLabels.tsui/src/consts.tsui/src/features/dags/components/common/NodeStatusChip.tsxui/src/pages/dag-runs/index.tsxui/src/pages/index.tsxui/src/features/dags/components/dag-details/NodeStatusTableRow.tsxui/src/features/dags/components/dag-execution/ApprovalModal.tsxui/src/features/dags/components/dag-details/DAGStatusOverview.tsxui/src/api/v2/schema.tsui/src/ui/StatusChip.tsx
{api/v1,api/v2}/**/*.{proto,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
API definitions live in
api/v1andapi/v2; generated server stubs land ininternal/service, while matching TypeScript clients flow intoui/src/api
Files:
api/v2/api.yaml
🧠 Learnings (5)
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Use consistent metadata styling with `bg-slate-200 dark:bg-slate-700` backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Applied to files:
ui/src/consts.tsui/src/ui/StatusChip.tsx
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: `make test` (or `make test-coverage`) executes the Go suite via `gotestsum`; append `TEST_TARGET=./internal/...` to focus packages
Applied to files:
internal/runtime/builtin/hitl/hitl_test.go
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/runtime/builtin/hitl/hitl_test.gointernal/service/frontend/api/v2/validation_test.gointernal/runtime/transform/node_test.go
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/runtime/builtin/hitl/hitl_test.gointernal/service/frontend/api/v2/validation_test.go
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Applied to files:
ui/src/features/dags/components/dag-execution/ApprovalModal.tsx
🧬 Code graph analysis (29)
internal/core/spec/dag_test.go (1)
internal/core/dag.go (2)
MailOn(561-565)HandlerOn(551-558)
internal/service/frontend/auth/middleware.go (4)
internal/auth/user.go (1)
User(10-24)internal/auth/role.go (2)
Role(13-13)RoleAdmin(17-17)internal/auth/context.go (1)
WithUser(14-16)internal/core/execution/context.go (1)
WithContext(245-247)
ui/src/features/dags/components/dag-execution/RejectionModal.tsx (2)
ui/src/api/v2/schema.ts (1)
components(1180-2050)ui/src/components/ui/button.tsx (1)
Button(57-57)
ui/src/features/dags/components/common/statusLabels.ts (2)
ui/src/features/dags/components/common/index.ts (1)
STATUS_DISPLAY_LABELS(15-15)api/v2/api.gen.go (1)
StatusLabel(1022-1022)
internal/integration/handler_on_test.go (2)
internal/core/dag.go (2)
DAG(63-178)HandlerOn(551-558)internal/core/status.go (4)
Waiting(14-14)Status(4-4)NodeSucceeded(69-69)NodeFailed(67-67)
ui/src/features/dags/components/common/NodeStatusChip.tsx (3)
internal/core/status.go (1)
NodeStatus(62-62)api/v2/api.gen.go (1)
NodeStatus(764-764)api/v1/api.gen.go (1)
NodeStatus(444-444)
ui/src/pages/dag-runs/index.tsx (5)
internal/core/status.go (1)
Status(4-4)api/v2/api.gen.go (1)
Status(1019-1019)internal/cmd/status.go (1)
Status(15-37)internal/persistence/legacy/model/status.go (1)
Status(30-46)ui/src/components/ui/select.tsx (1)
SelectItem(178-178)
ui/src/pages/index.tsx (4)
internal/core/status.go (1)
Status(4-4)api/v2/api.gen.go (1)
Status(1019-1019)internal/cmd/status.go (1)
Status(15-37)internal/persistence/legacy/model/status.go (1)
Status(30-46)
internal/runtime/builtin/dag/parallel.go (3)
internal/core/dag.go (1)
DAG(63-178)internal/core/step.go (1)
SubDAG(127-130)internal/core/status.go (4)
Status(4-4)Waiting(14-14)Succeeded(11-11)PartiallySucceeded(13-13)
internal/runtime/builtin/hitl/hitl_test.go (3)
internal/core/step.go (1)
ExecutorConfig(195-203)internal/runtime/builtin/hitl/config.go (1)
Config(6-13)internal/core/status.go (1)
NodeWaiting(72-72)
ui/src/features/dags/components/dag-details/NodeStatusTableRow.tsx (3)
ui/src/components/ui/button.tsx (1)
Button(57-57)ui/src/features/dags/components/dag-execution/ApprovalModal.tsx (1)
ApprovalModal(31-172)ui/src/features/dags/components/dag-execution/RejectionModal.tsx (1)
RejectionModal(25-122)
internal/runtime/agent/reporter_test.go (2)
internal/core/dag.go (3)
MailConfig(576-581)DAG(63-178)MailOn(561-565)internal/core/status.go (2)
Status(4-4)Waiting(14-14)
internal/runtime/agent/reporter.go (3)
internal/core/status.go (2)
Status(4-4)Waiting(14-14)internal/persistence/legacy/model/status.go (1)
Status(30-46)internal/core/dag.go (1)
MailOn(561-565)
internal/runtime/transform/node.go (1)
internal/cmd/dagpicker/dagpicker.go (1)
State(22-22)
internal/runtime/transform/node_test.go (5)
internal/common/collections/syncmap.go (1)
SyncMap(10-14)internal/core/execution/node.go (2)
Node(9-36)SubDAGRun(39-42)internal/runtime/data.go (2)
SubDAGRun(101-122)NodeData(24-27)internal/runtime/transform/node.go (1)
ToNode(12-49)internal/runtime/transform/status.go (2)
NewStatusBuilder(17-19)WithNodes(33-41)
ui/src/features/dags/components/dag-details/DAGStatusOverview.tsx (4)
internal/core/status.go (1)
Status(4-4)api/v2/api.gen.go (1)
Status(1019-1019)internal/cmd/status.go (1)
Status(15-37)internal/service/scheduler/scheduler.go (1)
Clock(37-37)
internal/runtime/runner.go (4)
internal/core/status.go (9)
Rejected(15-15)Waiting(14-14)Status(4-4)Running(8-8)NodeWaiting(72-72)NodeRejected(73-73)NodeNotStarted(65-65)NodeRunning(66-66)NodeSkipped(70-70)internal/core/dag.go (2)
HandlerOnFailure(599-599)HandlerOnWait(602-602)internal/runtime/env.go (2)
GetEnv(380-386)WithEnv(375-377)internal/core/execution/node.go (1)
Node(9-36)
internal/core/dag.go (1)
internal/core/step.go (2)
ExecutorConfig(195-203)ExecutorTypeHITL(330-330)
internal/core/status.go (4)
api/v2/api.gen.go (2)
Status(1019-1019)NodeStatus(764-764)internal/cmd/status.go (1)
Status(15-37)internal/persistence/legacy/model/status.go (1)
Status(30-46)api/v1/api.gen.go (1)
NodeStatus(444-444)
internal/runtime/plan_test.go (5)
internal/runtime/node.go (2)
NodeWithData(51-55)Node(33-43)internal/runtime/data.go (3)
NodeData(24-27)NodeState(29-81)Parallel(85-89)internal/core/status.go (7)
Status(4-4)NodeRejected(73-73)NodeSucceeded(69-69)NodeAborted(68-68)NodeNotStarted(65-65)NodeRunning(66-66)NodeWaiting(72-72)internal/core/execution/node.go (1)
Node(9-36)internal/runtime/plan.go (2)
CreateRetryPlan(66-95)NewPlanFromNodes(98-118)
internal/runtime/builtin/dag/dag.go (3)
internal/core/dag.go (1)
DAG(63-178)internal/core/step.go (1)
SubDAG(127-130)internal/core/status.go (2)
Waiting(14-14)NodeWaiting(72-72)
internal/service/frontend/auth/oidc.go (2)
internal/auth/role.go (1)
RoleAdmin(17-17)internal/auth/context.go (1)
WithUser(14-16)
internal/persistence/filedagrun/dataroot_test.go (4)
internal/persistence/filedagrun/dagrun_test.go (1)
DAGRunTest(41-45)internal/core/status.go (3)
Status(4-4)Succeeded(11-11)Waiting(14-14)internal/core/execution/runstatus.go (1)
DAGRunStatus(36-61)internal/common/fileutil/fileutil.go (1)
FileExists(44-47)
internal/runtime/node.go (1)
internal/core/status.go (4)
NodeRejected(73-73)NodeNotStarted(65-65)NodeRunning(66-66)NodeWaiting(72-72)
internal/runtime/plan.go (2)
internal/core/execution/node.go (1)
Node(9-36)internal/core/status.go (10)
NodeAborted(68-68)NodeRejected(73-73)Status(4-4)NodeRunning(66-66)NodeWaiting(72-72)NodeNotStarted(65-65)NodeSucceeded(69-69)NodeFailed(67-67)NodeSkipped(70-70)NodePartiallySucceeded(71-71)
internal/service/frontend/api/v2/transformer.go (1)
internal/core/step.go (1)
ExecutorConfig(195-203)
internal/runtime/builtin/hitl/hitl.go (4)
internal/runtime/executor/executor.go (2)
NodeStatusDeterminer(57-59)RegisterExecutor(41-47)internal/core/step.go (2)
Step(13-103)ExecutorConfig(195-203)internal/runtime/builtin/hitl/config.go (1)
Config(6-13)internal/core/status.go (1)
NodeWaiting(72-72)
ui/src/ui/StatusChip.tsx (4)
internal/core/status.go (1)
Status(4-4)api/v2/api.gen.go (1)
Status(1019-1019)internal/cmd/status.go (1)
Status(15-37)internal/persistence/legacy/model/status.go (1)
Status(30-46)
internal/runtime/runner_test.go (1)
internal/core/status.go (7)
Status(4-4)NodeRejected(73-73)NodeWaiting(72-72)Rejected(15-15)Waiting(14-14)NodeSucceeded(69-69)NodeNotStarted(65-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test on ubuntu-latest
| approvedAt: | ||
| type: string | ||
| description: "RFC3339 timestamp when the HITL step was approved" | ||
| approvedBy: | ||
| type: string | ||
| description: "Username of who approved the HITL step" | ||
| approvalInputs: | ||
| type: object | ||
| additionalProperties: | ||
| type: string | ||
| description: "Key-value inputs provided during HITL approval" | ||
| rejectedAt: | ||
| type: string | ||
| description: "RFC3339 timestamp when the HITL step was rejected" | ||
| rejectedBy: | ||
| type: string | ||
| description: "Username of who rejected the HITL step" | ||
| rejectionReason: | ||
| type: string | ||
| description: "Optional reason for rejection" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how user identification is stored in approval/rejection handling
rg -nC5 'approvedBy|rejectedBy' --type=go -g '!vendor/**' -g '!api/**'Repository: dagu-org/dagu
Length of output: 1351
🏁 Script executed:
#!/bin/bash
# Search for user identification patterns in auth/user handling
rg -nC3 'username|user.*id' --type=go -g '**/auth/**' -g '**/user/**' | head -100Repository: dagu-org/dagu
Length of output: 6715
🏁 Script executed:
#!/bin/bash
# Check the actual API spec to verify field descriptions
# Look at the referenced lines and surrounding context
grep -n "createdBy\|approvedBy\|rejectedBy" api/v2/api.yaml | head -20Repository: dagu-org/dagu
Length of output: 179
🏁 Script executed:
#!/bin/bash
# Get more context around the fields mentioned in the review
sed -n '3095,3105p' api/v2/api.yaml
sed -n '4435,4445p' api/v2/api.yaml
sed -n '3748,3767p' api/v2/api.yamlRepository: dagu-org/dagu
Length of output: 1461
Ensure consistent terminology for user identification fields across the API spec.
The descriptions for approvedBy and rejectedBy state "Username of who..." while similar fields elsewhere in the spec (e.g., WebhookDetails.createdBy at line 3100, APIKey.createdBy at line 4441) use "User ID who...". The backend confirms these fields store usernames, but this inconsistent terminology across the API spec may confuse consumers. Standardize the language: either describe all such fields as "Username" or "User ID" depending on what they actually contain.
🤖 Prompt for AI Agents
In api/v2/api.yaml around lines 3748 to 3767, the descriptions for approvedBy
and rejectedBy use "Username of who..." which is inconsistent with other fields
that say "User ID who..."; the backend confirms these fields store usernames, so
update the descriptions to a consistent phrasing like "Username of the user who
approved the HITL step" and "Username of the user who rejected the HITL step"
(also search and align any other similar fields to use "Username" if they
actually store usernames).
| // IsActive checks if the status is active (not yet completed). | ||
| // This includes Running, Queued, and Wait (waiting for human approval). | ||
| func (s Status) IsActive() bool { | ||
| return s == Running || s == Queued | ||
| return s == Running || s == Queued || s == Waiting | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the status.go file to understand the Status type and IsActive() method
cd /
find . -name "status.go" -path "*/internal/core/*" 2>/dev/null | head -5Repository: dagu-org/dagu
Length of output: 100
🏁 Script executed:
# Find the file and examine its content
cat -n internal/core/status.goRepository: dagu-org/dagu
Length of output: 3189
🏁 Script executed:
# Search for all usages of IsActive() in the codebase to understand the impact
rg -n --type=go '\bIsActive\s*\(' -A 3 -B 1Repository: dagu-org/dagu
Length of output: 3060
🏁 Script executed:
# Also search for Waiting constant definition to understand its semantics
rg -n --type=go '\bWaiting\s*=' -B 2 -A 2Repository: dagu-org/dagu
Length of output: 39
🏁 Script executed:
# Look at the test for IsActive() to see expected behavior
sed -n '1220,1240p' internal/runtime/runner_test.goRepository: dagu-org/dagu
Length of output: 507
Update comment from "Wait" to "Waiting" for consistency.
Line 46 mentions "Wait" but the actual status constant is Waiting. Also, the test file internal/runtime/runner_test.go (lines 1220–1238) does not include a test case for the Waiting status in TestStatus_IsActive(), despite the method now returning true for it. Consider adding:
{core.Waiting, true},
All production code callers of IsActive() correctly handle Waiting as an active (non-terminal) state—the behavioral change is semantically sound.
🤖 Prompt for AI Agents
In internal/core/status.go around lines 45 to 49, update the comment to refer to
"Waiting" (not "Wait") to match the Status constant, and add a unit test in
internal/runtime/runner_test.go TestStatus_IsActive() to include the missing
case for Waiting (e.g., {core.Waiting, true}) so the test suite verifies that
Waiting is treated as active; ensure the existing callers remain unchanged.
| createAttemptWithStatusType := func(dagRunTest DAGRunTest, ts time.Time, status core.Status) *Attempt { | ||
| attempt, err := dagRunTest.CreateAttempt(root.Context, execution.NewUTC(ts), nil) | ||
| require.NoError(t, err) | ||
| require.NoError(t, attempt.Open(root.Context)) | ||
| dagStatus := execution.DAGRunStatus{ | ||
| Name: "test-dag", | ||
| DAGRunID: dagRunTest.dagRunID, | ||
| Status: status, | ||
| } | ||
| require.NoError(t, attempt.Write(root.Context, dagStatus)) | ||
| require.NoError(t, attempt.Close(root.Context)) | ||
|
|
||
| // Set the file modification time to match the old timestamp | ||
| err = os.Chtimes(attempt.file, ts, ts) | ||
| require.NoError(t, err) | ||
|
|
||
| return attempt | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor duplicated helper to reduce code duplication.
The createAttemptWithStatusType helper is nearly identical to createAttemptWithStatus used in lines 158-175, 205-222, and 252-269. The only difference is the status parameter.
Extract a shared helper at the file level to eliminate ~60 lines of duplication:
🔎 Proposed refactor
Add this shared helper after the setupTestDataRoot function:
// createTestAttemptWithStatus creates an attempt with the specified status and timestamp.
// It handles opening, writing status data, closing, and setting the file modification time.
func createTestAttemptWithStatus(t *testing.T, ctx context.Context, dagRunTest DAGRunTest, ts time.Time, status core.Status) *Attempt {
t.Helper()
attempt, err := dagRunTest.CreateAttempt(ctx, execution.NewUTC(ts), nil)
require.NoError(t, err)
require.NoError(t, attempt.Open(ctx))
dagStatus := execution.DAGRunStatus{
Name: "test-dag",
DAGRunID: dagRunTest.dagRunID,
Status: status,
}
require.NoError(t, attempt.Write(ctx, dagStatus))
require.NoError(t, attempt.Close(ctx))
// Set the file modification time to match the timestamp
err = os.Chtimes(attempt.file, ts, ts)
require.NoError(t, err)
return attempt
}Then replace all local createAttemptWithStatus and createAttemptWithStatusType functions with calls to this shared helper:
- createAttemptWithStatus := func(dagRunTest DAGRunTest, ts time.Time) *Attempt {
- attempt, err := dagRunTest.CreateAttempt(root.Context, execution.NewUTC(ts), nil)
- require.NoError(t, err)
- require.NoError(t, attempt.Open(root.Context))
- status := execution.DAGRunStatus{
- Name: "test-dag",
- DAGRunID: dagRunTest.dagRunID,
- Status: core.Succeeded,
- }
- require.NoError(t, attempt.Write(root.Context, status))
- require.NoError(t, attempt.Close(root.Context))
-
- // Set the file modification time to match the old timestamp
- err = os.Chtimes(attempt.file, ts, ts)
- require.NoError(t, err)
-
- return attempt
- }
-
- createAttemptWithStatus(dagRun1, ts1)
- createAttemptWithStatus(dagRun2, ts2)
+ createTestAttemptWithStatus(t, root.Context, dagRun1, ts1, core.Succeeded)
+ createTestAttemptWithStatus(t, root.Context, dagRun2, ts2, core.Succeeded)Apply similar changes to all test cases (lines 158-175, 205-222, 252-269, 302-319).
Based on coding guidelines, which recommend using shared fixtures instead of duplicating mocks.
Committable suggestion skipped: line range outside the PR's diff.
| .status-skipped { color: #6b7280; font-weight: 500; } | ||
| .status-aborted { color: #db2777; font-weight: 500; } | ||
| .status-partial-success { color: #ea580c; font-weight: 500; } | ||
| .status-waiting { color: #92400e; font-weight: 500; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.go" | grep -E "(core|status)" | head -20Repository: dagu-org/dagu
Length of output: 747
🏁 Script executed:
ls -la internal/Repository: dagu-org/dagu
Length of output: 841
🏁 Script executed:
find . -type f -name "*.go" | xargs grep -l "type.*Status.*struct" | head -10Repository: dagu-org/dagu
Length of output: 293
🏁 Script executed:
cat -n internal/core/execution/runstatus.go | head -100Repository: dagu-org/dagu
Length of output: 4588
🏁 Script executed:
# Search for String() methods for Status types
rg -n "func.*String\(\)" internal/core/execution/runstatus.go -A 2Repository: dagu-org/dagu
Length of output: 124
🏁 Script executed:
# Look at the reporter.go file, especially around the lines mentioned
head -20 internal/runtime/agent/reporter.goRepository: dagu-org/dagu
Length of output: 571
🏁 Script executed:
# Find Status type definition
rg -n "type Status" --type go | head -20Repository: dagu-org/dagu
Length of output: 556
🏁 Script executed:
# Find NodeStatus type definition
rg -n "type NodeStatus" --type go | head -20Repository: dagu-org/dagu
Length of output: 365
🏁 Script executed:
# Look at reporter.go around line 199-601 where the comparisons are
wc -l internal/runtime/agent/reporter.goRepository: dagu-org/dagu
Length of output: 95
🏁 Script executed:
cat -n internal/core/status.goRepository: dagu-org/dagu
Length of output: 3189
🏁 Script executed:
# Look at reporter.go around the relevant lines
sed -n '190,210p' internal/runtime/agent/reporter.go
sed -n '250,270p' internal/runtime/agent/reporter.go
sed -n '480,500p' internal/runtime/agent/reporter.go
sed -n '590,610p' internal/runtime/agent/reporter.goRepository: dagu-org/dagu
Length of output: 3156
🏁 Script executed:
# Get more context around line 490
sed -n '475,510p' internal/runtime/agent/reporter.goRepository: dagu-org/dagu
Length of output: 1096
🏁 Script executed:
# Search for where "wait" string might be set or used in reporter.go
rg -n '"wait"' internal/runtime/agent/reporter.go -B 3 -A 3Repository: dagu-org/dagu
Length of output: 270
Fix status string mismatch: line 490 checks "wait" but Status.String() returns "waiting".
The DAG-level status rendering at line 490 will never match a waiting DAG status because:
core.Waiting.String()returns"waiting"(confirmed ininternal/core/status.goline 36-37)- The switch statement checks for
case "wait"instead ofcase "waiting"
This is inconsistent with the node-level rendering at lines 260 and 601, which correctly check for "waiting". Change line 490 to:
case "waiting":
statusClass = "wait"
Also note: the switch statement is missing cases for "not_started", "queued", "partially_succeeded", and "rejected" status values.
🤖 Prompt for AI Agents
In internal/runtime/agent/reporter.go around line 199 (and the switch at ~490),
update the DAG-level status switch so it checks for "waiting" (not "wait") and
sets statusClass = "wait"; additionally add missing switch cases for
"not_started", "queued", "partially_succeeded", and "rejected" to mirror the
node-level rendering mappings used at lines ~260 and ~601 — copy those same
case-to-statusClass assignments so DAG rendering matches node rendering.
| func TestNodeFieldsRoundTrip(t *testing.T) { | ||
| outputVars := &collections.SyncMap{} | ||
| outputVars.Store("KEY", "KEY=value") | ||
|
|
||
| original := &execution.Node{ | ||
| Step: core.Step{Name: "test-step"}, | ||
| Status: core.NodeSucceeded, | ||
| Stdout: "/tmp/stdout.log", | ||
| Stderr: "/tmp/stderr.log", | ||
| StartedAt: "2024-01-15T10:00:00Z", | ||
| FinishedAt: "2024-01-15T10:05:00Z", | ||
| RetriedAt: "2024-01-15T10:01:00Z", | ||
| RetryCount: 2, | ||
| DoneCount: 3, | ||
| Repeated: true, | ||
| Error: "test error", | ||
| SubRuns: []execution.SubDAGRun{{DAGRunID: "sub-1", Params: "p1"}}, | ||
| SubRunsRepeated: []execution.SubDAGRun{{DAGRunID: "sub-2", Params: "p2"}}, | ||
| OutputVariables: outputVars, | ||
| ApprovalInputs: map[string]string{"input1": "value1"}, | ||
| ApprovedAt: "2024-01-15T10:02:00Z", | ||
| ApprovedBy: "admin", | ||
| RejectedAt: "2024-01-15T10:03:00Z", | ||
| RejectedBy: "reviewer", | ||
| RejectionReason: "test rejection reason", | ||
| } | ||
|
|
||
| // Round-trip: execution.Node -> runtime.Node -> execution.Node | ||
| runtimeNode := transform.ToNode(original) | ||
| state := runtimeNode.State() | ||
|
|
||
| dag := &core.DAG{Name: "test", Steps: []core.Step{original.Step}} | ||
| status := transform.NewStatusBuilder(dag).Create("run-1", core.Succeeded, 0, time.Now(), | ||
| transform.WithNodes([]runtime.NodeData{{Step: original.Step, State: state}})) | ||
|
|
||
| result := status.Nodes[0] | ||
|
|
||
| // OutputVariables is a pointer, compare separately | ||
| assert.Equal(t, original.OutputVariables, result.OutputVariables) | ||
|
|
||
| // Compare rest of the struct | ||
| original.OutputVariables = nil | ||
| result.OutputVariables = nil | ||
| assert.Equal(t, original, result) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Comprehensive round-trip test, but use require for assertions.
This test provides excellent coverage of the node transformation pipeline, including all new HITL-related fields. The special handling of OutputVariables pointer comparison is correct. However, the test should use require instead of assert per the coding guidelines to ensure fast failure on assertion errors.
Based on learnings, use stretchr/testify/require for Go tests.
🔎 Refactor to use require
import (
"testing"
"time"
"github.com/dagu-org/dagu/internal/common/collections"
"github.com/dagu-org/dagu/internal/core"
"github.com/dagu-org/dagu/internal/core/execution"
"github.com/dagu-org/dagu/internal/runtime"
"github.com/dagu-org/dagu/internal/runtime/transform"
- "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
)
func TestNodeFieldsRoundTrip(t *testing.T) {
// ... test setup ...
result := status.Nodes[0]
// OutputVariables is a pointer, compare separately
- assert.Equal(t, original.OutputVariables, result.OutputVariables)
+ require.Equal(t, original.OutputVariables, result.OutputVariables)
// Compare rest of the struct
original.OutputVariables = nil
result.OutputVariables = nil
- assert.Equal(t, original, result)
+ require.Equal(t, original, result)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/runtime/transform/node_test.go around lines 15 to 59, replace uses
of assert with require so the test fails fast: import
"github.com/stretchr/testify/require" (and remove assert if no longer used),
change assert.Equal(t, ...) calls to require.Equal(t, ...) for both the
OutputVariables pointer comparison and the final struct comparison, ensuring the
test stops immediately on failure.
| func TestValidateRequiredInputs(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| step core.Step | ||
| body *api.ApproveStepRequest | ||
| expectErr bool | ||
| errMsg string | ||
| }{ | ||
| { | ||
| name: "no config - always valid", | ||
| step: core.Step{ | ||
| Name: "test", | ||
| }, | ||
| body: nil, | ||
| expectErr: false, | ||
| }, | ||
| { | ||
| name: "empty config - always valid", | ||
| step: core.Step{ | ||
| Name: "test", | ||
| ExecutorConfig: core.ExecutorConfig{ | ||
| Type: "hitl", | ||
| Config: map[string]any{}, | ||
| }, | ||
| }, | ||
| body: nil, | ||
| expectErr: false, | ||
| }, | ||
| { | ||
| name: "no required fields - always valid", | ||
| step: core.Step{ | ||
| Name: "test", | ||
| ExecutorConfig: core.ExecutorConfig{ | ||
| Type: "hitl", | ||
| Config: map[string]any{ | ||
| "input": []any{"reason", "approver"}, | ||
| }, | ||
| }, | ||
| }, | ||
| body: nil, | ||
| expectErr: false, | ||
| }, | ||
| { | ||
| name: "required fields provided", | ||
| step: core.Step{ | ||
| Name: "test", | ||
| ExecutorConfig: core.ExecutorConfig{ | ||
| Type: "hitl", | ||
| Config: map[string]any{ | ||
| "input": []any{"reason", "approver"}, | ||
| "required": []any{"reason"}, | ||
| }, | ||
| }, | ||
| }, | ||
| body: &api.ApproveStepRequest{ | ||
| Inputs: &map[string]string{ | ||
| "reason": "approved for testing", | ||
| }, | ||
| }, | ||
| expectErr: false, | ||
| }, | ||
| { | ||
| name: "required fields missing - no body", | ||
| step: core.Step{ | ||
| Name: "test", | ||
| ExecutorConfig: core.ExecutorConfig{ | ||
| Type: "hitl", | ||
| Config: map[string]any{ | ||
| "required": []any{"reason"}, | ||
| }, | ||
| }, | ||
| }, | ||
| body: nil, | ||
| expectErr: true, | ||
| errMsg: "missing required inputs: [reason]", | ||
| }, | ||
| { | ||
| name: "required fields missing - empty inputs", | ||
| step: core.Step{ | ||
| Name: "test", | ||
| ExecutorConfig: core.ExecutorConfig{ | ||
| Type: "hitl", | ||
| Config: map[string]any{ | ||
| "required": []any{"reason", "approver"}, | ||
| }, | ||
| }, | ||
| }, | ||
| body: &api.ApproveStepRequest{ | ||
| Inputs: &map[string]string{}, | ||
| }, | ||
| expectErr: true, | ||
| errMsg: "missing required inputs: [reason approver]", | ||
| }, | ||
| { | ||
| name: "partial required fields provided", | ||
| step: core.Step{ | ||
| Name: "test", | ||
| ExecutorConfig: core.ExecutorConfig{ | ||
| Type: "hitl", | ||
| Config: map[string]any{ | ||
| "required": []any{"reason", "approver"}, | ||
| }, | ||
| }, | ||
| }, | ||
| body: &api.ApproveStepRequest{ | ||
| Inputs: &map[string]string{ | ||
| "reason": "approved", | ||
| }, | ||
| }, | ||
| expectErr: true, | ||
| errMsg: "missing required inputs: [approver]", | ||
| }, | ||
| { | ||
| name: "all required fields provided with extras", | ||
| step: core.Step{ | ||
| Name: "test", | ||
| ExecutorConfig: core.ExecutorConfig{ | ||
| Type: "hitl", | ||
| Config: map[string]any{ | ||
| "required": []any{"reason"}, | ||
| }, | ||
| }, | ||
| }, | ||
| body: &api.ApproveStepRequest{ | ||
| Inputs: &map[string]string{ | ||
| "reason": "approved", | ||
| "comment": "extra field", | ||
| }, | ||
| }, | ||
| expectErr: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| err := validateRequiredInputs(tt.step, tt.body) | ||
| if tt.expectErr { | ||
| assert.Error(t, err) | ||
| if tt.errMsg != "" { | ||
| assert.Equal(t, tt.errMsg, err.Error()) | ||
| } | ||
| } else { | ||
| assert.NoError(t, err) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use require instead of assert for test assertions.
The test provides excellent coverage with comprehensive scenarios, but should follow the coding guideline to use stretchr/testify/require instead of assert. This ensures tests fail fast on assertion errors rather than continuing execution.
Based on learnings, prefer require for Go tests.
🔎 Refactor to use require
import (
"testing"
"github.com/dagu-org/dagu/api/v2"
"github.com/dagu-org/dagu/internal/core"
- "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
)
func TestValidateRequiredInputs(t *testing.T) {
// ... test setup ...
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
err := validateRequiredInputs(tt.step, tt.body)
if tt.expectErr {
- assert.Error(t, err)
+ require.Error(t, err)
if tt.errMsg != "" {
- assert.Equal(t, tt.errMsg, err.Error())
+ require.Equal(t, tt.errMsg, err.Error())
}
} else {
- assert.NoError(t, err)
+ require.NoError(t, err)
}
})
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
internal/service/frontend/api/v2/validation_test.go around lines 11 to 161:
replace stretchr/testify/assert usage with stretchr/testify/require so tests
fail fast; update imports to include require and remove/adjust assert if unused,
and change assert.Error->require.Error, assert.NoError->require.NoError,
assert.Equal->require.Equal (and any other assert.* calls) in each subtest.
| // Create user and add to context | ||
| basicUser := &auth.User{ | ||
| ID: user, | ||
| Username: user, | ||
| Role: auth.RoleAdmin, | ||
| } | ||
| ctx := auth.WithUser(r.Context(), basicUser) | ||
| next.ServeHTTP(w, r.WithContext(ctx)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Basic Auth credential configuration and role usage patterns
echo "=== Finding Creds map definition ==="
rg -n --type=go 'Creds\s*map\[string\]string' -A 3 -B 2
echo "=== Finding all role constant/enum definitions ==="
rg -n --type=go 'Role(Admin|Viewer|Editor|Guest|User)' -B 2 -A 1
echo "=== Check for role-based checks in codebase ==="
rg -n --type=go 'auth\.Role|\.Role\s*==' -B 1 -A 1 | head -40
echo "=== Find Basic Auth and API token auth blocks in middleware ==="
rg -n --type=go 'Basic Auth|basicAuth|ParseBasicAuth' -B 3 -A 8
echo "=== Get file size to read safely ==="
wc -l internal/service/frontend/auth/middleware.goRepository: dagu-org/dagu
Length of output: 50369
🏁 Script executed:
#!/bin/bash
# Read the middleware.go file focusing on auth block implementations
echo "=== Full middleware auth context (lines 140-175) ==="
sed -n '140,175p' internal/service/frontend/auth/middleware.go
echo "=== Check if there are Basic Auth tests ==="
rg -n --type=go 'basicAuth|Basic Auth' -B 5 -A 10 internal/service/frontend/auth/middleware_test.goRepository: dagu-org/dagu
Length of output: 1277
🏁 Script executed:
#!/bin/bash
# Search for Basic Auth tests more broadly
echo "=== Searching for BasicAuth tests ==="
rg -n --type=go 'BasicAuth|basicAuth' -i -B 2 -A 5 internal/service/frontend/auth/
echo "=== Check entire middleware_test.go for auth tests ==="
grep -n "func Test" internal/service/frontend/auth/middleware_test.go
echo "=== Look for Basic Auth handling in tests ==="
grep -n "Creds\|checkBasicAuth" internal/service/frontend/auth/middleware_test.goRepository: dagu-org/dagu
Length of output: 6993
🏁 Script executed:
#!/bin/bash
# Check the entire middleware_test.go file to see if there are any Basic Auth tests
echo "=== Total lines in middleware_test.go ==="
wc -l internal/service/frontend/auth/middleware_test.go
echo "=== All test function names ==="
grep -n "^func Test" internal/service/frontend/auth/middleware_test.go
echo "=== Search for any mention of Basic, basic, Creds, or password ==="
grep -i "basic\|creds\|password" internal/service/frontend/auth/middleware_test.go
echo "=== Check git log for Basic Auth related changes ==="
git log --oneline --all -S "BasicAuth" -- internal/service/frontend/auth/middleware.go | head -10Repository: dagu-org/dagu
Length of output: 1115
🏁 Script executed:
#!/bin/bash
# Check the actual diff/commit that added Basic Auth to understand intent
echo "=== Examine the recent commit with Basic Auth ==="
git show 62b8088 --stat | head -20
echo "=== Look at the actual changes in that commit ==="
git show 62b8088 -- internal/service/frontend/auth/middleware.go | head -100Repository: dagu-org/dagu
Length of output: 4755
Add explicit documentation for Basic Auth role assignment.
The RoleAdmin assignment for all Basic Auth users is intentional and aligns with the existing pattern: both API tokens (line 147-154) and OIDC (oidc.go line 205) also grant admin access. This ensures consistent behavior across all authentication methods.
However, Basic Auth currently lacks an explicit comment explaining this design choice, unlike the other methods. Consider adding a comment similar to the API token case:
// Create user and add to context
basicUser := &auth.User{
ID: user,
Username: user,
Role: auth.RoleAdmin, // Basic auth users get admin by default
}This improves clarity for future maintainers about why all Basic Auth users receive full admin privileges.
🤖 Prompt for AI Agents
In internal/service/frontend/auth/middleware.go around lines 162 to 169, add an
explicit inline comment explaining that Basic Auth users are intentionally
granted admin role to match other authentication methods; update the existing
comment above the basicUser construction to state that Basic Auth users get
admin by default (similar to the API token comment), so future maintainers
understand this design decision.
| {nodeStatus.waiting && ( | ||
| <div className="flex items-center"> | ||
| <div className="h-2 w-2 mr-1 rounded-full bg-amber-500 animate-pulse"></div> | ||
| <span className="text-xs text-muted-foreground"> | ||
| Waiting: {nodeStatus.waiting} | ||
| </span> | ||
| </div> | ||
| )} | ||
|
|
||
| {nodeStatus.rejected && ( | ||
| <div className="flex items-center"> | ||
| <div className="h-2 w-2 mr-1 rounded-full bg-red-600"></div> | ||
| <span className="text-xs text-muted-foreground"> | ||
| Rejected: {nodeStatus.rejected} | ||
| </span> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Color inconsistency between consts.ts and DAGStatusOverview.
The Rejected status color differs between files:
ui/src/consts.tsline 20 definesStatus.Rejectedas#b91c1c(red-700)- This file uses
bg-red-600for rejected indicators (lines 366, 428)
For consistency, either:
- Change
bg-red-600tobg-red-700to match consts.ts - Or preferably, use the semantic color system like other statuses (e.g.,
bg-errorfor Failed,bg-successfor Success)
Note: The Waiting status uses bg-amber-500 consistently, which is good.
🔎 Suggested fix to use red-700
{nodeStatus.rejected && (
<div className="flex items-center">
- <div className="h-2 w-2 mr-1 rounded-full bg-red-600"></div>
+ <div className="h-2 w-2 mr-1 rounded-full bg-red-700"></div>
<span className="text-xs text-muted-foreground">
Rejected: {nodeStatus.rejected}
</span>
</div>
)}And for the progress bar:
{nodeStatus.rejected && (
<div
- className="h-full bg-red-600 float-left"
+ className="h-full bg-red-700 float-left"
style={{
width: `${(nodeStatus.rejected / totalNodes) * 100}%`,
}}
></div>
)}Similar update needed at line 463 for the icon color.
Also applies to: 417-432
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.