feat: isolate environment variables of workers #1581
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds an immutable EnvScope and threads context-aware env/command evaluation through build and runtime; defers build-time env/shell/auth expansion to runtime; prevents process-wide os.Setenv; adds unexported helpers to restore a DAG from a prior run and integrates them into start/retry/restart flows. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/runtime/agent/reporter_test.go (1)
451-456: Remove unusedminhelper function.The
minfunction is unused in this file and redundant with the built-inminintroduced in Go 1.21. The project targets Go 1.25, so this local definition can be safely removed.internal/core/spec/loader.go (1)
169-181:Load()does not wirebuildEnvtoBuildOpts, unlikeLoadYAML().
LoadYAML()at line 198 passesoptions.buildEnvtoBuildOpts.BuildEnv, butLoad()does not includeBuildEnvin theBuildOptsstruct. IfWithBuildEnvis used withLoad(), it will be silently ignored.Is this intentional? If
Load()should not supportBuildEnv, consider documenting this limitation or panicking ifoptions.buildEnvis non-empty. Otherwise, add the missing field:Proposed fix
buildContext := BuildContext{ ctx: ctx, opts: BuildOpts{ Base: options.baseConfig, Parameters: options.params, ParametersList: options.paramsList, Name: options.name, DAGsDir: options.dagsDir, DefaultWorkingDir: options.defaultWorkingDir, Flags: options.flags, + BuildEnv: options.buildEnv, }, }
🤖 Fix all issues with AI agents
In `@internal/cmn/cmdutil/cmdutil_test.go`:
- Around line 824-844: The round-trip test in TestJoinSplitCommandArgsRoundTrip
misses the empty-args edge case; update SplitCommandArgs so that when the joined
string has an empty args portion it returns nil (or an empty slice consistently)
instead of [""], and then add the failing case {"pwd", []string{}} to the test
to assert JoinCommandArgs("pwd", []string{}) round-trips to ("pwd", nil) or
("pwd", []string{}) depending on your chosen convention; adjust assertions in
TestJoinSplitCommandArgsRoundTrip to expect the new behavior and ensure
JoinCommandArgs and SplitCommandArgs use the same empty-args contract.
In `@internal/cmn/cmdutil/envscope_test.go`:
- Around line 431-435: Replace the direct nil context in TestGetEnvScope with a
non-nil placeholder by calling context.TODO() when invoking GetEnvScope (i.e.,
use GetEnvScope(context.TODO()) in the "NilContext" subtest) or, if the goal is
to assert explicit nil-handling, add a comment explaining that this subtest
intentionally verifies nil handling; reference the TestGetEnvScope test and the
GetEnvScope function when making the change.
In `@internal/cmn/cmdutil/expand_test.go`:
- Around line 45-52: The test passes a nil Context to ExpandEnvContext which
triggers SA1012; change the call in the test "NilContextFallsBackToOSExpandEnv"
to use context.TODO() instead of nil (i.e., ExpandEnvContext(context.TODO(),
...)) to satisfy the linter, or if you intentionally want to assert nil
handling, add a brief comment above the test explaining why nil is used and
suppress the lint warning; ensure the reference to ExpandEnvContext remains
unchanged.
In `@internal/core/dag.go`:
- Around line 329-336: The dotenvLoaded check in DAG.LoadDotEnv is not atomic
and can race; replace the boolean guard with a sync.Once to ensure the load runs
exactly once: add a field (e.g. dotenvOnce sync.Once) on the DAG struct and call
dotenvOnce.Do(func(){ ...existing LoadDotEnv body that mutates d.Env... })
inside LoadDotEnv, removing direct reads/writes to dotenvLoaded so concurrent
calls cannot duplicate entries in d.Env.
In `@internal/runtime/builtin/chat/executor.go`:
- Around line 251-256: Add a nil check for EnvScope before calling AllSecrets in
maskSecretsForProvider: retrieve the DAG context via runtime.GetDAGContext(ctx),
return msgs immediately if e.EnvScope is nil, otherwise call
e.EnvScope.AllSecrets() and proceed; update maskSecretsForProvider to mirror the
defensive checks used by UserEnvsMap()/AllEnvs().
🧹 Nitpick comments (7)
internal/cmn/cmdutil/substitute_test.go (2)
120-136: Consider using testify assertions for consistency.The error messages contain awkward formatting like
"substituteCommandsWithContext(context.Background(),)"with a trailing comma before the closing parenthesis. While functional, this looks like a find-replace artifact. More importantly, this test file mixest.Errorfwith testify assertions (assert/require) used elsewhere in the file.♻️ Suggested improvement using testify for consistency
// Run test got, err := substituteCommandsWithContext(context.Background(), tt.input) // Check error - if (err != nil) != tt.wantErr { - t.Errorf("substituteCommandsWithContext(context.Background(),) error = %v, wantErr %v", err, tt.wantErr) - return - } - - // If we expect an error, don't check the output - if tt.wantErr { - return - } - - // Compare output - if got != tt.want { - t.Errorf("substituteCommandsWithContext(context.Background(),) = %q, want %q", got, tt.want) + if tt.wantErr { + assert.Error(t, err) + } else { + require.NoError(t, err) + assert.Equal(t, tt.want, got) }
180-191: Same formatting issue in edge case tests.The error message formatting inconsistency continues here. As per coding guidelines, prefer
stretchr/testify/requirefor consistency across test files.internal/core/spec/variables.go (1)
71-106: Inefficient scope recreation on each iteration.A new
EnvScopeis created inside the loop for every pair. For DAGs with many environment variables, this creates O(n) scope objects each copying OS environment and accumulated vars.Consider building the scope once outside the loop and using
WithEntryto chain immutably:♻️ Suggested optimization
func evaluatePairs(ctx BuildContext, pairs []pair) (map[string]string, error) { vars := make(map[string]string, len(pairs)) + + // Build scope once with OS env, then chain entries immutably + scope := cmdutil.NewEnvScope(nil, true) + + // Create evaluation context - handle nil parent context + evalCtx := ctx.ctx + if evalCtx == nil { + evalCtx = context.Background() + } for _, p := range pairs { value := p.val if !ctx.opts.Has(BuildFlagNoEval) { - // Build an EnvScope with OS env + accumulated vars for evaluation. - // This ensures command substitution and variable expansion work correctly - // without mutating the global OS environment. - scope := cmdutil.NewEnvScope(nil, true) - if len(vars) > 0 { - scope = scope.WithEntries(vars, cmdutil.EnvSourceDAGEnv) - } - - // Create evaluation context - handle nil parent context - evalCtx := ctx.ctx - if evalCtx == nil { - evalCtx = context.Background() - } - evalCtx = cmdutil.WithEnvScope(evalCtx, scope) + // Update scope with accumulated vars for this iteration + currentScope := scope.WithEntries(vars, cmdutil.EnvSourceDAGEnv) + scopedCtx := cmdutil.WithEnvScope(evalCtx, currentScope) var err error - value, err = cmdutil.EvalString(evalCtx, value, cmdutil.WithVariables(vars)) + value, err = cmdutil.EvalString(scopedCtx, value, cmdutil.WithVariables(vars)) if err != nil { return nil, core.NewValidationError("env", p.val, fmt.Errorf("%w: %s", ErrInvalidEnvValue, p.val)) } } vars[p.key] = value + // Chain the new entry for next iteration + scope = scope.WithEntry(p.key, value, cmdutil.EnvSourceDAGEnv) } return vars, nil }internal/cmn/cmdutil/envscope_test.go (1)
719-725: Helper functionmustpanics on failure.While acceptable in test code, consider using
requireassertions from testify instead, which provide better error messages and integrate with the test framework.♻️ Alternative using testify
-// Helper function for test assertions -func must(value string, ok bool) string { - if !ok { - panic("expected value to be present") - } - return value -} +// Helper function for test assertions - use with care in tests only +func must(value string, ok bool) string { + if !ok { + // In production test code, prefer require.True(t, ok) instead + panic("expected value to be present") + } + return value +}Or refactor usages to inline assertions:
val, ok := scope.Get("A") require.True(t, ok) assert.Equal(t, "1", val)internal/cmn/cmdutil/eval.go (1)
686-705: Intentional exclusion of OS-sourced vars inreplaceVarsWithScope.The logic to skip
EnvSourceOSentries and only expand user-defined vars is intentional—it allowsexpandWithShellContextto read live OS environment values later. This is a subtle but important design decision that should be documented.Consider adding a brief comment explaining why OS vars are skipped here:
📝 Documentation suggestion
// replaceVarsWithScope substitutes $VAR and ${VAR} patterns using EnvScope. // Only expands USER-defined vars (not OS-sourced), allowing live OS env // reads via expandWithShellContext later. +// +// Why skip OS vars? The EnvScope captures OS env at scope creation time, +// but we want shell expansion to use current OS values. By skipping OS-sourced +// entries here, we defer their expansion to expandWithShellContext which +// calls os.Getenv directly. func replaceVarsWithScope(template string, scope *EnvScope) string {internal/runtime/agent/reporter.go (2)
110-116: Consider usinghtml.EscapeStringfrom the standard library.The custom
escapeHTMLfunction doesn't escape single quotes ('), which could be problematic if values are used in single-quoted HTML attributes. The standard library'shtml.EscapeStringhandles all necessary escaping.♻️ Suggested refactor
+import "html" + // escapeHTML escapes special HTML characters in a string. func escapeHTML(s string) string { - s = strings.ReplaceAll(s, "&", "&") - s = strings.ReplaceAll(s, "<", "<") - s = strings.ReplaceAll(s, ">", ">") - return s + return html.EscapeString(s) }
284-302: Consider escaping timestamp values for defense-in-depth.While
StartedAtandFinishedAtare likely internally formatted timestamps, escaping them would provide defense-in-depth against potential injection if the format ever changes.♻️ Optional defensive escaping
func writeNodeRow(buffer *bytes.Buffer, index int, n *exec.Node) { _, _ = buffer.WriteString("<tr>") _, _ = buffer.WriteString(fmt.Sprintf("<td class=\"row-number\">%d</td>", index+1)) _, _ = buffer.WriteString(fmt.Sprintf("<td class=\"step-name\">%s</td>", escapeHTML(n.Step.Name))) - _, _ = buffer.WriteString(fmt.Sprintf("<td class=\"timestamp\">%s</td>", n.StartedAt)) - _, _ = buffer.WriteString(fmt.Sprintf("<td class=\"timestamp\">%s</td>", n.FinishedAt)) + _, _ = buffer.WriteString(fmt.Sprintf("<td class=\"timestamp\">%s</td>", escapeHTML(n.StartedAt))) + _, _ = buffer.WriteString(fmt.Sprintf("<td class=\"timestamp\">%s</td>", escapeHTML(n.FinishedAt)))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
internal/cmd/helper.gointernal/cmd/restart.gointernal/cmd/retry.gointernal/cmd/start.gointernal/cmn/cmdutil/cmdutil.gointernal/cmn/cmdutil/cmdutil_test.gointernal/cmn/cmdutil/envscope.gointernal/cmn/cmdutil/envscope_test.gointernal/cmn/cmdutil/eval.gointernal/cmn/cmdutil/eval_obj_test.gointernal/cmn/cmdutil/eval_test.gointernal/cmn/cmdutil/expand.gointernal/cmn/cmdutil/expand_test.gointernal/cmn/cmdutil/substitute.gointernal/cmn/cmdutil/substitute_test.gointernal/cmn/cmdutil/substitute_unix_test.gointernal/cmn/secrets/env.gointernal/core/container.gointernal/core/dag.gointernal/core/dag_security_test.gointernal/core/exec/context.gointernal/core/spec/builder.gointernal/core/spec/builder_test.gointernal/core/spec/dag.gointernal/core/spec/dag_test.gointernal/core/spec/loader.gointernal/core/spec/params.gointernal/core/spec/variables.gointernal/runtime/agent/agent.gointernal/runtime/agent/reporter.gointernal/runtime/agent/reporter_test.gointernal/runtime/builtin/chat/executor.gointernal/runtime/builtin/docker/eval_test.gointernal/runtime/builtin/gha/executor.gointernal/runtime/debug_test.gointernal/runtime/env.gointernal/runtime/env_test.gointernal/runtime/eval_test.gointernal/runtime/executor/dag_runner_test.gointernal/runtime/node.gointernal/runtime/node_test.gointernal/runtime/output.gointernal/runtime/runner.gointernal/service/frontend/server.gointernal/test/server.go
💤 Files with no reviewable changes (3)
- internal/runtime/executor/dag_runner_test.go
- internal/cmn/cmdutil/cmdutil.go
- internal/runtime/debug_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/output.gointernal/runtime/builtin/chat/executor.gointernal/cmn/cmdutil/substitute_test.gointernal/runtime/builtin/docker/eval_test.gointernal/cmd/restart.gointernal/runtime/node.gointernal/cmn/cmdutil/expand.gointernal/core/spec/builder_test.gointernal/cmn/cmdutil/eval_obj_test.gointernal/cmd/helper.gointernal/cmd/retry.gointernal/cmn/cmdutil/expand_test.gointernal/cmd/start.gointernal/cmn/cmdutil/envscope_test.gointernal/cmn/cmdutil/substitute.gointernal/service/frontend/server.gointernal/runtime/eval_test.gointernal/core/spec/builder.gointernal/cmn/cmdutil/eval_test.gointernal/cmn/cmdutil/eval.gointernal/core/dag_security_test.gointernal/runtime/runner.gointernal/core/spec/variables.gointernal/runtime/env_test.gointernal/cmn/secrets/env.gointernal/core/exec/context.gointernal/core/spec/dag.gointernal/core/spec/dag_test.gointernal/core/container.gointernal/cmn/cmdutil/cmdutil_test.gointernal/runtime/agent/agent.gointernal/core/spec/params.gointernal/runtime/builtin/gha/executor.gointernal/core/dag.gointernal/runtime/node_test.gointernal/cmn/cmdutil/substitute_unix_test.gointernal/runtime/env.gointernal/runtime/agent/reporter.gointernal/cmn/cmdutil/envscope.gointernal/core/spec/loader.gointernal/test/server.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/cmn/cmdutil/substitute_test.gointernal/runtime/builtin/docker/eval_test.gointernal/core/spec/builder_test.gointernal/cmn/cmdutil/eval_obj_test.gointernal/cmn/cmdutil/expand_test.gointernal/cmn/cmdutil/envscope_test.gointernal/runtime/eval_test.gointernal/cmn/cmdutil/eval_test.gointernal/core/dag_security_test.gointernal/runtime/env_test.gointernal/core/spec/dag_test.gointernal/cmn/cmdutil/cmdutil_test.gointernal/runtime/node_test.gointernal/cmn/cmdutil/substitute_unix_test.gointernal/runtime/agent/reporter_test.go
🧠 Learnings (6)
📚 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/cmn/cmdutil/substitute_test.gointernal/core/spec/builder_test.gointernal/cmn/cmdutil/eval_obj_test.gointernal/cmn/cmdutil/expand_test.gointernal/cmn/cmdutil/envscope_test.gointernal/runtime/eval_test.gointernal/cmn/cmdutil/eval_test.gointernal/core/dag_security_test.gointernal/runtime/env_test.gointernal/core/spec/dag_test.gointernal/cmn/cmdutil/cmdutil_test.gointernal/cmn/cmdutil/substitute_unix_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/docker/eval_test.gointernal/cmn/cmdutil/eval_obj_test.gointernal/cmn/cmdutil/expand_test.gointernal/runtime/eval_test.gointernal/core/spec/dag_test.gointernal/cmn/cmdutil/cmdutil_test.gointernal/cmn/cmdutil/substitute_unix_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: `make test` (or `make test-coverage`) executes the Go suite via `gotestsum`; append `TEST_TARGET=./internal/...` to focus packages
Applied to files:
internal/core/spec/builder_test.gointernal/cmn/cmdutil/expand_test.gointernal/cmn/cmdutil/envscope_test.gointernal/runtime/env_test.gointernal/cmn/cmdutil/cmdutil_test.gointernal/cmn/cmdutil/substitute_unix_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 **/*.go : Backend entrypoint in `cmd/` orchestrates the scheduler and CLI; runtime, persistence, and service layers sit under `internal/*` (for example `internal/runtime`, `internal/persistence`)
Applied to files:
internal/runtime/eval_test.gointernal/core/spec/builder.gointernal/runtime/env_test.gointernal/cmn/secrets/env.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: Commit summaries follow the Go convention `package: change` (lowercase package or area, present tense summary); keep body paragraphs wrapped at 72 chars when needed
Applied to files:
internal/core/dag.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 **/*.go : Keep Go files `gofmt`/`goimports` clean; use tabs, PascalCase for exported symbols (`SchedulerClient`), lowerCamelCase for locals, and `Err...` names for package-level errors
Applied to files:
internal/core/dag.go
🧬 Code graph analysis (28)
internal/runtime/output.go (2)
internal/runtime/context.go (1)
GetDAGContext(50-52)internal/cmn/cmdutil/envscope.go (1)
EnvScope(37-41)
internal/runtime/builtin/chat/executor.go (2)
internal/runtime/context.go (1)
GetDAGContext(50-52)internal/cmn/cmdutil/envscope.go (1)
EnvScope(37-41)
internal/runtime/builtin/docker/eval_test.go (1)
internal/cmn/cmdutil/envscope.go (2)
EnvSourceStepEnv(21-21)EnvSourceOutput(19-19)
internal/runtime/node.go (1)
internal/cmn/cmdutil/envscope.go (1)
EnvSourceStepEnv(21-21)
internal/cmn/cmdutil/expand.go (1)
internal/cmn/cmdutil/envscope.go (1)
GetEnvScope(217-225)
internal/core/spec/builder_test.go (1)
internal/runtime/builtin/command/shell.go (1)
Shell(13-20)
internal/cmn/cmdutil/eval_obj_test.go (1)
internal/runtime/eval.go (1)
EvalObject(26-28)
internal/cmd/retry.go (1)
internal/cmd/context.go (1)
Context(44-59)
internal/cmd/start.go (1)
internal/cmd/context.go (1)
Context(44-59)
internal/cmn/cmdutil/envscope_test.go (1)
internal/cmn/cmdutil/envscope.go (13)
NewEnvScope(45-58)EnvSourceDAGEnv(16-16)EnvSourceParam(18-18)EnvSourceOutput(19-19)EnvSource(12-12)EnvSourceOS(15-15)EnvSourceDotEnv(17-17)EnvSourceSecret(20-20)EnvSourceStepEnv(21-21)WithEnvScope(211-213)GetEnvScope(217-225)EnvSourceStep(25-25)EnvScope(37-41)
internal/cmn/cmdutil/substitute.go (3)
internal/cmn/cmdutil/cmdutil.go (1)
GetShellCommand(35-61)internal/cmn/cmdutil/envscope.go (1)
GetEnvScope(217-225)internal/runtime/env.go (1)
Env(23-48)
internal/runtime/eval_test.go (1)
internal/cmn/cmdutil/envscope.go (1)
EnvSourceStepEnv(21-21)
internal/core/spec/builder.go (1)
internal/cmn/cmdutil/envscope.go (1)
EnvScope(37-41)
internal/cmn/cmdutil/eval_test.go (2)
internal/cmn/cmdutil/eval.go (5)
EvalOption(38-38)WithoutSubstitute(64-68)NewEvalOptions(30-36)WithVariables(40-44)WithoutExpandShell(58-62)internal/cmn/cmdutil/envscope.go (5)
EnvScope(37-41)NewEnvScope(45-58)EnvSourceDAGEnv(16-16)EnvSourceStepEnv(21-21)EnvSourceSecret(20-20)
internal/cmn/cmdutil/eval.go (3)
internal/core/exec/context.go (1)
Context(17-28)internal/cmn/cmdutil/envscope.go (3)
GetEnvScope(217-225)EnvScope(37-41)EnvSourceOS(15-15)internal/cmn/cmdutil/expand.go (1)
ExpandEnvContext(10-16)
internal/core/spec/variables.go (4)
internal/core/spec/types/env.go (1)
EnvValue(29-33)internal/cmn/cmdutil/envscope.go (3)
NewEnvScope(45-58)EnvSourceDAGEnv(16-16)WithEnvScope(211-213)internal/cmn/cmdutil/eval.go (2)
EvalString(154-217)WithVariables(40-44)internal/core/spec/errors.go (1)
ErrInvalidEnvValue(27-27)
internal/runtime/env_test.go (3)
internal/runtime/env.go (2)
Env(23-48)NewEnv(72-98)internal/cmn/cmdutil/envscope.go (2)
EnvSourceStepEnv(21-21)EnvSourceOutput(19-19)internal/core/exec/context.go (1)
Context(17-28)
internal/cmn/secrets/env.go (2)
internal/core/dag.go (1)
SecretRef(198-207)internal/cmn/cmdutil/envscope.go (1)
GetEnvScope(217-225)
internal/core/exec/context.go (2)
internal/cmn/cmdutil/envscope.go (4)
EnvScope(37-41)NewEnvScope(45-58)EnvSourceDAGEnv(16-16)EnvSourceSecret(20-20)internal/runtime/env.go (1)
AllEnvs(365-367)
internal/core/spec/dag_test.go (3)
internal/core/dag.go (1)
AuthConfig(471-479)internal/runtime/data.go (1)
Parallel(85-89)internal/core/spec/builder.go (1)
BuildContext(14-29)
internal/core/container.go (1)
internal/runtime/env.go (1)
Env(23-48)
internal/cmn/cmdutil/cmdutil_test.go (1)
internal/cmn/cmdutil/cmdutil.go (3)
ArgsDelimiter(16-16)JoinCommandArgs(20-22)SplitCommandArgs(25-32)
internal/core/dag.go (2)
api/v2/api.gen.go (2)
Step(1133-1199)DAG(429-459)internal/core/step.go (1)
Step(13-103)
internal/runtime/node_test.go (1)
internal/cmn/cmdutil/envscope.go (1)
EnvSourceStepEnv(21-21)
internal/runtime/env.go (4)
internal/core/exec/context.go (1)
Context(17-28)internal/runtime/context.go (2)
Context(14-14)GetDAGContext(50-52)internal/cmn/cmdutil/envscope.go (4)
EnvScope(37-41)NewEnvScope(45-58)EnvSourceStepEnv(21-21)WithEnvScope(211-213)internal/cmn/cmdutil/eval.go (2)
EvalString(154-217)EvalOption(38-38)
internal/runtime/agent/reporter.go (4)
internal/core/dag.go (2)
MailConfig(585-590)DAG(64-194)internal/core/exec/runstatus.go (1)
DAGRunStatus(36-62)internal/core/status.go (6)
Status(4-4)NodeFailed(67-67)Failed(9-9)Succeeded(11-11)PartiallySucceeded(13-13)Waiting(14-14)internal/runtime/data.go (1)
NodeData(24-27)
internal/cmn/cmdutil/envscope.go (1)
internal/core/exec/context.go (1)
Context(17-28)
internal/test/server.go (1)
internal/service/frontend/server.go (3)
Server(50-60)NewServer(85-202)WithListener(68-72)
🪛 GitHub Check: Check for spelling errors
internal/cmn/cmdutil/eval_test.go
[failure] 2334-2334:
Hel ==> Help, Hell, Heal
🪛 GitHub Check: Go Linter
internal/cmn/cmdutil/expand_test.go
[failure] 50-50:
SA1012: do not pass a nil Context, even if a function permits it; pass context.TODO if you are unsure about which Context to use (staticcheck)
internal/cmn/cmdutil/envscope_test.go
[failure] 433-433:
SA1012: do not pass a nil Context, even if a function permits it; pass context.TODO if you are unsure about which Context to use (staticcheck)
⏰ 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
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/runtime/agent/reporter.go`:
- Around line 476-487: The badge class map in reporter.go uses keys tied to
dagStatus.Status.String() (stored in statusStr) but the "waiting" status is
incorrectly keyed as "wait", so update the badgeClasses map to use "waiting":
"wait" -> "waiting" (i.e., change the map entry key for the waiting state to
"waiting"), and optionally add a safe default lookup or fallback when retrieving
badgeClasses[statusStr] to avoid writing an empty string if statusStr is
missing.
In `@internal/runtime/env.go`:
- Around line 149-160: In resolveExpandedDir update the error path so you return
a safe fallback instead of an empty string: if fileutil.ResolvePath(expandedDir)
returns an error (or returns an empty dir), log the warning as you already do
and then return expandedDir (the un-resolved path) as the fallback; ensure this
change is applied in the resolveExpandedDir function so callers won’t receive an
empty working directory when ResolvePath fails.
🧹 Nitpick comments (2)
internal/cmn/cmdutil/substitute_test.go (2)
108-117: Consider usingt.Setenvfor consistency.Other tests in this file (lines 378, 421) use
t.Setenv, which automatically restores the original value after the test. This would simplify the setup and align with the pattern used elsewhere.♻️ Suggested simplification
// Setup environment if needed if tt.setupEnv != nil { for k, v := range tt.setupEnv { - oldValue := os.Getenv(k) - _ = os.Setenv(k, v) - defer func() { - _ = os.Setenv(k, oldValue) - }() + t.Setenv(k, v) } }Note: This also makes the
cleanupEnvfield in the test struct unnecessary.
268-278: Minor inconsistency in assertion pattern.Line 272 uses
assert.Error(t, err)while other tests (TestSubstituteCommandsline 124,TestSubstituteCommandsEdgeCasesline 178) userequire.Error(t, err)with an early return. While functionally equivalent here due to theif-elsestructure, usingrequirewith return would be consistent with the rest of the file.♻️ Suggested change for consistency
t.Run(tt.name, func(t *testing.T) { got, err := substituteCommandsWithContext(context.Background(), tt.input) if tt.wantErr { - assert.Error(t, err) - } else { - require.NoError(t, err) - assert.Equal(t, tt.want, got) + require.Error(t, err) + return } + require.NoError(t, err) + assert.Equal(t, tt.want, got) })
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
internal/cmn/cmdutil/cmdutil.gointernal/cmn/cmdutil/cmdutil_test.gointernal/cmn/cmdutil/envscope.gointernal/cmn/cmdutil/envscope_test.gointernal/cmn/cmdutil/eval.gointernal/cmn/cmdutil/substitute_test.gointernal/core/dag.gointernal/core/spec/loader.gointernal/core/spec/variables.gointernal/persis/filedagrun/store_test.gointernal/runtime/agent/reporter.gointernal/runtime/agent/reporter_test.gointernal/runtime/builtin/chat/executor.gointernal/runtime/env.gointernal/runtime/executor/dag_runner.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/cmn/cmdutil/cmdutil_test.go
- internal/core/spec/loader.go
- internal/cmn/cmdutil/envscope_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/executor/dag_runner.gointernal/cmn/cmdutil/substitute_test.gointernal/runtime/builtin/chat/executor.gointernal/persis/filedagrun/store_test.gointernal/cmn/cmdutil/cmdutil.gointernal/cmn/cmdutil/eval.gointernal/runtime/agent/reporter.gointernal/runtime/agent/reporter_test.gointernal/runtime/env.gointernal/core/dag.gointernal/core/spec/variables.gointernal/cmn/cmdutil/envscope.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/cmn/cmdutil/substitute_test.gointernal/persis/filedagrun/store_test.gointernal/runtime/agent/reporter_test.go
🧠 Learnings (6)
📚 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/cmn/cmdutil/substitute_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/cmn/cmdutil/substitute_test.gointernal/persis/filedagrun/store_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 **/*.go : Keep Go files `gofmt`/`goimports` clean; use tabs, PascalCase for exported symbols (`SchedulerClient`), lowerCamelCase for locals, and `Err...` names for package-level errors
Applied to files:
internal/core/dag.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: Commit summaries follow the Go convention `package: change` (lowercase package or area, present tense summary); keep body paragraphs wrapped at 72 chars when needed
Applied to files:
internal/core/dag.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 **/*.go : Repository linting relies on `golangci-lint`; prefer idiomatic Go patterns, minimal global state, and structured logging helpers in `internal/common`
Applied to files:
internal/core/dag.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 **/*.go : Backend entrypoint in `cmd/` orchestrates the scheduler and CLI; runtime, persistence, and service layers sit under `internal/*` (for example `internal/runtime`, `internal/persistence`)
Applied to files:
internal/core/dag.go
🧬 Code graph analysis (5)
internal/runtime/builtin/chat/executor.go (2)
internal/runtime/context.go (1)
GetDAGContext(50-52)internal/cmn/cmdutil/envscope.go (1)
EnvScope(37-41)
internal/cmn/cmdutil/eval.go (3)
internal/core/exec/context.go (1)
Context(17-28)internal/cmn/cmdutil/envscope.go (3)
GetEnvScope(217-225)EnvScope(37-41)EnvSourceOS(15-15)internal/cmn/cmdutil/expand.go (1)
ExpandEnvContext(10-16)
internal/runtime/agent/reporter.go (3)
internal/core/dag.go (3)
MailConfig(594-599)DAG(65-195)MailOn(579-583)internal/core/exec/runstatus.go (1)
DAGRunStatus(36-62)internal/core/status.go (6)
Status(4-4)NodeFailed(67-67)Failed(9-9)Succeeded(11-11)PartiallySucceeded(13-13)Waiting(14-14)
internal/core/dag.go (4)
internal/runtime/env.go (1)
Env(23-48)internal/cmn/logger/tag/tag.go (3)
Name(276-278)Dir(82-84)File(77-79)internal/cmn/cmdutil/eval.go (1)
EvalString(154-217)internal/runtime/eval.go (1)
EvalString(14-16)
internal/cmn/cmdutil/envscope.go (1)
internal/core/exec/context.go (1)
Context(17-28)
⏰ 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 (37)
internal/cmn/cmdutil/substitute_test.go (2)
119-130: LGTM! Context-aware substitution and assertion updates look correct.The migration to
substituteCommandsWithContextwithcontext.Background()is appropriate for tests. The assertion pattern usingrequirefor error checks andassert.Equalfor value comparison follows best practices.
174-184: LGTM! Consistent update to context-aware API with proper assertions.The edge case tests correctly mirror the assertion pattern used in
TestSubstituteCommands. Based on learnings, this follows the guideline to usestretchr/testify/requirefor error handling.internal/cmn/cmdutil/cmdutil.go (1)
31-34: Confirm empty-argument semantics.This branch treats a trailing space (
"cmd ") as “no args,” whereas previously it would produce a single empty arg ([]string{""}). If empty-string args are meaningful in any call paths, this change will silently drop them—please confirm that’s acceptable.internal/runtime/builtin/chat/executor.go (1)
252-257: Good defensive guard for nil EnvScope.Prevents a potential panic when the context lacks a scope and keeps masking behavior safe.
internal/persis/filedagrun/store_test.go (1)
306-309: Field-level assertions make this test stable.Avoids copying sync.Once while still validating key DAG identity fields.
internal/runtime/agent/reporter_test.go (1)
72-77: Constructor-based setup looks good.Keeps reporter initialization consistent with production code paths.
internal/runtime/agent/reporter.go (6)
37-58: Centralized reporter configuration is clean.The constructor makes reporter initialization clearer and more testable.
65-71: Per-step error mail routing is clear and consistent.Using the configured mail settings plus attachments reads well.
78-109: Mail selection/sending refactor is tidy.The split into select/send helpers improves readability and reuse.
111-124: statusToClass helper keeps rendering consistent.Nice encapsulation for CSS mapping.
270-288: Row renderer now safely escapes user-controlled fields.This is a good safety and readability improvement.
494-554: DAG field escaping + row reuse improves safety and consistency.Good move to escape DAG values and reuse the row renderer for uniform output.
internal/cmn/cmdutil/envscope.go (2)
11-107: EnvScope core API looks solid.The immutable layering helpers and source tracking are clear and consistent.
109-304: Lookup/expansion/provenance helpers are well-rounded.The context integration and query utilities round out the scope cleanly.
internal/cmn/cmdutil/eval.go (9)
79-88: Centralized regex helpers for variable/JSON-path parsing look good.Nice consolidation; should make expansion logic easier to maintain.
91-124: Argument quoting helper is a solid improvement.Early return plus
quoteArgIfNeededmakes command reconstruction safer and clearer.
126-150: Scope-aware variable expansion order looks correct.Step refs → explicit vars → scope expansion is clear and predictable.
165-215: Context-aware substitution + shell expansion fallback is clean.The new flow and fallback logging read well.
227-246: EvalIntString updates fit the new context-aware pipeline.
290-304: Consolidating struct/map/slice evaluation viaevalStringValueis good.Nice reduction of duplicated logic across containers.
Also applies to: 324-329, 369-377, 424-430, 479-483
612-657: JSON-path expansion now covers step refs and EnvScope cleanly.
660-715: Variable replacement helpers read well (single-quote skip + OS deferral).
718-757: Shell expansion now respects scoped env with OS fallback.internal/core/spec/variables.go (2)
13-69: Good shift to returning evaluated vars without touching process env.
71-109: Scope-chained evaluation per pair is clean and thread-safe.internal/core/dag.go (4)
64-195: Hiding build-time secrets from JSON is a solid safeguard.
210-225: Clone +slices.Containsupdates look good.
254-262: GetName simplification is clear and correct.
341-395: Idempotent dotenv loading + noos.Setenvis a win.internal/runtime/executor/dag_runner.go (1)
81-87: UsingDAG.Clone()keeps local DAG env isolation intact.internal/runtime/env.go (7)
29-41: Scope as the single source of truth is clearly documented.
51-67: AllEnvs/UserEnvsMap now delegate to Scope cleanly.
73-97: NewEnv scope chaining with step envs looks good.
173-190: fallbackWorkingDir is a sensible safety net.
192-272: Shell/DAGShell evaluation with scope is solid.
289-315: MailerConfig/EvalString now correctly use Scope.
333-378: Scope-based env mutations and maps look good.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1581 +/- ##
==========================================
+ Coverage 64.64% 65.08% +0.44%
==========================================
Files 255 258 +3
Lines 28519 28645 +126
==========================================
+ Hits 18435 18643 +208
+ Misses 8430 8332 -98
- Partials 1654 1670 +16
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.