feat: add human task API and web UI#2399
Merged
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds durable human-task completion and resume APIs, service logic, CLI delegation, manual-action concurrency controls, frontend task forms, identity attribution, and related persistence, scheduler, filesystem, server, and test updates. ChangesHuman-task contracts and service
Frontend and supporting behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class “human task” support to Dagu’s control plane by exposing completion/resume through the REST API and wiring a schema-driven UI for operator input, while tightening manual-step concurrency semantics and eliminating a server port-allocation race in tests/startup.
Changes:
- Add human-task API endpoints (+ OpenAPI/types) for completing a waiting task and retrying a pending resume handoff.
- Add a new Human Tasks UI tab and centralize “manual action” (approval vs human task) state derivation across the UI.
- Harden manual-step mutations (CAS-style updates/rollback), remote proxy body preservation, and reserve server listeners to avoid port races.
Reviewed changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/features/dags/components/human-task/index.ts | Exposes the Human Tasks tab component. |
| ui/src/features/dags/components/human-task/HumanTasksTab.tsx | New UI for rendering and completing human tasks (schema form) and retrying resume. |
| ui/src/features/dags/components/human-task/tests/HumanTasksTab.test.tsx | UI coverage for completion, typed form submission, resume retry, and permission gating. |
| ui/src/features/dags/components/DAGStatus.tsx | Adds “Human tasks” tab and routes waiting manual work to the right panel. |
| ui/src/features/dags/components/dag-details/NodeStatusTableRow.tsx | Prevents retry/status mutation affordances for human-task steps. |
| ui/src/features/dags/components/dag-details/DAGStatusOverview.tsx | Updates waiting copy to “manual action”. |
| ui/src/features/dags/components/common/DAGActions.tsx | Uses centralized manual-action state; blocks retry when human-task state requires it. |
| ui/src/features/dags/components/approval/ApprovalTab.tsx | Filters waiting nodes to approvals only via centralized manual-action state. |
| ui/src/features/dags/components/tests/DAGStatus.test.tsx | Verifies human tasks route to their own tab and graph status mutation is disabled. |
| ui/src/features/dag-runs/lib/manualActionState.ts | New helper to derive approval vs human-task waiting state and retry-blocking rules. |
| ui/src/features/dag-runs/lib/tests/manualActionState.test.ts | Unit tests for manual-action state derivation. |
| ui/src/features/dag-runs/components/common/DAGRunActions.tsx | Uses centralized manual-action state for reject/retry enablement. |
| ui/src/api/v1/schema.ts | Regenerated OpenAPI TS types: human-task endpoints/schemas + labels/codes updates. |
| README.md | Adds human.task to built-in actions list. |
| internal/service/frontend/api/v1/transformer.go | Serializes human-task config and adds humanTaskResumePending to run details. |
| internal/service/frontend/api/v1/transformer_test.go | Asserts transformer preserves number precision and exposes human-task contract fields. |
| internal/service/frontend/api/v1/remote.go | Proxies request bodies without re-marshalling to preserve exact JSON payload. |
| internal/service/frontend/api/v1/remote_test.go | Ensures remote proxy preserves body/headers/query and content length for human-task completion. |
| internal/service/frontend/api/v1/manual_steps.go | Shared CAS helpers for manual-step mutations and push-back rollback. |
| internal/service/frontend/api/v1/humantasks.go | Implements completion/resume API + input middleware with body limit & strict JSON parsing. |
| internal/service/frontend/api/v1/humantasks_internal_test.go | Middleware tests for preserving validated body, rejecting ambiguous JSON, and enforcing size limits. |
| internal/service/frontend/api/v1/dagruns.go | Moves manual-step mutations to CAS approach; blocks human tasks from approval/status-mutation paths; validates retry. |
| internal/service/frontend/api/v1/dagruns_test.go | Integration test covering end-to-end human-task completion via REST API. |
| internal/service/frontend/api/v1/dagruns_internal_test.go | Tests push-back rollback behavior with concurrent unrelated node changes. |
| internal/service/frontend/api/v1/api.go | Wires the human-task input middleware into the API router. |
| internal/launcher/launcher.go | Adds HumanTaskRetry to inject a hidden resume-claim token into retry subprocess args. |
| internal/launcher/launcher_test.go | Tests resume-claim token placement in retry command args. |
| internal/intg/srvcmd_test.go | Reserves server listener to eliminate port-allocation race in integration tests. |
| internal/dagrun/humantask/service.go | New domain service for completion + recoverable resume handoff orchestration. |
| internal/dagrun/humantask/service_test.go | Extensive coverage for idempotency, concurrency, local/distributed handoff, and retry validation. |
| internal/dagrun/humantask/resume.go | Implements claim/lease/handoff logic for pending resume state. |
| internal/dagrun/humantask/input.go | Strict JSON/object decoding (preserve numbers, reject dup keys) + CLI key=value parsing. |
| internal/dagrun/humantask/completion.go | Implements atomic completion storage + resume-pending state transitions. |
| internal/core/exec/runstatus.go | Adds persisted HumanTaskResume state to run status. |
| internal/cmd/server.go | Allows injecting server options (e.g., listener) into the server command. |
| internal/cmd/server_test.go | Updates tests to use reserved listeners for deterministic startup. |
| internal/cmd/retry.go | Adds hidden --human-task-resume-token and validates retry against human-task checkpoints. |
| internal/cmd/human_task.go | Refactors CLI completion to use the shared human-task service and resume-claim retry path. |
| internal/cmd/human_task_test.go | Updates/expands CLI tests for the refactored completion flow and retry claim handling. |
| internal/cmd/flags_test.go | Ensures the new hidden resume-token flag stays hidden from usage output. |
| api/v1/api.yaml | Adds human-task endpoints, schemas, error codes, and updates waiting label text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
internal/service/frontend/api/v1/api.go (1)
1065-1065: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore transient retry handling for manual-step CAS updates
internal/service/frontend/api/v1/manual_steps.gonow goes straight intoCompareAndSwapLatestAttemptStatus, so transient lock/write failures are no longer retried. If these updates still need retry semantics, wrap this path with the previous retry loop or move it into the CAS helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/frontend/api/v1/api.go` at line 1065, Restore transient retry handling for manual-step status updates by wrapping the CompareAndSwapLatestAttemptStatus path in manual_steps.go with the prior retry loop, or by adding equivalent retry behavior inside that CAS helper. Preserve the existing update semantics while retrying only transient lock/write failures.
🧹 Nitpick comments (1)
ui/src/features/dag-runs/lib/__tests__/manualActionState.test.ts (1)
1-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the "no node details" fallback branch.
Both tests exercise
DAGRunDetailswith anodesarray. Neither test coversgetManualActionState(undefined)or aDAGRunSummary-shaped input (nonodesproperty), which exercises thehasNodeDetailsguard fallback inmanualActionState.ts. This is the path exercised whenever a run-list view calls this utility.✅ Suggested additional test
+ it('returns a safe default when node details are unavailable', () => { + const state = getManualActionState(undefined); + expect(state).toEqual({ + isWaiting: false, + waitingApprovalNodes: [], + waitingHumanTaskNodes: [], + hasHumanTaskWork: false, + humanTaskBlocksRetry: false, + }); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/dag-runs/lib/__tests__/manualActionState.test.ts` around lines 1 - 59, Add a test in the getManualActionState suite covering inputs without node details, including undefined and a DAGRunSummary-shaped object without nodes. Assert the hasNodeDetails fallback behavior returned by getManualActionState, including the expected waiting, approval, human-task, and retry-related state values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/src/features/dags/components/human-task/index.ts`:
- Line 1: Add the repository-standard GPL v3 copyright and SPDX license header
to the barrel file containing the HumanTasksTab export, matching the header
format used by the other new TypeScript files; leave the export unchanged.
---
Outside diff comments:
In `@internal/service/frontend/api/v1/api.go`:
- Line 1065: Restore transient retry handling for manual-step status updates by
wrapping the CompareAndSwapLatestAttemptStatus path in manual_steps.go with the
prior retry loop, or by adding equivalent retry behavior inside that CAS helper.
Preserve the existing update semantics while retrying only transient lock/write
failures.
---
Nitpick comments:
In `@ui/src/features/dag-runs/lib/__tests__/manualActionState.test.ts`:
- Around line 1-59: Add a test in the getManualActionState suite covering inputs
without node details, including undefined and a DAGRunSummary-shaped object
without nodes. Assert the hasNodeDetails fallback behavior returned by
getManualActionState, including the expected waiting, approval, human-task, and
retry-related state values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a27ec55b-b585-44d1-84fd-2317fb87df36
📒 Files selected for processing (42)
README.mdapi/v1/api.gen.goapi/v1/api.yamlinternal/cmd/flags_test.gointernal/cmd/human_task.gointernal/cmd/human_task_test.gointernal/cmd/retry.gointernal/cmd/server.gointernal/cmd/server_test.gointernal/core/exec/runstatus.gointernal/dagrun/humantask/completion.gointernal/dagrun/humantask/input.gointernal/dagrun/humantask/resume.gointernal/dagrun/humantask/service.gointernal/dagrun/humantask/service_test.gointernal/intg/srvcmd_test.gointernal/launcher/launcher.gointernal/launcher/launcher_test.gointernal/service/frontend/api/v1/api.gointernal/service/frontend/api/v1/dagruns.gointernal/service/frontend/api/v1/dagruns_internal_test.gointernal/service/frontend/api/v1/dagruns_test.gointernal/service/frontend/api/v1/humantasks.gointernal/service/frontend/api/v1/humantasks_internal_test.gointernal/service/frontend/api/v1/manual_steps.gointernal/service/frontend/api/v1/remote.gointernal/service/frontend/api/v1/remote_test.gointernal/service/frontend/api/v1/transformer.gointernal/service/frontend/api/v1/transformer_test.goui/src/api/v1/schema.tsui/src/features/dag-runs/components/common/DAGRunActions.tsxui/src/features/dag-runs/lib/__tests__/manualActionState.test.tsui/src/features/dag-runs/lib/manualActionState.tsui/src/features/dags/components/DAGStatus.tsxui/src/features/dags/components/__tests__/DAGStatus.test.tsxui/src/features/dags/components/approval/ApprovalTab.tsxui/src/features/dags/components/common/DAGActions.tsxui/src/features/dags/components/dag-details/DAGStatusOverview.tsxui/src/features/dags/components/dag-details/NodeStatusTableRow.tsxui/src/features/dags/components/human-task/HumanTasksTab.tsxui/src/features/dags/components/human-task/__tests__/HumanTasksTab.test.tsxui/src/features/dags/components/human-task/index.ts
Collaborator
Author
|
Addressed the remaining CodeRabbit review in
The generic docstring-coverage warning was not applied: repository policy requires API-contract documentation where callers need it and avoids filler comments added only to satisfy a percentage metric. It is not an enforced repository check. |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/service/frontend/api/v1/humantasks_internal_test.go (1)
96-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
calledtracking.The
calledflag is redundant in this test becauseWithRemoteNodeinitialized with anilresolver will always return a503 Service Unavailableresponse and exit without calling the wrapped innermost handler.The assertion
assert.Equal(t, http.StatusBadRequest, response.Code)is already sufficient to prove thathumanTaskInputMiddlewaresuccessfully executed and short-circuited the request beforeWithRemoteNodecould return a 503.♻️ Proposed refactor
func TestHumanTaskInputMiddlewareValidatesBeforeRemoteProxy(t *testing.T) { - called := false handler := humanTaskInputMiddleware("/api/v1")( - WithRemoteNode(nil, "/api/v1")(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { - called = true - })), + WithRemoteNode(nil, "/api/v1")(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})), ) request := httptest.NewRequest( http.MethodPost, "/api/v1/dag-runs/deploy/run-1/human-tasks/review/complete?remoteNode=edge", strings.NewReader(`{"confirmed":true,"confirmed":false}`), ) response := httptest.NewRecorder() handler.ServeHTTP(response, request) - assert.False(t, called) assert.Equal(t, http.StatusBadRequest, response.Code) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/frontend/api/v1/humantasks_internal_test.go` around lines 96 - 114, Remove the redundant called variable, the innermost handler’s assignment, and the assert.False assertion from TestHumanTaskInputMiddlewareValidatesBeforeRemoteProxy. Keep the WithRemoteNode wrapper and the existing Bad Request status assertion to verify validation short-circuits before the remote proxy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/service/frontend/api/v1/humantasks_internal_test.go`:
- Around line 96-114: Remove the redundant called variable, the innermost
handler’s assignment, and the assert.False assertion from
TestHumanTaskInputMiddlewareValidatesBeforeRemoteProxy. Keep the WithRemoteNode
wrapper and the existing Bad Request status assertion to verify validation
short-circuits before the remote proxy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bb42b8f-69b4-4b05-8d29-212339cb5ad1
📒 Files selected for processing (3)
internal/service/frontend/api/v1/api.gointernal/service/frontend/api/v1/humantasks.gointernal/service/frontend/api/v1/humantasks_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/service/frontend/api/v1/humantasks.go
- internal/service/frontend/api/v1/api.go
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 42 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
ui/src/features/dag-runs/lib/manualActionState.ts:54
humanTaskBlocksRetryis computed asisWaiting && (resumePending || anyHumanTaskNodeExists), which blocks retries even when the run is waiting for approval and all human tasks are already completed (i.e., no active human-task checkpoint). This also diverges from the intent of only blocking retry while a human-task checkpoint is active.
Consider keying this strictly off actionable human-task work (waitingHumanTaskNodes or humanTaskResumePending).
Collaborator
Author
|
Addressed the latest review findings in |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
yohamta0
force-pushed
the
feat/human-task-api-ui
branch
from
July 23, 2026 14:36
75a182d to
a7d0203
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ui/src/features/dag-runs/components/common/DAGRunActions.tsx (1)
151-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the single-reject-anchor design decision inline.
Using only
waitingApprovalNodes[0]is intentional per the backend contract (one rejection is the run-level rejection anchor;applyRejectionatomically transitions the whole run toRejected). This exact line already triggered a false-positive review concern once. A short comment here would save future reviewers/maintainers from re-litigating it.📝 Suggested clarifying comment
+ // Only the first waiting approval node is used: rejecting one node is the + // run-level rejection anchor (backend applyRejection transitions the whole + // DAG run to Rejected), so iterating remaining nodes is unnecessary. const { isWaiting, waitingApprovalNodes } = getManualActionState(dagRun); const waitingApprovalStepName = waitingApprovalNodes[0]?.step.name;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/dag-runs/components/common/DAGRunActions.tsx` around lines 151 - 153, In the DAGRunActions logic around getManualActionState, add a brief inline comment explaining that waitingApprovalNodes[0] is intentionally the single run-level rejection anchor, as guaranteed by the backend contract, and that applyRejection atomically transitions the entire run to Rejected. Keep the existing behavior unchanged.internal/persis/file/collection.go (1)
420-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared
WalkDircallback to avoid duplication.
collectIDsandcollectnow contain an identical directory/lock-skip/suffix-filter block. Consider factoring this into a shared helper (e.g., a function returning afilepath.WalkDirmatcher, or a smallshouldSkipOrProcess(path, d) (skip bool, isJSON bool)helper) to keep the two traversal implementations in sync going forward.Also applies to: 453-465
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/persis/file/collection.go` around lines 420 - 432, Extract the duplicated WalkDir filtering logic from collectIDs and collect into a shared helper near these traversal methods. Preserve the existing behavior: return errors, skip lock directories with filepath.SkipDir, ignore directories otherwise, and process only .json files; update both callbacks to use the helper so they remain synchronized.internal/cmd/server_test.go (1)
41-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the now-unused
findPorthelper frominternal/cmd/server_test.go.
internal/cmd/server_test.gono longer constructs or passes port strings viafindPort; the only remaining occurrences are in sibling test files, so removing this local function preventsunused/staticcheck lint failures.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmd/human_task.go`:
- Around line 90-97: Trim the value returned for dagRunID in the human-task
command before placing it in CompleteRequest.DAGRunID, matching the existing
sanitization applied to stepID and dagName. Preserve the current error handling
for ctx.StringParam and use the trimmed identifier for subsequent operations.
---
Nitpick comments:
In `@internal/cmd/server_test.go`:
- Around line 41-48: Remove the unused findPort helper from
internal/cmd/server_test.go, including its function body; do not alter the
sibling test helpers or unrelated test logic.
In `@internal/persis/file/collection.go`:
- Around line 420-432: Extract the duplicated WalkDir filtering logic from
collectIDs and collect into a shared helper near these traversal methods.
Preserve the existing behavior: return errors, skip lock directories with
filepath.SkipDir, ignore directories otherwise, and process only .json files;
update both callbacks to use the helper so they remain synchronized.
In `@ui/src/features/dag-runs/components/common/DAGRunActions.tsx`:
- Around line 151-153: In the DAGRunActions logic around getManualActionState,
add a brief inline comment explaining that waitingApprovalNodes[0] is
intentionally the single run-level rejection anchor, as guaranteed by the
backend contract, and that applyRejection atomically transitions the entire run
to Rejected. Keep the existing behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19f22215-c086-4470-a729-5428575dd3d4
📒 Files selected for processing (96)
README.mdapi/v1/api.gen.goapi/v1/api.yamlconformance/spec031_human_task/completion_test.goconformance/spec031_human_task/distributed_test.goconformance/spec031_human_task/helpers_test.goconformance/spec031_human_task/lifecycle_test.goengine_test.gointernal/cmd/human_task.gointernal/cmd/human_task_test.gointernal/cmd/retry.gointernal/cmd/server.gointernal/cmd/server_test.gointernal/cmn/dirlock/dirlock.gointernal/cmn/fileutil/retry.gointernal/cmn/fileutil/retry_test.gointernal/core/dag.gointernal/core/exec/enqueue_retry.gointernal/core/exec/enqueue_retry_test.gointernal/core/exec/node.gointernal/core/exec/pushback.gointernal/dagrun/humantask/completion.gointernal/dagrun/humantask/input.gointernal/dagrun/humantask/resume.gointernal/dagrun/humantask/service.gointernal/dagrun/humantask/service_test.gointernal/intg/distr/fixtures_test.gointernal/intg/distr/human_task_test.gointernal/intg/distr/queue_recovery_test.gointernal/intg/queue/fixture_test.gointernal/intg/srvcmd_test.gointernal/persis/file/collection.gointernal/persis/file/collection_test.gointernal/persis/file/dagrun/attempt.gointernal/persis/file/dagrun/dagrun.gointernal/persis/file/dagrun/dataroot.gointernal/persis/file/dagrun/writer.gointernal/persis/store/distributed_admission.gointernal/persis/store/distributed_admission_test.gointernal/persis/store/distributed_dispatch.gointernal/runtime/agent/agent.gointernal/runtime/data.gointernal/runtime/plan_test.gointernal/runtime/pushback.gointernal/runtime/transform/node.gointernal/runtime/transform/node_test.gointernal/service/coordinator/handler.gointernal/service/coordinator/handler_test.gointernal/service/frontend/api/v1/api.gointernal/service/frontend/api/v1/dagruns.gointernal/service/frontend/api/v1/dagruns_edit_retry.gointernal/service/frontend/api/v1/dagruns_edit_retry_internal_test.gointernal/service/frontend/api/v1/dagruns_internal_test.gointernal/service/frontend/api/v1/dagruns_retry_internal_test.gointernal/service/frontend/api/v1/dagruns_test.gointernal/service/frontend/api/v1/humantasks.gointernal/service/frontend/api/v1/humantasks_internal_test.gointernal/service/frontend/api/v1/manual_steps.gointernal/service/frontend/api/v1/openapi_test.gointernal/service/frontend/api/v1/remote.gointernal/service/frontend/api/v1/remote_test.gointernal/service/frontend/api/v1/transformer.gointernal/service/frontend/api/v1/transformer_test.gointernal/service/scheduler/queue_dispatcher.gointernal/service/scheduler/queue_processor.gointernal/service/scheduler/queue_processor_test.gointernal/service/scheduler/retry_scanner.gointernal/service/scheduler/retry_scanner_test.gointernal/test/helper.gointernal/test/server.gospecs/031-human-task.mdui/src/api/v1/schema.tsui/src/features/dag-runs/components/common/DAGRunActions.tsxui/src/features/dag-runs/components/common/RejectDAGRunDialog.tsxui/src/features/dag-runs/components/common/__tests__/DAGRunActions.test.tsxui/src/features/dag-runs/components/common/__tests__/RejectDAGRunDialog.test.tsxui/src/features/dag-runs/lib/__tests__/manualActionState.test.tsui/src/features/dag-runs/lib/manualActionState.tsui/src/features/dags/components/DAGStatus.tsxui/src/features/dags/components/__tests__/DAGStatus.test.tsxui/src/features/dags/components/approval/ApprovalTab.tsxui/src/features/dags/components/common/DAGActions.tsxui/src/features/dags/components/common/ManualActionSubject.tsxui/src/features/dags/components/common/PushBackHistory.tsxui/src/features/dags/components/common/__tests__/DAGActions.test.tsxui/src/features/dags/components/common/__tests__/PushBackHistory.test.tsxui/src/features/dags/components/dag-details/DAGStatusOverview.tsxui/src/features/dags/components/dag-details/NodeStatusTableRow.tsxui/src/features/dags/components/dag-details/__tests__/NodeStatusTableRow.test.tsxui/src/features/dags/components/human-task/HumanTasksTab.tsxui/src/features/dags/components/human-task/__tests__/HumanTasksTab.test.tsxui/src/features/dags/components/human-task/index.tsui/src/features/dags/components/visualization/__tests__/TimelineChart.test.tsxui/src/pages/profiles/__tests__/index.test.tsxui/src/pages/users/__tests__/index.test.tsxui/src/test/setup.ts
💤 Files with no reviewable changes (4)
- internal/intg/distr/human_task_test.go
- ui/src/pages/profiles/tests/index.test.tsx
- ui/src/pages/users/tests/index.test.tsx
- internal/service/scheduler/retry_scanner_test.go
Comment on lines
+90
to
97
| dagRunID, err := ctx.StringParam(humanTaskRunIDFlag.name) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| completionInput, err := parseHumanTaskCompletionInput(ctx.Command) | ||
| stepID, err := ctx.StringParam(humanTaskStepFlag.name) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim dagRunID for consistency with stepID/dagName sanitization.
stepID and dagName are trimmed before use, but dagRunID is passed through untouched into CompleteRequest.DAGRunID. Stray whitespace would cause a spurious not-found instead of being sanitized like the other identifiers.
🩹 Proposed fix
dagRunID, err := ctx.StringParam(humanTaskRunIDFlag.name)
if err != nil {
return err
}
+ dagRunID = strings.TrimSpace(dagRunID)
+ if dagRunID == "" {
+ return fmt.Errorf("--run-id must not be empty")
+ }
stepID, err := ctx.StringParam(humanTaskStepFlag.name)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dagRunID, err := ctx.StringParam(humanTaskRunIDFlag.name) | |
| if err != nil { | |
| return err | |
| } | |
| completionInput, err := parseHumanTaskCompletionInput(ctx.Command) | |
| stepID, err := ctx.StringParam(humanTaskStepFlag.name) | |
| if err != nil { | |
| return err | |
| } | |
| dagRunID, err := ctx.StringParam(humanTaskRunIDFlag.name) | |
| if err != nil { | |
| return err | |
| } | |
| dagRunID = strings.TrimSpace(dagRunID) | |
| if dagRunID == "" { | |
| return fmt.Errorf("--run-id must not be empty") | |
| } | |
| stepID, err := ctx.StringParam(humanTaskStepFlag.name) | |
| if err != nil { | |
| return err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cmd/human_task.go` around lines 90 - 97, Trim the value returned for
dagRunID in the human-task command before placing it in
CompleteRequest.DAGRunID, matching the existing sanitization applied to stepID
and dagName. Preserve the current error handling for ctx.StringParam and use the
trimmed identifier for subsequent operations.
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/cmd/human_task.go (1)
90-93: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
dagRunIDstill isn't trimmed (previously flagged, appears unaddressed).
stepIDanddagNameare trimmed and validated non-empty, butdagRunIDis passed through untouched intoCompleteRequest.DAGRunID. Stray whitespace would cause a spurious not-found instead of a clear validation error.🩹 Proposed fix
dagRunID, err := ctx.StringParam(humanTaskRunIDFlag.name) if err != nil { return err } + dagRunID = strings.TrimSpace(dagRunID) + if dagRunID == "" { + return fmt.Errorf("--run-id must not be empty") + } stepID, err := ctx.StringParam(humanTaskStepFlag.name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/human_task.go` around lines 90 - 93, Update the dagRunID handling in the human-task command to trim surrounding whitespace immediately after retrieving it with ctx.StringParam and before constructing CompleteRequest.DAGRunID. Validate the trimmed value as non-empty, matching the existing stepID and dagName handling, and return the established validation error for blank input.
🧹 Nitpick comments (1)
internal/persis/file/dagrun/dataroot.go (1)
866-872: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
isDirEmptystill uses rawos.ReadDir.Three other directory-listing sites in this file were switched to
fileutil.ReadDirfor Windows-retry robustness, butisDirEmptywas left onos.ReadDir, leaving it exposed to the same transient-failure class this PR is hardening against elsewhere.♻️ Proposed fix
func isDirEmpty(path string) bool { - entries, err := os.ReadDir(path) + entries, err := fileutil.ReadDir(path) if err != nil { return false } return len(entries) == 0 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/persis/file/dagrun/dataroot.go` around lines 866 - 872, Update isDirEmpty to use fileutil.ReadDir instead of os.ReadDir, preserving its existing false-on-error and empty-directory checks so it receives the same Windows-retry behavior as the other directory-listing sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/src/features/dags/components/common/ManualActionSubject.tsx`:
- Around line 21-35: Update the trigger span in ManualActionSubject to include
the whitespace-normal and break-words classes, preserving the existing className
and label rendering so long subject labels wrap in tables and flex rows.
---
Duplicate comments:
In `@internal/cmd/human_task.go`:
- Around line 90-93: Update the dagRunID handling in the human-task command to
trim surrounding whitespace immediately after retrieving it with ctx.StringParam
and before constructing CompleteRequest.DAGRunID. Validate the trimmed value as
non-empty, matching the existing stepID and dagName handling, and return the
established validation error for blank input.
---
Nitpick comments:
In `@internal/persis/file/dagrun/dataroot.go`:
- Around line 866-872: Update isDirEmpty to use fileutil.ReadDir instead of
os.ReadDir, preserving its existing false-on-error and empty-directory checks so
it receives the same Windows-retry behavior as the other directory-listing
sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a80752a-2184-47ea-a898-2c8e56983923
📒 Files selected for processing (98)
README.mdapi/v1/api.gen.goapi/v1/api.yamlconformance/spec031_human_task/completion_test.goconformance/spec031_human_task/distributed_test.goconformance/spec031_human_task/helpers_test.goconformance/spec031_human_task/lifecycle_test.goengine_test.gointernal/cmd/human_task.gointernal/cmd/human_task_test.gointernal/cmd/retry.gointernal/cmd/server.gointernal/cmd/server_test.gointernal/cmn/dirlock/dirlock.gointernal/cmn/fileutil/retry.gointernal/cmn/fileutil/retry_test.gointernal/core/dag.gointernal/core/exec/enqueue_retry.gointernal/core/exec/enqueue_retry_test.gointernal/core/exec/node.gointernal/core/exec/pushback.gointernal/dagrun/humantask/completion.gointernal/dagrun/humantask/input.gointernal/dagrun/humantask/resume.gointernal/dagrun/humantask/service.gointernal/dagrun/humantask/service_test.gointernal/intg/distr/fixtures_test.gointernal/intg/distr/human_task_test.gointernal/intg/distr/queue_recovery_test.gointernal/intg/queue/fixture_test.gointernal/intg/srvcmd_test.gointernal/persis/file/collection.gointernal/persis/file/collection_test.gointernal/persis/file/dagrun/attempt.gointernal/persis/file/dagrun/attempt_external_test.gointernal/persis/file/dagrun/dagrun.gointernal/persis/file/dagrun/dataroot.gointernal/persis/file/dagrun/store.gointernal/persis/file/dagrun/writer.gointernal/persis/store/distributed_admission.gointernal/persis/store/distributed_admission_test.gointernal/persis/store/distributed_dispatch.gointernal/runtime/agent/agent.gointernal/runtime/data.gointernal/runtime/plan_test.gointernal/runtime/pushback.gointernal/runtime/transform/node.gointernal/runtime/transform/node_test.gointernal/service/coordinator/handler.gointernal/service/coordinator/handler_test.gointernal/service/frontend/api/v1/api.gointernal/service/frontend/api/v1/dagruns.gointernal/service/frontend/api/v1/dagruns_edit_retry.gointernal/service/frontend/api/v1/dagruns_edit_retry_internal_test.gointernal/service/frontend/api/v1/dagruns_internal_test.gointernal/service/frontend/api/v1/dagruns_retry_internal_test.gointernal/service/frontend/api/v1/dagruns_test.gointernal/service/frontend/api/v1/humantasks.gointernal/service/frontend/api/v1/humantasks_internal_test.gointernal/service/frontend/api/v1/manual_steps.gointernal/service/frontend/api/v1/openapi_test.gointernal/service/frontend/api/v1/remote.gointernal/service/frontend/api/v1/remote_test.gointernal/service/frontend/api/v1/transformer.gointernal/service/frontend/api/v1/transformer_test.gointernal/service/scheduler/queue_dispatcher.gointernal/service/scheduler/queue_processor.gointernal/service/scheduler/queue_processor_test.gointernal/service/scheduler/retry_scanner.gointernal/service/scheduler/retry_scanner_test.gointernal/test/helper.gointernal/test/server.gospecs/031-human-task.mdui/src/api/v1/schema.tsui/src/features/dag-runs/components/common/DAGRunActions.tsxui/src/features/dag-runs/components/common/RejectDAGRunDialog.tsxui/src/features/dag-runs/components/common/__tests__/DAGRunActions.test.tsxui/src/features/dag-runs/components/common/__tests__/RejectDAGRunDialog.test.tsxui/src/features/dag-runs/lib/__tests__/manualActionState.test.tsui/src/features/dag-runs/lib/manualActionState.tsui/src/features/dags/components/DAGStatus.tsxui/src/features/dags/components/__tests__/DAGStatus.test.tsxui/src/features/dags/components/approval/ApprovalTab.tsxui/src/features/dags/components/common/DAGActions.tsxui/src/features/dags/components/common/ManualActionSubject.tsxui/src/features/dags/components/common/PushBackHistory.tsxui/src/features/dags/components/common/__tests__/DAGActions.test.tsxui/src/features/dags/components/common/__tests__/PushBackHistory.test.tsxui/src/features/dags/components/dag-details/DAGStatusOverview.tsxui/src/features/dags/components/dag-details/NodeStatusTableRow.tsxui/src/features/dags/components/dag-details/__tests__/NodeStatusTableRow.test.tsxui/src/features/dags/components/human-task/HumanTasksTab.tsxui/src/features/dags/components/human-task/__tests__/HumanTasksTab.test.tsxui/src/features/dags/components/human-task/index.tsui/src/features/dags/components/visualization/__tests__/TimelineChart.test.tsxui/src/pages/profiles/__tests__/index.test.tsxui/src/pages/users/__tests__/index.test.tsxui/src/test/setup.ts
💤 Files with no reviewable changes (5)
- internal/intg/distr/human_task_test.go
- ui/src/pages/profiles/tests/index.test.tsx
- internal/service/scheduler/retry_scanner_test.go
- ui/src/pages/users/tests/index.test.tsx
- internal/persis/file/dagrun/attempt_external_test.go
Comment on lines
+21
to
+35
| if (!id) { | ||
| return <span className={className}>{label}</span>; | ||
| } | ||
|
|
||
| return ( | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <span className={className} tabIndex={0}> | ||
| {label} | ||
| </span> | ||
| </TooltipTrigger> | ||
| <TooltipContent>Subject ID: {id}</TooltipContent> | ||
| </Tooltip> | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap long subject labels to prevent table/list overflow.
The trigger <span> has no whitespace-normal break-words, so long usernames or fallback subject IDs (UUIDs, os:<uid>) can overflow when rendered in table cells (NodeStatusTableRow.tsx) or flex rows (PushBackHistory.tsx), both of which reuse this component.
🔧 Proposed fix
if (!id) {
- return <span className={className}>{label}</span>;
+ return (
+ <span className={cn('whitespace-normal break-words', className)}>
+ {label}
+ </span>
+ );
}
return (
<Tooltip>
<TooltipTrigger asChild>
- <span className={className} tabIndex={0}>
+ <span
+ className={cn('whitespace-normal break-words', className)}
+ tabIndex={0}
+ >
{label}
</span>
</TooltipTrigger>As per coding guidelines, "Always handle long text in tables and lists with whitespace-normal break-words to prevent layout overflow."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!id) { | |
| return <span className={className}>{label}</span>; | |
| } | |
| return ( | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <span className={className} tabIndex={0}> | |
| {label} | |
| </span> | |
| </TooltipTrigger> | |
| <TooltipContent>Subject ID: {id}</TooltipContent> | |
| </Tooltip> | |
| ); | |
| } | |
| if (!id) { | |
| return ( | |
| <span className={cn('whitespace-normal break-words', className)}> | |
| {label} | |
| </span> | |
| ); | |
| } | |
| return ( | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <span | |
| className={cn('whitespace-normal break-words', className)} | |
| tabIndex={0} | |
| > | |
| {label} | |
| </span> | |
| </TooltipTrigger> | |
| <TooltipContent>Subject ID: {id}</TooltipContent> | |
| </Tooltip> | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/src/features/dags/components/common/ManualActionSubject.tsx` around lines
21 - 35, Update the trigger span in ManualActionSubject to include the
whitespace-normal and break-words classes, preserving the existing className and
label rendering so long subject labels wrap in tables and flex rows.
Source: Coding guidelines
# Conflicts: # api/v1/api.gen.go
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/intg/queue/fixture_test.go (1)
533-544: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFixture ignores whether the retry was actually enqueued.
EnqueueRetryreturnsfalse, nilfor no-op cases (already queued, stale-but-same-attempt). Ignoring the bool meansRetryEnqueuecan silently succeed without having queued anything, which could mask fixture setup problems in tests relying on this helper.🔧 Proposed fix
func (f *fixture) RetryEnqueue(runID string) *fixture { - _, err := exec.EnqueueRetry( + enqueued, err := exec.EnqueueRetry( f.th.Context, f.th.DAGRunStore, f.th.QueueStore, f.dag, f.MustStatus(runID), exec.EnqueueRetryOptions{}, ) require.NoError(f.t, err) + require.True(f.t, enqueued, "expected retry to be enqueued") return f }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/intg/queue/fixture_test.go` around lines 533 - 544, Update fixture.RetryEnqueue to capture the boolean result from exec.EnqueueRetry and assert it is true after verifying the error is nil. Preserve the existing error assertion and fluent return so no-op retries fail fixture setup instead of being treated as successful.
🧹 Nitpick comments (1)
internal/persis/file/collection.go (1)
421-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated WalkDir entry-filtering logic.
The identical lock-dir-skip +
.jsonsuffix filter block is now duplicated betweencollectIDsandcollect. This is the exact logic that needed a coordinated fix here; keeping two copies risks the two functions drifting apart again in the future.♻️ Proposed refactor to share the WalkDir filtering logic
+func isRelevantJSONEntry(path string, d fs.DirEntry) (bool, error) { + if d.IsDir() { + if dirlock.IsLockDirectoryName(d.Name()) { + return false, filepath.SkipDir + } + return false, nil + } + return strings.HasSuffix(path, ".json"), nil +} + func (c *Collection) collectIDs(prefix string) ([]string, error) { walkRoot := c.prefixWalkRoot(prefix) var ids []string err := filepath.WalkDir(walkRoot, func(path string, d fs.DirEntry, err error) error { if err != nil { return err } - if d.IsDir() { - if dirlock.IsLockDirectoryName(d.Name()) { - return filepath.SkipDir - } - return nil - } - if !strings.HasSuffix(path, ".json") { + relevant, err := isRelevantJSONEntry(path, d) + if err != nil { + return err + } + if !relevant { return nil }(apply the same substitution in
collect)Also applies to: 454-465
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/persis/file/collection.go` around lines 421 - 432, Extract the shared WalkDir entry-filtering logic from collectIDs and collect into a common helper, preserving the existing dirlock.IsLockDirectoryName and “.json” suffix behavior, then update both functions to use it so the filters cannot drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1/api.yaml`:
- Around line 8745-8753: The HumanTaskStepId path parameter’s pattern and
required constraint are narrower than the supported Step.id values. Align the
route definition with Step.id by either requiring a matching id whenever
humanTask is configured or removing the restrictive pattern and length
constraint to accept the actual supported step-ID format; preserve
path-parameter validity.
In `@internal/service/coordinator/handler.go`:
- Around line 1729-1782: Update preservesCompletedManualActions to recognize
persisted wait-step rejection via RejectedAt, RejectedBy, RejectedByID, or
RejectionReason, including rejection states coexisting with
HasRejected/HasWaiting. When such a current node exists, reject incoming reports
that regress the node to Waiting unless the incoming run status is Rejected,
while preserving the existing manual-action checks.
In `@internal/service/frontend/api/v1/manual_steps.go`:
- Around line 152-160: Update requireApprovalNode to handle node == nil
separately and return the appropriate missing-node error for the step. Keep the
existing human-task error only for non-nil nodes whose Step.HumanTask is
configured, while preserving the approval configuration validation for other
nodes.
---
Outside diff comments:
In `@internal/intg/queue/fixture_test.go`:
- Around line 533-544: Update fixture.RetryEnqueue to capture the boolean result
from exec.EnqueueRetry and assert it is true after verifying the error is nil.
Preserve the existing error assertion and fluent return so no-op retries fail
fixture setup instead of being treated as successful.
---
Nitpick comments:
In `@internal/persis/file/collection.go`:
- Around line 421-432: Extract the shared WalkDir entry-filtering logic from
collectIDs and collect into a common helper, preserving the existing
dirlock.IsLockDirectoryName and “.json” suffix behavior, then update both
functions to use it so the filters cannot drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a80752a-2184-47ea-a898-2c8e56983923
📒 Files selected for processing (98)
README.mdapi/v1/api.gen.goapi/v1/api.yamlconformance/spec031_human_task/completion_test.goconformance/spec031_human_task/distributed_test.goconformance/spec031_human_task/helpers_test.goconformance/spec031_human_task/lifecycle_test.goengine_test.gointernal/cmd/human_task.gointernal/cmd/human_task_test.gointernal/cmd/retry.gointernal/cmd/server.gointernal/cmd/server_test.gointernal/cmn/dirlock/dirlock.gointernal/cmn/fileutil/retry.gointernal/cmn/fileutil/retry_test.gointernal/core/dag.gointernal/core/exec/enqueue_retry.gointernal/core/exec/enqueue_retry_test.gointernal/core/exec/node.gointernal/core/exec/pushback.gointernal/humantask/completion.gointernal/humantask/input.gointernal/humantask/resume.gointernal/humantask/service.gointernal/humantask/service_test.gointernal/intg/distr/fixtures_test.gointernal/intg/distr/human_task_test.gointernal/intg/distr/queue_recovery_test.gointernal/intg/queue/fixture_test.gointernal/intg/srvcmd_test.gointernal/persis/file/collection.gointernal/persis/file/collection_test.gointernal/persis/file/dagrun/attempt.gointernal/persis/file/dagrun/attempt_external_test.gointernal/persis/file/dagrun/dagrun.gointernal/persis/file/dagrun/dataroot.gointernal/persis/file/dagrun/store.gointernal/persis/file/dagrun/writer.gointernal/persis/store/distributed_admission.gointernal/persis/store/distributed_admission_test.gointernal/persis/store/distributed_dispatch.gointernal/runtime/agent/agent.gointernal/runtime/data.gointernal/runtime/plan_test.gointernal/runtime/pushback.gointernal/runtime/transform/node.gointernal/runtime/transform/node_test.gointernal/service/coordinator/handler.gointernal/service/coordinator/handler_test.gointernal/service/frontend/api/v1/api.gointernal/service/frontend/api/v1/dagruns.gointernal/service/frontend/api/v1/dagruns_edit_retry.gointernal/service/frontend/api/v1/dagruns_edit_retry_internal_test.gointernal/service/frontend/api/v1/dagruns_internal_test.gointernal/service/frontend/api/v1/dagruns_retry_internal_test.gointernal/service/frontend/api/v1/dagruns_test.gointernal/service/frontend/api/v1/humantasks.gointernal/service/frontend/api/v1/humantasks_internal_test.gointernal/service/frontend/api/v1/manual_steps.gointernal/service/frontend/api/v1/openapi_test.gointernal/service/frontend/api/v1/remote.gointernal/service/frontend/api/v1/remote_test.gointernal/service/frontend/api/v1/transformer.gointernal/service/frontend/api/v1/transformer_test.gointernal/service/scheduler/queue_dispatcher.gointernal/service/scheduler/queue_processor.gointernal/service/scheduler/queue_processor_test.gointernal/service/scheduler/retry_scanner.gointernal/service/scheduler/retry_scanner_test.gointernal/test/helper.gointernal/test/server.gospecs/031-human-task.mdui/src/api/v1/schema.tsui/src/features/dag-runs/components/common/DAGRunActions.tsxui/src/features/dag-runs/components/common/RejectDAGRunDialog.tsxui/src/features/dag-runs/components/common/__tests__/DAGRunActions.test.tsxui/src/features/dag-runs/components/common/__tests__/RejectDAGRunDialog.test.tsxui/src/features/dag-runs/lib/__tests__/manualActionState.test.tsui/src/features/dag-runs/lib/manualActionState.tsui/src/features/dags/components/DAGStatus.tsxui/src/features/dags/components/__tests__/DAGStatus.test.tsxui/src/features/dags/components/approval/ApprovalTab.tsxui/src/features/dags/components/common/DAGActions.tsxui/src/features/dags/components/common/ManualActionSubject.tsxui/src/features/dags/components/common/PushBackHistory.tsxui/src/features/dags/components/common/__tests__/DAGActions.test.tsxui/src/features/dags/components/common/__tests__/PushBackHistory.test.tsxui/src/features/dags/components/dag-details/DAGStatusOverview.tsxui/src/features/dags/components/dag-details/NodeStatusTableRow.tsxui/src/features/dags/components/dag-details/__tests__/NodeStatusTableRow.test.tsxui/src/features/dags/components/human-task/HumanTasksTab.tsxui/src/features/dags/components/human-task/__tests__/HumanTasksTab.test.tsxui/src/features/dags/components/human-task/index.tsui/src/features/dags/components/visualization/__tests__/TimelineChart.test.tsxui/src/pages/profiles/__tests__/index.test.tsxui/src/pages/users/__tests__/index.test.tsxui/src/test/setup.ts
💤 Files with no reviewable changes (5)
- internal/intg/distr/human_task_test.go
- ui/src/pages/profiles/tests/index.test.tsx
- internal/persis/file/dagrun/attempt_external_test.go
- ui/src/pages/users/tests/index.test.tsx
- internal/service/scheduler/retry_scanner_test.go
Comment on lines
+8745
to
+8753
| HumanTaskStepId: | ||
| name: stepId | ||
| in: path | ||
| description: explicit ID of the human-task step | ||
| required: true | ||
| schema: | ||
| type: string | ||
| pattern: "^[A-Za-z][A-Za-z0-9_]*$" | ||
| maxLength: 40 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the route ID constraint with Step.id.
Step.id is optional and unconstrained, but this endpoint only accepts [A-Za-z][A-Za-z0-9_]*. A valid human-task step without an ID—or with an ID such as review-1—cannot be completed through the documented API. Require a matching id whenever humanTask is configured, or relax this path constraint to the actual supported step-ID format.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v1/api.yaml` around lines 8745 - 8753, The HumanTaskStepId path
parameter’s pattern and required constraint are narrower than the supported
Step.id values. Align the route definition with Step.id by either requiring a
matching id whenever humanTask is configured or removing the restrictive pattern
and length constraint to accept the actual supported step-ID format; preserve
path-parameter validity.
Comment on lines
+1729
to
+1782
| func preservesCompletedManualActions(current, incoming *exec.DAGRunStatus) bool { | ||
| if current == nil || incoming == nil { | ||
| return false | ||
| } | ||
| incomingNodes := make(map[string]*exec.Node, len(incoming.Nodes)) | ||
| for _, node := range incoming.Nodes { | ||
| if node != nil { | ||
| incomingNodes[manualActionNodeKey(node.Step)] = node | ||
| } | ||
| } | ||
| for _, node := range current.Nodes { | ||
| if node == nil { | ||
| continue | ||
| } | ||
| completedHumanTask := node.Step.HumanTask != nil && len(node.HumanTaskInput) > 0 | ||
| completedApproval := node.Step.Approval != nil && node.ApprovedAt != "" | ||
| pushedBack := node.ApprovalIteration > 0 | ||
| if !completedHumanTask && !completedApproval && !pushedBack { | ||
| continue | ||
| } | ||
| next := incomingNodes[manualActionNodeKey(node.Step)] | ||
| if next == nil { | ||
| return false | ||
| } | ||
| if (completedHumanTask || completedApproval) && | ||
| (next.Status != node.Status || next.FinishedAt != node.FinishedAt) { | ||
| return false | ||
| } | ||
| if completedHumanTask && | ||
| (next.Step.HumanTask == nil || | ||
| !bytes.Equal(next.HumanTaskInput, node.HumanTaskInput) || | ||
| next.HumanTaskCompletedBy != node.HumanTaskCompletedBy || | ||
| next.HumanTaskCompletedByID != node.HumanTaskCompletedByID || | ||
| !equalOptionalString(next.StepOutputsValue, node.StepOutputsValue)) { | ||
| return false | ||
| } | ||
| if completedApproval && | ||
| (next.Step.Approval == nil || | ||
| next.ApprovedAt != node.ApprovedAt || | ||
| next.ApprovedBy != node.ApprovedBy || | ||
| next.ApprovedByID != node.ApprovedByID || | ||
| !maps.Equal(next.ApprovalInputs, node.ApprovalInputs)) { | ||
| return false | ||
| } | ||
| if pushedBack && | ||
| (next.ApprovalIteration != node.ApprovalIteration || | ||
| !maps.Equal(next.PushBackInputs, node.PushBackInputs) || | ||
| !reflect.DeepEqual(next.PushBackHistory, node.PushBackHistory) || | ||
| next.PushBackPreviousStdout != node.PushBackPreviousStdout) { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for where wait-step rejection is set and whether the run remains Waiting afterward.
rg -nP --type=go -C3 '\bRejectedAt\b\s*[:=]'
rg -nP --type=go -C4 'NodeRejected|RejectionReason'Repository: dagucloud/dagu
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -u
echo "tracked go files near coordinator/exec:"
git ls-files | rg '(^|/)(handler\.go|node\.go|.*exec.*|.*coordinator.*|.*approval.*|.*manual.*|.*wait.*)\.go$' || true
echo
echo "Search rejection symbols (all files):"
rg -n -C3 -i 'Rejected|Rejection|ApprovalIteration|ApprovedAt|Waiting' . --glob '*.go' || true
echo
echo "Locate handler.go:"
fd -a 'handler.go$' . | sed -n '1,50p'Repository: dagucloud/dagu
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -u
echo "Find approval/wait/rejection related symbols in Go files:"
rg -n -C3 --glob '*.go' 'ApprovalIteration|ApprovedAt|ApprovedBy|RejectedAt|RejectedBy|RejectedByID|RejectionReason|NodeRejected|approval|Approval|HumanTask|manualActionNodeKey|preservesCompletedManualActions' .
echo
echo "Candidate exec node/status files:"
git ls-files | rg '(^|/)(node|status).*\.go$|exec/.*\.go$|core/stage.*\.go$|core/step.*\.go$' | head -200
echo
echo "Inspect handler function slice:"
sed -n '1720,1786p' internal/service/coordinator/handler.goRepository: dagucloud/dagu
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -u
echo "Internal exec node fields:"
cat -n internal/core/exec/node.go | sed -n '1,120p'
echo
echo "Internal runtime node state fields:"
sed -n '1,170p' internal/runtime/node.go | cat -n
echo
echo "Guard implementation:"
sed -n '1740,1786p' internal/service/coordinator/handler.go | cat -n
echo
echo "Waiting-regression rejection tests:"
sed -n '3082,3315p' internal/service/coordinator/handler_test.go | cat -n
echo
echo "Rejected-waiting coexistence evidence (focused):"
rg -n -C3 --glob '*.go' 'rejected and waiting together|HasWaiting|HasRejected|NodeRejected|RejectedAt|Status == core.Waiting|core.Waiting' internal/runtime internal/core/exec internal/service/coordinator/handler.go | head -250
echo
echo "Any current wait-step reject writers:"
rg -n -C3 --glob '*.go' 'NodeRejected|RejectedAt|RejectionReason' internal/cmd internal/runtime internal/humantask internal/service | head -300Repository: dagucloud/dagu
Length of output: 50370
Preserve persisted wait-step rejections against regression. preservesCompletedManualActions only protects human-task completion, approval (ApprovedAt), and push-back (ApprovalIteration). A waited step can be rejected without approval iteration by recording RejectedAt/RejectedBy/RejectedByID/RejectionReason; that state can coexist with another NodeWaiting dependency (HasRejected/HasWaiting). Once wait-step rejection is persisted, reject incoming Waiting reports that regress it to waiting, unless the run status is also changing to Rejected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/coordinator/handler.go` around lines 1729 - 1782, Update
preservesCompletedManualActions to recognize persisted wait-step rejection via
RejectedAt, RejectedBy, RejectedByID, or RejectionReason, including rejection
states coexisting with HasRejected/HasWaiting. When such a current node exists,
reject incoming reports that regress the node to Waiting unless the incoming run
status is Rejected, while preserving the existing manual-action checks.
Comment on lines
+152
to
+160
| func requireApprovalNode(node *exec.Node, stepName string) error { | ||
| if node == nil || node.Step.HumanTask != nil { | ||
| return fmt.Errorf("%w: step %s is a human task", errManualStepHumanTask, stepName) | ||
| } | ||
| if node.Step.Approval == nil { | ||
| return fmt.Errorf("%w: step %s does not have approval configuration", errManualStepNotApproval, stepName) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Nil-node branch reports a misleading "is a human task" error.
node == nil and node.Step.HumanTask != nil are collapsed into one branch/message. When node is nil (nodes slices elsewhere in this file, e.g. rollbackPushBack, treat nil entries as a real possibility), the returned error incorrectly claims "step %s is a human task" instead of reporting a missing/nil node.
🩹 Suggested fix
func requireApprovalNode(node *exec.Node, stepName string) error {
- if node == nil || node.Step.HumanTask != nil {
+ if node == nil {
+ return fmt.Errorf("step %s not found", stepName)
+ }
+ if node.Step.HumanTask != nil {
return fmt.Errorf("%w: step %s is a human task", errManualStepHumanTask, stepName)
}
if node.Step.Approval == nil {
return fmt.Errorf("%w: step %s does not have approval configuration", errManualStepNotApproval, stepName)
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func requireApprovalNode(node *exec.Node, stepName string) error { | |
| if node == nil || node.Step.HumanTask != nil { | |
| return fmt.Errorf("%w: step %s is a human task", errManualStepHumanTask, stepName) | |
| } | |
| if node.Step.Approval == nil { | |
| return fmt.Errorf("%w: step %s does not have approval configuration", errManualStepNotApproval, stepName) | |
| } | |
| return nil | |
| } | |
| func requireApprovalNode(node *exec.Node, stepName string) error { | |
| if node == nil { | |
| return fmt.Errorf("step %s not found", stepName) | |
| } | |
| if node.Step.HumanTask != nil { | |
| return fmt.Errorf("%w: step %s is a human task", errManualStepHumanTask, stepName) | |
| } | |
| if node.Step.Approval == nil { | |
| return fmt.Errorf("%w: step %s does not have approval configuration", errManualStepNotApproval, stepName) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/frontend/api/v1/manual_steps.go` around lines 152 - 160,
Update requireApprovalNode to handle node == nil separately and return the
appropriate missing-node error for the step. Keep the existing human-task error
only for non-nil nodes whose Step.HumanTask is configured, while preserving the
approval configuration validation for other nodes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Context
Builds on #2396, which introduced the
human.taskruntime action. This PR exposes that action through the API and web UI while keeping completion durable, retry-safe, and consistent for local and distributed runs.Changes
Related Issues
Validation
make test— 12,303 tests passed; 54 platform/environment skipsmake lintmake api-validatemake binDAGU_BIN=.local/bin/dagu go test ./conformance/spec031_human_task -count=1pnpm test— 112 files, 467 testspnpm typecheckpnpm buildChecklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
human.taskaction and updated status descriptions to “Waiting for manual action.”