fix(runtime): decode log tail in proper encoding#1449
Conversation
WalkthroughAdds charset-aware log decoding (new DecodeString) and threads a LogEncodingCharset through a refactored execution context API (DAGContext → Context, SetupDAGContext → NewContext with functional options). TailWriter stores raw bytes and decodes tails with the provided encoding; numerous runtime call sites and tests updated. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/runtime/env.go (1)
104-131: Minor redundancy:GetDAGContextis called twice.
execution.GetDAGContext(ctx)is called at line 106 to getparentEnv, and then called again at line 125 to initialize theContextfield. SinceparentEnvalready holds the result, you could reuse it:func NewEnvForStep(ctx context.Context, step core.Step) Env { parentEnv := execution.GetDAGContext(ctx) workingDir := resolveStepWorkingDir(ctx, step, parentEnv) envs := map[string]string{ execution.EnvKeyDAGRunStepName: step.Name, "PWD": workingDir, } variables := &collections.SyncMap{} if parentEnv.DAG != nil { for _, param := range parentEnv.DAG.Params { parts := strings.SplitN(param, "=", 2) if len(parts) == 2 { variables.Store(parts[0], param) } } } return Env{ - Context: execution.GetDAGContext(ctx), + Context: parentEnv, Variables: variables, Step: step, Envs: envs, StepMap: make(map[string]cmdutil.StepInfo), WorkingDir: workingDir, } }internal/runtime/eval_test.go (2)
14-15: Outdated comment: references removedSetupDAGContextfunction.The comment mentions
execution.SetupDAGContextbut the codebase has been refactored to useexecution.NewContext. Update or remove this comment for accuracy.-// Note: These tests use execution.SetupDAGContext to set up the DAG context, -// then use runtime.NewEnvForStep/GetEnv/WithEnv to manage the runtime Env. +// Note: These tests use execution.NewContext or execution.WithDAGContext to set up the DAG context, +// then use runtime.NewEnvForStep/GetEnv/WithEnv to manage the runtime Env.
17-26: Consider usingNewContextfor consistency.
setupTestContext()constructs anexecution.Contextstruct directly and usesWithDAGContext, while other tests in this file (lines 484, 634) useexecution.NewContext(). UsingNewContextensures theEnvsmap is properly initialized with standard keys likeEnvKeyDAGRunLogFile,EnvKeyDAGRunID, etc.This inconsistency may not cause test failures currently but could lead to subtle issues if tests start depending on those standard environment keys.
internal/runtime/env_test.go (1)
110-114: Consider migrating toNewContextfor consistency.These test cases still use direct
execution.Context{}struct construction withWithDAGContext. While this works, it differs from the newer pattern usingexecution.NewContext()seen elsewhere in the file. The direct construction skips auto-initialization of theEnvsmap with standard keys.This is a low-priority refactor suggestion for test consistency.
Also applies to: 252-255, 283-286, 323-326
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
internal/common/fileutil/logutil.go(1 hunks)internal/common/fileutil/logutil_test.go(1 hunks)internal/core/execution/context.go(3 hunks)internal/core/execution/context_test.go(3 hunks)internal/runtime/agent/agent.go(2 hunks)internal/runtime/builtin/command/command.go(1 hunks)internal/runtime/builtin/command/command_test.go(3 hunks)internal/runtime/builtin/docker/executor.go(1 hunks)internal/runtime/builtin/mail/mail_test.go(2 hunks)internal/runtime/debug_test.go(1 hunks)internal/runtime/env.go(5 hunks)internal/runtime/env_test.go(12 hunks)internal/runtime/eval_test.go(3 hunks)internal/runtime/executor/dag_runner.go(1 hunks)internal/runtime/executor/dag_runner_test.go(10 hunks)internal/runtime/executor/tail.go(5 hunks)internal/runtime/node_test.go(8 hunks)internal/runtime/output_test.go(5 hunks)internal/runtime/runner_helper_test.go(1 hunks)internal/runtime/runner_test.go(3 hunks)
🧰 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/common/fileutil/logutil.gointernal/runtime/builtin/docker/executor.gointernal/runtime/executor/dag_runner.gointernal/runtime/debug_test.gointernal/runtime/env.gointernal/runtime/executor/tail.gointernal/runtime/executor/dag_runner_test.gointernal/common/fileutil/logutil_test.gointernal/runtime/builtin/command/command_test.gointernal/runtime/runner_helper_test.gointernal/runtime/builtin/mail/mail_test.gointernal/runtime/agent/agent.gointernal/runtime/builtin/command/command.gointernal/runtime/node_test.gointernal/core/execution/context_test.gointernal/runtime/env_test.gointernal/runtime/output_test.gointernal/runtime/runner_test.gointernal/runtime/eval_test.gointernal/core/execution/context.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/runtime/debug_test.gointernal/runtime/executor/dag_runner_test.gointernal/common/fileutil/logutil_test.gointernal/runtime/builtin/command/command_test.gointernal/runtime/runner_helper_test.gointernal/runtime/builtin/mail/mail_test.gointernal/runtime/node_test.gointernal/core/execution/context_test.gointernal/runtime/env_test.gointernal/runtime/output_test.gointernal/runtime/runner_test.gointernal/runtime/eval_test.go
🧠 Learnings (1)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/common/fileutil/logutil_test.gointernal/runtime/node_test.go
🧬 Code graph analysis (18)
internal/runtime/builtin/docker/executor.go (2)
internal/runtime/env.go (1)
GetEnv(371-377)internal/runtime/executor/tail.go (1)
NewTailWriterWithEncoding(42-46)
internal/runtime/executor/dag_runner.go (1)
internal/core/execution/context.go (1)
Context(16-27)
internal/runtime/debug_test.go (2)
internal/core/execution/context.go (1)
NewContext(199-237)internal/core/dag.go (1)
DAG(33-135)
internal/runtime/env.go (1)
internal/core/execution/context.go (2)
Context(16-27)GetDAGContext(246-258)
internal/runtime/executor/tail.go (1)
internal/common/fileutil/logutil.go (1)
DecodeString(557-576)
internal/runtime/executor/dag_runner_test.go (1)
internal/core/execution/context.go (1)
Context(16-27)
internal/common/fileutil/logutil_test.go (1)
internal/common/fileutil/logutil.go (1)
DecodeString(557-576)
internal/runtime/builtin/command/command_test.go (1)
internal/core/execution/context.go (1)
NewContext(199-237)
internal/runtime/runner_helper_test.go (1)
internal/core/execution/context.go (2)
NewContext(199-237)Context(16-27)
internal/runtime/builtin/mail/mail_test.go (2)
internal/core/execution/context.go (1)
NewContext(199-237)internal/core/dag.go (2)
DAG(33-135)SMTPConfig(490-495)
internal/runtime/agent/agent.go (2)
internal/core/execution/context.go (6)
NewContext(199-237)WithDatabase(155-159)WithRootDAGRun(162-166)WithParams(169-173)WithCoordinator(176-180)WithSecrets(183-187)internal/core/params.go (1)
Params(36-45)
internal/runtime/builtin/command/command.go (2)
internal/runtime/env.go (1)
GetEnv(371-377)internal/runtime/executor/tail.go (1)
NewTailWriterWithEncoding(42-46)
internal/runtime/node_test.go (1)
internal/core/execution/context.go (2)
NewContext(199-237)Context(16-27)
internal/core/execution/context_test.go (1)
internal/core/execution/context.go (2)
NewContext(199-237)WithSecrets(183-187)
internal/runtime/env_test.go (1)
internal/core/execution/context.go (3)
Context(16-27)NewContext(199-237)WithSecrets(183-187)
internal/runtime/output_test.go (1)
internal/core/execution/context.go (1)
NewContext(199-237)
internal/runtime/runner_test.go (1)
internal/core/execution/context.go (2)
NewContext(199-237)Context(16-27)
internal/runtime/eval_test.go (1)
internal/core/execution/context.go (2)
Context(16-27)NewContext(199-237)
⏰ 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 (38)
internal/runtime/executor/dag_runner.go (1)
49-49: LGTM! Type refactor aligns with context API changes.The field type change from
execution.DAGContexttoexecution.Contextis consistent with the broader context API refactor across the codebase, and all usages throughout the file remain correct.internal/runtime/debug_test.go (1)
18-18: LGTM! Properly migrated to new context construction API.The update from
SetupDAGContexttoNewContextcorrectly uses the new builder-pattern constructor with the simplified signature.internal/common/fileutil/logutil_test.go (2)
839-929: Excellent test coverage for charset decoding!The table-driven test comprehensively covers multiple encodings (Japanese, Chinese, Korean, Latin) and edge cases (empty input, unknown charset). Well-structured and follows Go testing best practices.
931-953: Good test for charset name normalization!Testing that various Shift_JIS charset name variants (
shift_jis,shift-jis,shiftjis,sjis,s-jis) all decode correctly ensures robust user experience regardless of charset naming conventions.internal/common/fileutil/logutil.go (1)
553-576: LGTM! Well-designed utility with graceful error handling.The
DecodeStringfunction provides a clean API for charset-aware decoding with sensible fallback behavior. Returning the original bytes as a string when decoding fails ensures robustness without requiring callers to handle errors.internal/runtime/builtin/docker/executor.go (1)
140-144: LGTM! Properly integrated encoding-aware tail writer.The update correctly obtains the log encoding from the DAG context environment and passes it to
NewTailWriterWithEncoding, enabling proper decoding of non-UTF-8 stderr output in error messages.internal/runtime/executor/dag_runner_test.go (1)
38-44: LGTM! Consistent type refactor across all test cases.The changes from
execution.DAGContexttoexecution.Contextthroughout all test cases are mechanical and align with the broader context API refactor. All field assignments remain correct.Also applies to: 83-89, 129-135, 157-164, 207-214, 237-244, 344-348, 390-394, 432-436, 469-473
internal/runtime/builtin/command/command.go (1)
56-61: LGTM! Properly integrated encoding-aware tail writer.The update correctly obtains the log encoding from the DAG context environment and uses
NewTailWriterWithEncoding, matching the pattern used in the docker executor and enabling proper decoding of non-UTF-8 stderr output.internal/runtime/runner_helper_test.go (1)
204-204: LGTM! Properly migrated to new context construction API.The update from
SetupDAGContexttoNewContextcorrectly uses the new builder-pattern constructor. No options are needed for this basic test setup.internal/runtime/builtin/mail/mail_test.go (1)
77-79: LGTM!The migration from
SetupDAGContexttoNewContextis correct. The test only requires a DAG with SMTP configuration, and the empty strings fordagRunIDandlogFileare appropriate since these tests don't use those values.Also applies to: 152-154
internal/runtime/output_test.go (1)
67-67: LGTM!The migration to
NewContextis consistent across all test functions. Each test uses descriptivedagRunIDvalues that match their test purpose, making debugging easier.Also applies to: 135-135, 184-184, 220-220, 256-256
internal/runtime/executor/tail.go (3)
39-46: LGTM!The
NewTailWriterWithEncodingconstructor follows a clean delegation pattern, reusingNewTailWriterfor default initialization. The encoding field enables proper charset-aware decoding in theTail()method.
21-22: Good refactor:[]bytebuffer is more appropriate for raw byte handling.Switching from
stringto[]bytecorrectly separates the raw byte accumulation from the charset-aware decoding. Theappendapproach is also more efficient than repeated string concatenation.Also applies to: 61-64
72-78: LGTM!The
Tail()method correctly decodes the buffer using the specified encoding at read time. This is the right design choice as it:
- Preserves raw bytes until final output
- Handles the decoding under mutex protection for thread safety
- Gracefully falls back to UTF-8 for empty or unknown encodings
internal/runtime/runner_test.go (1)
1322-1322: LGTM!The migration to
NewContextis consistent across all test functions. Each call properly constructs the execution context with the appropriate DAG, run ID, and log file path.Also applies to: 1398-1398, 2803-2803
internal/runtime/builtin/command/command_test.go (1)
361-361: LGTM!The migration to
NewContextis consistent. ThesetupTestContexthelper function (line 536-548) centralizes context creation, reducing code duplication across tests. Using empty string forlogFileis appropriate since these command execution tests don't require log file functionality.Also applies to: 439-439, 545-545
internal/runtime/env.go (1)
28-28: LGTM! Clean embedding ofexecution.Context.The field rename from
DAGContexttoContextwith embeddedexecution.Contexttype aligns with the broader API refactor. This provides direct access to Context fields likeLogEncodingCharsetwhich supports the PR's goal of charset-aware log decoding.internal/runtime/eval_test.go (1)
484-484: LGTM! Correct usage of the newNewContextAPI.These test cases properly use
execution.NewContext(context.Background(), &core.DAG{}, "test-run", "test.log")which aligns with the new builder-pattern API.Also applies to: 634-634
internal/runtime/env_test.go (2)
353-356: LGTM! Correct usage ofNewContextAPI.Proper use of
execution.NewContext(ctx, dag, "test-run", "test.log")for test setup. The functional options pattern is clean and extensible.
368-370: LGTM! Proper use ofWithSecretsoption.The tests correctly use
execution.WithSecrets(secrets)to inject secret environment variables, which aligns with the new functional options pattern and properly tests the secret override behavior.Also applies to: 397-399
internal/core/execution/context_test.go (1)
12-82: LGTM! Well-structured tests for the newNewContextAPI.The test cases provide good coverage:
ExcludesOSEnvironment: Verifies OS env vars like PATH are not includedSecretOverridesEnvs: Validates that secrets take precedence over DAG env varsCombinesAllSources: Ensures both DAG env and secrets are combined correctlyThe consistent use of
execution.NewContext()withexecution.WithSecrets()demonstrates the functional options pattern working as intended.internal/runtime/agent/agent.go (2)
297-303: LGTM! Comprehensive context initialization.The main execution path correctly uses
execution.NewContextwith all necessary functional options:
WithDatabasefor DB accessWithRootDAGRunfor hierarchy trackingWithParamsfor DAG parametersWithCoordinatorfor distributed executionWithSecretsfor resolved secret environment variablesThis aligns well with the new builder-pattern API.
851-856: Verify: Dry-run omitsWithSecrets- is this intentional?The dry-run path doesn't pass
WithSecrets(secretEnvs)toNewContext, unlike the main execution path (line 302). This means steps referencing secret variables (e.g.,${MY_SECRET}) won't resolve during dry-run.If intentional (secrets shouldn't be resolved in dry-run for security), this is fine. If not, steps with secret references may show unexpected behavior during dry-run validation.
internal/runtime/node_test.go (8)
486-486: LGTM!Correct migration to the new
NewContextAPI. The test properly initializes the execution context with required parameters.
818-818: LGTM!Properly uses
NewContextto create the execution context with the DAG containingMaxOutputSizeconfiguration for output capture testing.
871-871: LGTM!Consistent API usage for the DAG max output size configuration tests.
1139-1141: LGTM!The helper method correctly creates execution context for test node execution. The hardcoded
"logFile"string is appropriate for test purposes.
1168-1168: LGTM!Correct context setup for the output redirect test with working directory configuration.
1208-1208: LGTM!Consistent usage of
NewContextfor relative path working directory test.
1253-1253: LGTM!Proper context initialization for stderr redirect test.
1299-1299: LGTM!Consistent API usage for nested relative path test case.
internal/core/execution/context.go (7)
15-27: LGTM! Clean struct definition with proper documentation.The
Contextstruct is well-organized with clear field documentation. The newLogEncodingCharsetfield properly supports the PR objective of charset-aware log decoding.
32-54: LGTM!Receiver type correctly updated from
DAGContexttoContext. The precedence logic (SecretEnvs > Envs > DAG.Env) is clearly documented and implemented.
57-59: LGTM!Clean method with updated receiver type.
64-86: LGTM!Correct receiver type update with unchanged logic.
141-194: LGTM! Clean functional options pattern implementation.This is idiomatic Go for optional configuration. Each option function is well-documented with its purpose, and the pattern provides good extensibility for future options.
196-237: LGTM! Well-designed constructor with clear separation of required and optional parameters.The
NewContextfunction:
- Clearly documents required vs optional parameters
- Properly applies functional options
- Builds environment variables with correct precedence
- Includes all necessary context fields including the new
LogEncodingCharset
241-258: LGTM!The accessor functions are updated to use the
Contexttype. The defensive error handling inGetDAGContextthat returns an emptyContext{}on failure is appropriate, as it logs the error and avoids panics.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @yottahmd. * #1449 (comment) The following files were modified: * `internal/common/fileutil/logutil.go` * `internal/core/execution/context.go` * `internal/runtime/env.go` * `internal/runtime/executor/tail.go`
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/runtime/env_test.go (1)
110-114: Consider usingNewContextfor test context setup.The test manually constructs
execution.Context{}with onlyDAGandBaseEnvfields, bypassing the initialization logic inNewContext. This leaves fields likeEnvsandSecretEnvsas nil maps, which could cause issues if the code under test expects initialized maps.For better test hygiene and forward compatibility, consider using
NewContext:-dagCtx := execution.Context{ - DAG: dag, - BaseEnv: &baseEnv, -} -ctx := execution.WithContext(context.Background(), dagCtx) +ctx := execution.NewContext(context.Background(), dag, "", "")This pattern is already used correctly in other tests (e.g., lines 353, 368-370, 397-399).
Note: This same pattern appears at lines 252-255, 283-286, and 323-326. Consider applying this refactor consistently across all test cases.
internal/runtime/eval_test.go (1)
17-26: Consider updatingsetupTestContextto useNewContext.The helper function manually constructs
execution.Context{}, bypassing initialization inNewContext. This could leave fields likeEnvsandSecretEnvsuninitialized.Consider updating the helper:
func setupTestContext() context.Context { - ctx := context.Background() - baseEnv := config.NewBaseEnv(nil) - dagCtx := execution.Context{ - DAG: &core.DAG{Name: "test-dag"}, - BaseEnv: &baseEnv, - } - return execution.WithContext(ctx, dagCtx) + return execution.NewContext( + context.Background(), + &core.DAG{Name: "test-dag"}, + "", // dagRunID + "", // logFile + ) }This ensures proper initialization of all context fields and aligns with the new functional-options pattern used elsewhere in the tests (e.g., lines 484, 634).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
internal/core/execution/context.go(3 hunks)internal/core/execution/context_test.go(4 hunks)internal/runtime/builtin/archive/archive.go(1 hunks)internal/runtime/builtin/command/command_test.go(3 hunks)internal/runtime/builtin/mail/mail.go(1 hunks)internal/runtime/condition.go(1 hunks)internal/runtime/condition_test.go(1 hunks)internal/runtime/env.go(10 hunks)internal/runtime/env_test.go(12 hunks)internal/runtime/eval_test.go(9 hunks)internal/runtime/executor/dag_runner.go(12 hunks)internal/runtime/executor/dag_runner_test.go(10 hunks)internal/runtime/node.go(1 hunks)internal/runtime/node_test.go(9 hunks)internal/runtime/output.go(2 hunks)internal/runtime/runner.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/core/execution/context_test.go
- internal/runtime/builtin/command/command_test.go
- internal/runtime/executor/dag_runner_test.go
- internal/runtime/node_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/condition.gointernal/runtime/builtin/archive/archive.gointernal/runtime/builtin/mail/mail.gointernal/runtime/node.gointernal/runtime/executor/dag_runner.gointernal/runtime/env_test.gointernal/runtime/output.gointernal/runtime/runner.gointernal/core/execution/context.gointernal/runtime/eval_test.gointernal/runtime/condition_test.gointernal/runtime/env.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/runtime/env_test.gointernal/runtime/eval_test.gointernal/runtime/condition_test.go
🧠 Learnings (1)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/runtime/condition_test.go
🧬 Code graph analysis (9)
internal/runtime/condition.go (2)
internal/core/execution/context.go (1)
GetContext(246-258)internal/core/dag.go (1)
DAG(33-135)
internal/runtime/builtin/mail/mail.go (1)
internal/runtime/env.go (1)
NewEnv(105-132)
internal/runtime/node.go (1)
internal/core/execution/context.go (1)
GetContext(246-258)
internal/runtime/executor/dag_runner.go (3)
internal/core/execution/context.go (2)
Context(16-27)GetContext(246-258)internal/core/dag.go (1)
DAG(33-135)internal/core/execution/dagrun.go (1)
DAGRunRef(144-147)
internal/runtime/env_test.go (2)
internal/core/execution/context.go (4)
Context(16-27)WithContext(241-243)NewContext(199-237)WithSecrets(183-187)internal/runtime/env.go (2)
NewEnv(105-132)Env(24-54)
internal/runtime/output.go (2)
internal/core/execution/context.go (1)
GetContext(246-258)internal/core/dag.go (1)
DAG(33-135)
internal/runtime/eval_test.go (2)
internal/core/execution/context.go (3)
Context(16-27)WithContext(241-243)NewContext(199-237)internal/runtime/env.go (1)
NewEnv(105-132)
internal/runtime/condition_test.go (1)
internal/runtime/env.go (2)
WithEnv(366-368)NewEnv(105-132)
internal/runtime/env.go (3)
internal/core/execution/context.go (2)
Context(16-27)GetContext(246-258)internal/core/execution/env.go (1)
EnvKeyDAGRunStepName(15-15)internal/core/dag.go (1)
DAG(33-135)
⏰ 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 (26)
internal/core/execution/context.go (3)
15-27: LGTM! Clean API rename with new charset field.The rename from
DAGContexttoContextsimplifies the naming, and the addition ofLogEncodingCharsetenables proper character encoding support for log files. The field documentation is clear and includes helpful examples.
141-194: LGTM! Excellent use of the functional options pattern.The builder pattern with functional options (
WithDatabase,WithSecrets,WithLogEncoding, etc.) is idiomatic Go and provides a clean, extensible API for optional parameters. This is a significant improvement over the previous approach.
196-237: LGTM! Well-structured constructor with clear option application.The
NewContextfunction cleanly applies options and constructs the context. The environment variable setup is properly maintained, and the newLogEncodingCharsetfield is correctly propagated from options.internal/runtime/builtin/archive/archive.go (1)
82-88: LGTM! Variable rename aligns with project conventions.The rename from
execCtxtorCtxis consistent with the broader refactoring pattern seen across the codebase (e.g.,rCtxfor runtime context).internal/runtime/builtin/mail/mail.go (1)
40-40: LGTM! API usage updated correctly.The change from
NewEnvForSteptoNewEnvaligns with the simplified API surface introduced in this refactor.internal/runtime/condition.go (1)
69-70: LGTM! Context retrieval updated with proper nil checks.The change from
GetDAGContexttoGetContextis consistent with the API refactor, and the nil check pattern (rCtx.DAG != nil) properly guards against empty contexts.internal/runtime/node.go (1)
639-640: LGTM! Consistent context access pattern.The update to
GetContextmaintains the same nil-check pattern used elsewhere in the codebase for accessingMaxOutputSizefrom the DAG configuration.internal/runtime/condition_test.go (1)
115-115: LGTM! Test updated to use the new API.The test correctly uses the simplified
NewEnvfunction, maintaining existing test coverage while adopting the new API surface.internal/runtime/output.go (2)
71-77: LGTM! Context access updated correctly.The change to
GetContextis consistent with the refactor. AccessingSecretEnvsis safe even if an empty context is returned, as iterating over a nil map is a no-op in Go.
133-134: LGTM! Proper nil checks for DAG configuration access.The context retrieval and nil check pattern correctly guards access to
MaxOutputSizefrom the DAG configuration.internal/runtime/runner.go (2)
115-120: LGTM! Context retrieval and precondition evaluation updated correctly.The change to
GetContextis consistent with the refactor. Direct access torCtx.DAGfields is safe here sinceNewContextalways sets theDAGfield during context construction.
1092-1092: LGTM! Environment construction updated to use new API.The change from
NewEnvForSteptoNewEnvaligns with the simplified API surface introduced in this refactor.internal/runtime/env_test.go (2)
116-116: LGTM! NewEnv API migration is correct.All calls to
NewEnvcorrectly use the new signatureNewEnv(ctx, core.Step{...})instead of the deprecatedNewEnvForStep. The migration is consistent throughout the test file.Also applies to: 257-257, 294-294, 333-333
353-370: LGTM! Proper use of the new functional-options pattern.The tests correctly demonstrate the new
NewContextAPI with functional options likeWithSecrets. This provides cleaner and more flexible context initialization compared to manual struct construction.Also applies to: 397-399
internal/runtime/executor/dag_runner.go (4)
49-49: LGTM! Context API migration is correct.The
SubDAGExecutorcorrectly migrates from the oldDAGContextto the newContexttype. ThedagCtxfield is properly initialized at lines 81 and 98, and all context access throughout the struct uses the updated API (GetContext,rCtx.DB,rCtx.CoordinatorCli, etc.).Also applies to: 60-100
104-154: LGTM! Command building correctly uses the new context API.The
buildCommandmethod properly retrieves the execution context viaGetContext(ctx)and usesrCtxto access DAG run information, root DAG run references, and environment variables. The trace context injection and command setup are correct.
156-192: LGTM! Coordinator task building correctly uses the new context API.The
BuildCoordinatorTaskmethod correctly retrieves the execution context and usesrCtxto accessRootDAGRun,DAG.Name, andDAGRunIDfor constructing the coordinator task. The task options are properly configured.
270-282: LGTM! Status retrieval correctly uses the new context API.The
ExecuteandwaitCompletionmethods correctly useGetContext(ctx)and access the database viarCtx.DBfor status queries. The polling logic for distributed execution completion is correct, and the cancellation wait loop properly accesses status viarCtx.DB.Also applies to: 336-441
internal/runtime/eval_test.go (2)
33-33: LGTM! NewEnv API migration is correct.All calls to
NewEnvcorrectly use the new signature throughout the test file. The migration fromNewEnvForStepis complete and consistent.Also applies to: 82-82, 174-174, 216-216, 269-269, 325-325, 551-551
484-484: LGTM! Proper use ofNewContextfor test setup.These tests correctly use
NewContextto initialize the execution context, ensuring all fields are properly set up. This is the recommended pattern for test context creation.Also applies to: 634-634
internal/runtime/env.go (6)
24-54: LGTM! Env struct correctly embeds the newexecution.Context.The struct definition properly embeds
execution.Contextinstead of the deprecatedexecution.DAGContext. The documentation accurately describes the fields and their purposes. The embedding provides access to DAGRunID, RootDAGRun, DAG, DB, Envs, SecretEnvs, CoordinatorCli, Shell, and the newLogEncodingCharsetfield.
56-67: LGTM! Environment methods correctly use the embedded Context.The
AllEnvs()andUserEnvsMap()methods correctly delegate toe.Context.AllEnvs()ande.Context.UserEnvsMap()respectively. The precedence logic and environment variable merging are preserved correctly after the refactoring.Also applies to: 69-102
104-132: LGTM! NewEnv constructor correctly uses the new context API.The renamed
NewEnvfunction (formerlyNewEnvForStep) correctly:
- Retrieves the execution context via
GetContext(ctx)- Passes
rCtxtoresolveWorkingDirfor working directory resolution- Initializes the
Contextfield withrCtx- Properly initializes all other fields (Variables, Envs, StepMap, WorkingDir)
The migration is complete and correct.
134-203: LGTM! resolveWorkingDir correctly uses the execution context parameter.The function signature update to accept
rCtx execution.Contextis correct, and all DAG access is properly routed throughrCtx.DAG. The working directory resolution logic (handling absolute paths, relative paths, environment variable expansion, and fallbacks) is preserved correctly after the refactoring.
286-322: LGTM! EvalString correctly uses GetContext for environment variable expansion.The method correctly retrieves the execution context via
GetContext(ctx)and accessesrCtx.SecretEnvsandrCtx.Envsfor variable expansion. The precedence order is properly maintained:
- Step-level environment variables (
e.Envs)- Output variables from previous steps (
e.Variables)- Secrets (
rCtx.SecretEnvs)- DAG-level environment variables (
rCtx.Envs)
370-377: LGTM! GetEnv default correctly uses NewEnv.The fallback path correctly creates a default
EnvusingNewEnv(ctx, core.Step{})instead of the deprecatedNewEnvForStep. This ensures proper initialization even when no Env is stored in the context.
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/runtime/debug_test.go (1)
12-13: Stale comment references old API.The comment still references
execution.SetupDAGContextbut the code now usesruntime.NewContext. Update the comment to reflect the current API.-// Note: This test uses execution.SetupDAGContext to set up the DAG context, -// then uses runtime.GetEnv/WithEnv to manage the runtime Env. +// Note: This test uses runtime.NewContext to set up the DAG context, +// then uses runtime.GetEnv/WithEnv to manage the runtime Env.
🧹 Nitpick comments (4)
internal/runtime/eval_test.go (1)
25-31: UsingNewEnvwithcontext.Backgroundis fine, but may log missing DAG context
TestEvalString/TestEvalBoolnow callruntime.NewEnv(context.Background(), ...)without a DAG context. This is functionally fine for pure variable substitution, butGetDAGContextwill log a missing DAG context andresolveWorkingDirwill fall back to the process working directory.If you want to avoid those log entries and keep tests closer to production wiring, consider using a lightweight DAG context:
-ctx := context.Background() -env := runtime.NewEnv(ctx, core.Step{Name: "test-step"}) +ctx := runtime.NewContext(context.Background(), &core.DAG{Name: "test-dag"}, "", "") +env := runtime.NewEnv(ctx, core.Step{Name: "test-step"})internal/runtime/env_test.go (2)
272-302: Basic-fields test correctly targetsNewEnvdespite legacy test name
TestNewEnvForStep_BasicFieldsnow usesruntime.NewEnv(ctx, step)with a DAG-attached context, and the assertions forStep,Variables,Envs,StepMap,DAG_RUN_STEP_NAME, andPWDmatch the new constructor’s behavior.You might consider renaming the test to drop the legacy
NewEnvForStep_prefix (e.g.TestNewEnv_BasicFields) to better reflect the current API, but this is purely cosmetic.
96-121: Parallel subtests capture loop variable; consider rebinding for Go < 1.22In
TestEnv_AllEnvsMap, each subtest callst.Parallel()inside afor _, tt := range testsloop. On Go versions prior to 1.22, this pattern can cause flakiness because all parallel subtests may capture and use the final value oftt.To be robust across Go versions, rebind
ttinside the loop:-for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { +for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { t.Parallel() // ... }) }The same pattern applies to other tests in this file that combine
for _, tt := range ...witht.Parallel()inside the subtest body.internal/runtime/env.go (1)
134-203: Working directory resolution is robust and respects DAG/env precedence
resolveWorkingDirnow:
- Expands
step.Dirtokens using DAG-leveldag.Envfirst, then falls back toos.Getenv.- Uses
fileutil.ResolvePathfor absolute/tilde paths.- Resolves relative paths against
dag.WorkingDirwhen set.- Falls back to
dag.WorkingDir, thenos.Getwd(), then user home directory with appropriate logging.The manual expansion loop over
dag.Envis correct, though you could simplify readability withstrings.HasPrefix(env, key+"="):- for _, env := range dag.Env { - if len(env) > len(key)+1 && env[:len(key)] == key && env[len(key)] == '=' { - return env[len(key)+1:] - } - } + for _, e := range dag.Env { + prefix := key + "=" + if strings.HasPrefix(e, prefix) { + return e[len(prefix):] + } + }Behavior stays the same; this just avoids manual indexing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
internal/runtime/agent/agent.go(3 hunks)internal/runtime/agent/dbclient.go(3 hunks)internal/runtime/builtin/command/command_test.go(3 hunks)internal/runtime/builtin/mail/mail_test.go(3 hunks)internal/runtime/condition.go(1 hunks)internal/runtime/context.go(1 hunks)internal/runtime/debug_test.go(1 hunks)internal/runtime/env.go(10 hunks)internal/runtime/env_test.go(12 hunks)internal/runtime/eval_test.go(9 hunks)internal/runtime/node.go(1 hunks)internal/runtime/node_test.go(9 hunks)internal/runtime/output.go(2 hunks)internal/runtime/output_test.go(5 hunks)internal/runtime/runner.go(2 hunks)internal/runtime/runner_helper_test.go(1 hunks)internal/runtime/runner_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/runtime/output.go
- internal/runtime/node.go
- internal/runtime/agent/agent.go
- internal/runtime/builtin/mail/mail_test.go
- internal/runtime/node_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_test.gointernal/runtime/runner.gointernal/runtime/runner_test.gointernal/runtime/condition.gointernal/runtime/runner_helper_test.gointernal/runtime/agent/dbclient.gointernal/runtime/builtin/command/command_test.gointernal/runtime/debug_test.gointernal/runtime/eval_test.gointernal/runtime/env.gointernal/runtime/context.gointernal/runtime/env_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/runtime/output_test.gointernal/runtime/runner_test.gointernal/runtime/runner_helper_test.gointernal/runtime/builtin/command/command_test.gointernal/runtime/debug_test.gointernal/runtime/eval_test.gointernal/runtime/env_test.go
🧠 Learnings (3)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/runtime/output_test.gointernal/runtime/builtin/command/command_test.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
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/runner_helper_test.gointernal/runtime/debug_test.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: `make test` (or `make test-coverage`) executes the Go suite via `gotestsum`; append `TEST_TARGET=./internal/...` to focus packages
Applied to files:
internal/runtime/builtin/command/command_test.go
🧬 Code graph analysis (11)
internal/runtime/output_test.go (2)
internal/runtime/context.go (1)
NewContext(28-28)internal/core/execution/context.go (1)
NewContext(199-237)
internal/runtime/runner.go (3)
internal/runtime/context.go (1)
GetDAGContext(45-47)internal/core/dag.go (1)
DAG(33-135)internal/runtime/env.go (1)
NewEnv(105-132)
internal/runtime/condition.go (2)
internal/runtime/context.go (1)
GetDAGContext(45-47)internal/core/dag.go (1)
DAG(33-135)
internal/runtime/runner_helper_test.go (2)
internal/runtime/context.go (2)
NewContext(28-28)Context(14-14)internal/core/execution/context.go (2)
NewContext(199-237)Context(16-27)
internal/runtime/agent/dbclient.go (4)
internal/runtime/context.go (3)
Database(16-16)Context(14-14)RunStatus(20-20)internal/core/execution/context.go (3)
Database(90-99)Context(16-27)RunStatus(102-113)internal/core/execution/dagrun.go (1)
DAGRunRef(144-147)api/v1/api.gen.go (1)
RunStatus(496-496)
internal/runtime/builtin/command/command_test.go (3)
internal/runtime/context.go (1)
NewContext(28-28)internal/core/execution/context.go (1)
NewContext(199-237)internal/runtime/env.go (1)
NewEnv(105-132)
internal/runtime/debug_test.go (2)
internal/runtime/context.go (1)
NewContext(28-28)internal/core/execution/context.go (1)
NewContext(199-237)
internal/runtime/eval_test.go (3)
internal/runtime/context.go (1)
NewContext(28-28)internal/core/execution/context.go (1)
NewContext(199-237)internal/runtime/env.go (1)
NewEnv(105-132)
internal/runtime/env.go (4)
internal/runtime/context.go (2)
Context(14-14)GetDAGContext(45-47)internal/core/execution/context.go (1)
Context(16-27)internal/core/execution/env.go (1)
EnvKeyDAGRunStepName(15-15)internal/core/dag.go (1)
DAG(33-135)
internal/runtime/context.go (2)
internal/core/execution/context.go (2)
GetContext(246-258)WithContext(241-243)internal/core/execution/dagrun.go (1)
DAGRunRef(144-147)
internal/runtime/env_test.go (2)
internal/runtime/context.go (4)
NewContext(28-28)Context(14-14)WithDAGContext(51-53)WithSecrets(38-38)internal/runtime/env.go (2)
NewEnv(105-132)Env(24-54)
⏰ 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 (29)
internal/runtime/runner_helper_test.go (1)
203-203: Migration to runtime.NewContext is correct and complete.The context construction has been successfully migrated to use
runtime.NewContextwith all required parameters. Not passing encoding options is the intended pattern throughout the test suite—tests use the default empty encoding unless explicitly configured viaWithLogEncoding, which is appropriate for general runner tests.internal/runtime/condition.go (1)
68-70: LGTM!The variable rename from
envtorCtximproves clarity since this is a runtime context, not an environment. The change correctly uses the newGetDAGContextwrapper from the runtime package.internal/runtime/debug_test.go (1)
17-17: LGTM!The migration from
execution.SetupDAGContexttoruntime.NewContextis correct and consistent with the broader refactoring pattern.internal/runtime/output_test.go (1)
66-66: LGTM!Consistent migration to
NewContextAPI across all test cases. The test logic remains unchanged.internal/runtime/builtin/command/command_test.go (2)
360-364: LGTM!Clean migration to
runtime.NewContextandruntime.NewEnv. The test setup pattern is consistent with other tests in the codebase.
544-548: LGTM!The
setupTestContexthelper function is properly updated to use the new API, ensuring all tests using this helper benefit from the consistent refactoring.internal/runtime/runner.go (1)
1092-1092: LGTM!The rename from
NewEnvForSteptoNewEnvis consistent with the broader API simplification in this refactor.internal/runtime/runner_test.go (3)
1321-1321: LGTM!Correct migration to
runtime.NewContextfor the setup error test case.
1397-1397: LGTM!Consistent use of
runtime.NewContextfor DAG precondition testing.
2802-2802: LGTM!Proper usage of
runtime.NewContextin the deadlock detection test.internal/runtime/agent/dbclient.go (3)
10-13: LGTM!Using
runtime.Databaseandruntime.RunStatustype aliases maintains consistency within the runtime package hierarchy. Since these are type aliases to the execution package types, there's no functional change.
29-29: LGTM!Return type updated to use the runtime package's type alias consistently.
53-59: LGTM!Struct instantiation correctly uses
runtime.RunStatusto match the updated return type signature.internal/runtime/eval_test.go (3)
13-21: Context setup for tests aligns with new execution.Context APIUsing
execution.NewContextinsetupTestContextcorrectly seeds a DAG-awarecontext.Contextfor object evaluation tests, and it matches theruntime.NewEnvexpectations (DAG metadata available when needed, but not over-specified). No behavioral issues spotted.
474-540: Good coverage of edge cases with newruntime.NewContextusage
TestEvalStringEdgeCasesnow exercisesEvalStringagainst aruntime.NewContext-backed DAG context and derives the initialEnvviaGetEnv(ctx)→NewEnv(ctx, core.Step{}). This correctly drives evaluation through the new context/Env stack and keeps the test aligned with runtime behavior.
624-719: Bool edge-case tests correctly matchEvalBool+strconv.ParseBoolsemantics
TestEvalBoolEdgeCases’ use ofruntime.NewContextandGetEnvmirrors the new runtime wiring, and expectations (yes/no/on/offerroring whilet/fparse successfully) are consistent withEvalBool’sstrconv.ParseBool-based implementation.internal/runtime/env_test.go (5)
100-121: Newexecution.NewContext+runtime.NewEnvwiring for AllEnvs tests looks correctCreating a
core.DAGwithWorkingDirand then calling:ctx := execution.NewContext(context.Background(), dag, "", "") env := runtime.NewEnv(ctx, core.Step{Name: "test-step"})ensures that:
Env.WorkingDiris resolved from the DAG,Env.EnvsincludesDAG_RUN_STEP_NAMEandPWD,AllEnvsMapsees both DAG-level and step-level variables.The precedence behavior asserted in
TestEnv_AllEnvsMapremains consistent with the runtime’sAllEnvs()implementation.
241-263: Context construction for working-dir tests matches runtime behaviorIn
TestNewEnvForStep_WorkingDirectory, constructing:dagCtx := execution.Context{DAG: dag} ctx := runtime.WithDAGContext(context.Background(), dagCtx) env := runtime.NewEnv(ctx, tt.step)is a minimal but sufficient DAG context for
resolveWorkingDirandNewEnv. It correctly drives the various absolute/relative/tilde/parent-directory cases under test.
311-333: DAG env expansion in working directory tests aligns with newresolveWorkingDirlogic
TestNewEnvForStep_WorkingDirectory_DAGEnvExpansionnow builds a DAG context and callsruntime.NewEnv(ctx, step)wherestep.Dirreferences$MY_SUBDIR. This correctly exercisesresolveWorkingDir’s new manual expansion ofdag.Envwithout relying onos.Setenv, and the symlink-normalized comparison is appropriate.
343-446: UserEnvsMap tests correctly validate precedence and secret handling under new Context APIThe
TestEnv_UserEnvsMaptable now:
- Creates DAG-aware contexts via
runtime.NewContextwith optionalruntime.WithSecrets.- Uses
runtime.NewEnv(ctx, step)to derive step environments.- Drives precedence scenarios where step envs/output vars override DAG envs and secrets, and verifies OS env is excluded.
This matches
Env.UserEnvsMap()’s documented precedence and confirms that swapping toe.Context.UserEnvsMap()has not regressed behavior.
451-555: EvalString precedence tests are well-aligned with the new EvalString implementation
TestEnv_EvalString_Precedencenow usesruntime.NewContext+runtime.NewEnvfor all cases. The scenarios where step envs override output vars and DAG envs, and output vars override DAG envs, match the documented precedence and the order in whichEnv.EvalStringcomposescmdutil.WithVariables(...)options.internal/runtime/env.go (6)
24-54: EmbeddingContextinto Env is a clean alignment with execution.ContextSwitching Env to embed
Context(the runtime alias forexecution.Context) preserves all existing field usages (e.DAG,e.DAGRunID, etc.) while simplifying access to execution metadata. This lines up well with the newexecution.ContextAPI and keeps callers unchanged.
56-67: AllEnvs now correctly builds on execution.Context.AllEnvsStarting from
e.Context.AllEnvs()and then layering step-specificEnvsand outputVariableskeeps OS env + DAG-level env + secrets behavior in one place (execution.Context) and adds step concerns on top. This matches how the tests assert AllEnvsMap should behave.
69-102: UserEnvsMap composition matches documented precedence and tests
UserEnvsMapnow:
- Seeds from
e.Context.UserEnvsMap()(DAG + secrets, no OS env),- Overlays previous-step output variables from
Variables,- Overlays step
Envs(e.g.PWD,DAG_RUN_STEP_NAME),- Finally fills gaps from
Step.Envwhere no value exists yet.This ordering is consistent with the precedence scenarios tested in
TestEnv_UserEnvsMapandTestEnv_EvalString_Precedence.
104-132: NewEnv correctly derives Env from the DAG context and step
NewEnv(ctx, step):
- Retrieves the execution context via
GetDAGContext(ctx),- Resolves
WorkingDirwithresolveWorkingDir,- Seeds
EnvswithDAG_RUN_STEP_NAMEandPWD,- Initializes a fresh
Variablesmap and backfills it fromrCtx.DAG.Paramswhen present.The constructed Env matches expectations in env/eval tests and keeps behavior coherent even when
ctxhas no DAG (falling back to process/home working dirs).
286-317: EvalString correctly sources secrets and DAG envs from the new ContextUsing
rCtx := GetDAGContext(ctx)and, whenoption.ExpandEnvis true, appending variables from:
e.Envse.Variables.Variables()rCtx.SecretEnvsrCtx.Envsmatches the documented precedence and the expectations in the
EvalStringprecedence tests. The non-ExpandEnv path correctly limits evaluation to step envs and output variables.
370-375: GetEnv fallback to NewEnv(ctx, core.Step{}) is a reasonable defaultWhen no Env is stored in the context,
GetEnvnow constructs one viaNewEnv(ctx, core.Step{}). This allows callers likeruntime.GetEnv(ctx)in tests to obtain a minimal but consistent Env without panicking, while still leveraging any DAG context present inctx.internal/runtime/context.go (2)
10-41: Runtime facade over execution types and options is well-structuredThe type aliases and re-exported variables (
NewContext,WithDatabase,WithRootDAGRun,WithParams,WithCoordinator,WithSecrets,WithLogEncoding) cleanly expose execution-layer primitives throughinternal/runtimewithout introducing extra behavior. This matches how the rest of the runtime now constructs contexts.
43-64: Context helpers correctly delegate to execution and avoid duplication
GetDAGContext,WithDAGContext,NewDAGRunRef, andNewContextForTestare thin wrappers over the execution package. Their signatures line up with the underlying functions, and they centralize DAG-context access for the runtime package and its tests.
| rCtx := GetDAGContext(ctx) | ||
| var shell []string | ||
| if env.DAG.Shell != "" { | ||
| shell = append([]string{env.DAG.Shell}, env.DAG.ShellArgs...) | ||
| if rCtx.DAG.Shell != "" { | ||
| shell = append([]string{rCtx.DAG.Shell}, rCtx.DAG.ShellArgs...) | ||
| } | ||
| if err := EvalConditions(ctx, shell, env.DAG.Preconditions); err != nil { | ||
| if err := EvalConditions(ctx, shell, rCtx.DAG.Preconditions); err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the GetDAGContext implementation
rg -n "func.*GetDAGContext" internal/runtime/Repository: dagu-org/dagu
Length of output: 137
🏁 Script executed:
# Check the Context struct definition to see if DAG field is always initialized
rg -n "type Context struct" internal/ -A 20Repository: dagu-org/dagu
Length of output: 2801
🏁 Script executed:
# Look for nil checks on DAG field in the runtime package
rg -n "\.DAG\s*(==|!=)\s*nil" internal/runtime/ -B2 -A2Repository: dagu-org/dagu
Length of output: 1923
🏁 Script executed:
# Check lines 115-120 and surrounding context in runner.go
sed -n '110,125p' internal/runtime/runner.goRepository: dagu-org/dagu
Length of output: 547
🏁 Script executed:
# Search for how GetDAGContext is typically used - are there patterns of nil checks?
rg -n "GetDAGContext" internal/runtime/ -A3 -B1Repository: dagu-org/dagu
Length of output: 2770
Add nil check before accessing rCtx.DAG on lines 117 and 120.
Lines 117-120 access rCtx.DAG.Shell, rCtx.DAG.ShellArgs, and rCtx.DAG.Preconditions without checking if rCtx.DAG is nil. Throughout the codebase (e.g., output.go:132, node.go:639, condition.go:68, env.go:115), similar accesses are consistently guarded with nil checks. Add a nil check here to prevent a potential panic.
🤖 Prompt for AI Agents
In internal/runtime/runner.go around lines 115 to 120, rCtx.DAG is accessed
without a nil check which can panic; add a guard: check if rCtx.DAG != nil
before reading rCtx.DAG.Shell, rCtx.DAG.ShellArgs, and rCtx.DAG.Preconditions,
build the shell slice and call EvalConditions only when DAG is non-nil
(otherwise leave shell/preconditions nil or skip the EvalConditions call) so the
function safely handles contexts with no DAG.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/runtime/eval_test.go (4)
75-76: Use proper context initialization for consistency.This test uses bare
context.Background()while other tests usesetupTestContext()orruntime.NewContext(). Sinceruntime.NewEnvcallsGetDAGContext(ctx)internally (per the relevant code snippets), the context should be properly initialized with DAG metadata.Apply this diff to align with other tests:
- ctx := context.Background() - env := runtime.NewEnv(ctx, core.Step{Name: "test-step"}) + ctx := setupTestContext() + env := runtime.NewEnv(ctx, core.Step{Name: "test-step"})
262-263: Use proper context initialization for consistency.Similar to
TestEvalBool, this test uses barecontext.Background()instead of initializing the context properly withsetupTestContext()orruntime.NewContext(). This is inconsistent with the pattern used in other tests.Apply this diff:
- ctx := context.Background() - env := runtime.NewEnv(ctx, core.Step{Name: "test-step"}) + ctx := setupTestContext() + env := runtime.NewEnv(ctx, core.Step{Name: "test-step"})
478-483: Inconsistent environment creation pattern.This test uses
runtime.GetEnv(ctx)to retrieve an environment, while all other tests in this file useruntime.NewEnv(ctx, step)to create one. This pattern inconsistency makes the code harder to maintain and reason about.Apply this diff to align with other tests:
ctx := runtime.NewContext(context.Background(), &core.DAG{}, "test-run", "test.log") - env := runtime.GetEnv(ctx) + env := runtime.NewEnv(ctx, core.Step{Name: "test-step"}) env.Variables.Store("EMPTY", "EMPTY=") env.Variables.Store("SPACES", "SPACES= ") env.Variables.Store("SPECIAL", "SPECIAL=special!@#")
628-638: Inconsistent environment creation pattern.Similar to
TestEvalStringEdgeCases, this test usesruntime.GetEnv(ctx)instead ofruntime.NewEnv(ctx, step). This pattern is inconsistent with all other tests in the file.Apply this diff:
ctx := runtime.NewContext(context.Background(), &core.DAG{}, "test-run", "test.log") - env := runtime.GetEnv(ctx) + env := runtime.NewEnv(ctx, core.Step{Name: "test-step"}) env.Variables.Store("YES", "YES=yes") env.Variables.Store("NO", "NO=no") env.Variables.Store("ON", "ON=on") env.Variables.Store("OFF", "OFF=off") env.Variables.Store("T", "T=t") env.Variables.Store("F", "F=f") - - ctx = runtime.WithEnv(ctx, env) + ctx = runtime.WithEnv(ctx, env)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/common/masking/writer_test.go(0 hunks)internal/runtime/builtin/command/command_test.go(3 hunks)internal/runtime/eval_test.go(9 hunks)
💤 Files with no reviewable changes (1)
- internal/common/masking/writer_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/runtime/builtin/command/command_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/eval_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/runtime/eval_test.go
🧬 Code graph analysis (1)
internal/runtime/eval_test.go (3)
internal/runtime/context.go (1)
NewContext(28-28)internal/core/execution/context.go (1)
NewContext(199-237)internal/runtime/env.go (1)
NewEnv(105-132)
⏰ 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 (6)
internal/runtime/eval_test.go (6)
13-20: LGTM: Context setup updated correctly.The helper now uses the new
execution.NewContextAPI as expected.
26-27: LGTM: Context and environment initialization updated correctly.The test now uses the new
runtime.NewContextandruntime.NewEnvAPIs as intended.
168-168: LGTM: Environment creation updated correctly.The test now uses
runtime.NewEnvas intended.
210-210: LGTM: Environment creation updated correctly.
319-319: LGTM: Environment creation updated correctly.
545-545: LGTM: Environment creation updated correctly.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1449 +/- ##
==========================================
+ Coverage 59.77% 59.83% +0.06%
==========================================
Files 187 188 +1
Lines 21095 21150 +55
==========================================
+ Hits 12610 12656 +46
- Misses 7171 7178 +7
- Partials 1314 1316 +2
... and 2 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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.