refactor(core/spec): restructure spec types and build logic#1499
refactor(core/spec): restructure spec types and build logic#1499
Conversation
WalkthroughReplaces the old YAML definition + builder registry with a manifest-based spec and typed YAML union types; removes many legacy builders/helpers/tests; adds typed parsing types, per-manifest build methods, a decode hook flow, and relocates DAG name validation to validator.go. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as LoadYAML (public API)
participant Decoder as TypedUnionDecodeHook / decodeViaYAML
participant Manifest as dag (manifest)
participant Builder as dag.build()
participant Core as core.DAG / core.Step
Client->>Decoder: Read & decode YAML (single or multi-doc)
Decoder->>Manifest: Unmarshal into typed unions (ShellValue, EnvValue, ScheduleValue...)
Client->>Manifest: supply base manifest (optional)
Manifest->>Builder: .build(ctx)
Builder->>Builder: run metadata transformers
Builder->>Builder: run full transformers (env, schedule, shell, secrets, steps...)
Builder->>Core: construct core.DAG and core.Step objects
Builder-->>Client: return core.DAG or aggregated BuildErrors
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
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: 1
🧹 Nitpick comments (12)
internal/integration/gha_test.go (1)
12-14: Consider a more descriptive skip message.The current skip message mentions "flaky test locally," but a more specific message could better explain the requirement:
"Skipping: GitHub Actions executor requires GitHub Actions environment".🔎 Suggested improvement
if os.Getenv("GITHUB_ACTIONS") != "true" { - t.Skip("Skipping flaky test locally") + t.Skip("Skipping: GitHub Actions executor requires GitHub Actions environment") }internal/core/spec/builder_test.go (1)
4368-4425: LGTM! Good test coverage for timeout handling.The test suite covers the key scenarios:
- Positive timeout correctly converts to duration
- Explicit zero and omitted timeout both yield zero duration
- Negative timeout properly triggers validation error
The test structure aligns with existing patterns in this file. Consider adding
t.Parallel()to each subtest for consistency with tests likeTestBuildStepthat parallelize subtests, though this is optional given the existing mixed usage in the file.internal/core/spec/variables.go (1)
93-128: The function is correct but duplicates core logic fromloadVariables.The evaluation/setenv loop (lines 103-125) is nearly identical to the loop in
loadVariables(lines 66-88). Consider extracting the common logic into a shared helper to reduce duplication:🔎 Potential refactor to reduce duplication
// processEnvPairs handles the common evaluation and setenv logic func processEnvPairs(ctx BuildContext, pairs []pair) (map[string]string, error) { vars := map[string]string{} for _, p := range pairs { value := p.val if !ctx.opts.Has(BuildFlagNoEval) { var err error value, err = cmdutil.EvalString(ctx.ctx, value, cmdutil.WithVariables(vars)) if err != nil { return nil, core.NewValidationError("env", p.val, fmt.Errorf("%w: %s", ErrInvalidEnvValue, p.val)) } if err := os.Setenv(p.key, value); err != nil { return nil, core.NewValidationError("env", p.key, fmt.Errorf("%w: %s", err, p.key)) } } vars[p.key] = value } return vars, nil }Then
loadVariablesFromEnvValuecould convert entries to pairs and call this helper.internal/core/spec/types/port_test.go (1)
1-117: LGTM! Comprehensive test coverage for PortValue unmarshaling.The tests cover key scenarios:
- Integer and string representations
- Edge cases (large port 65535, zero/unset value)
- Error paths for invalid types (array, map)
- Integration with struct unmarshaling
Consider adding tests for boundary cases if port validation is intended at this layer:
- Port 0 (valid for dynamic port assignment)
- Port > 65535 (invalid but may be caught elsewhere)
This is optional since validation might be handled at a different layer.
internal/core/spec/types/shell.go (1)
44-60: Consider validating array element types.The implementation uses
fmt.Sprintf("%v", ...)as a fallback when array elements are not strings (lines 50, 56). While this provides compatibility, it silently accepts invalid inputs like numbers or bools in shell command arrays, which could lead to unexpected behavior.Consider either:
- Returning an error for non-string array elements
- Adding a validation layer to flag this as a warning
🔎 Alternative implementation with strict type checking
case []any: // Array value: ["bash", "-e"] if len(v) > 0 { if cmd, ok := v[0].(string); ok { s.command = cmd } else { - s.command = fmt.Sprintf("%v", v[0]) + return fmt.Errorf("shell command must be string, got %T", v[0]) } for i := 1; i < len(v); i++ { if arg, ok := v[i].(string); ok { s.arguments = append(s.arguments, arg) } else { - s.arguments = append(s.arguments, fmt.Sprintf("%v", v[i])) + return fmt.Errorf("shell argument at index %d must be string, got %T", i, v[i]) } } }internal/core/spec/loader.go (1)
554-584: Consider a registry-based approach for type dispatch.The current implementation is clear and explicit, but adding new typed union types requires modifying this function. A map-based registry could make it more extensible.
That said, the current approach has the benefit of compile-time type safety and explicit documentation of supported types.
🔎 Optional: Registry-based alternative
var typedUnionTypes = map[reflect.Type]bool{ reflect.TypeOf(types.ShellValue{}): true, reflect.TypeOf(types.StringOrArray{}): true, reflect.TypeOf(types.ScheduleValue{}): true, reflect.TypeOf(types.EnvValue{}): true, reflect.TypeOf(types.ContinueOnValue{}): true, reflect.TypeOf(types.PortValue{}): true, } func TypedUnionDecodeHook() mapstructure.DecodeHookFunc { return func(_ reflect.Type, to reflect.Type, data any) (any, error) { if typedUnionTypes[to] { return decodeViaYAMLReflect(to, data) } return data, nil } }internal/core/spec/dag.go (1)
662-780: Consider extracting common auth config parsing logic.The
map[string]any(lines 680-713) andmap[any]any(lines 715-773) cases have nearly identical nested logic for parsing auth configuration. Extracting a helper function would reduce duplication and improve maintainability.🔎 Suggested refactor to reduce duplication
+// parseAuthConfig extracts auth configuration from the given map. +func parseAuthConfig(auth map[string]any, noEval bool) *core.AuthConfig { + config := &core.AuthConfig{} + if username, ok := auth["username"].(string); ok { + config.Username = username + if !noEval { + config.Username = os.ExpandEnv(config.Username) + } + } + if password, ok := auth["password"].(string); ok { + config.Password = password + if !noEval { + config.Password = os.ExpandEnv(config.Password) + } + } + if authStr, ok := auth["auth"].(string); ok { + config.Auth = authStr + if !noEval { + config.Auth = os.ExpandEnv(config.Auth) + } + } + return config +}Then use it in both
map[string]anyandmap[any]anycases by converting the map appropriately.internal/core/spec/step.go (4)
162-172: Consider adding a deprecation warning forDirfield.Unlike the
Runfield (which logs a deprecation warning inbuildSubDAG), the deprecatedDirfield is silently accepted. For consistency, consider logging a warning whenDiris used.🔎 Proposed fix
func (s *step) buildWorkingDir(_ StepBuildContext, result *core.Step) error { switch { case s.WorkingDir != "": result.Dir = strings.TrimSpace(s.WorkingDir) case s.Dir != "": + // TODO: Log deprecation warning similar to Run field result.Dir = strings.TrimSpace(s.Dir) default: result.Dir = "" } return nil }
373-395: Consider extracting common backoff parsing logic.The backoff parsing and validation logic (lines 373-395) is nearly identical to the one in
buildRetryPolicy(lines 276-297). Consider extracting this into a helper function to reduce duplication.
506-518: Minor inconsistency in CmdWithArgs construction.In the array case,
CmdWithArgs(line 517) is constructed by quoting each arg, but thecmditself is not quoted. Ifcmdcontains spaces (unlikely but possible), this could produce an inconsistent representation. The string case (line 480) uses the original value directly.This is minor since
CmdWithArgsappears to be primarily for display/logging purposes.
630-662: Unknown keys in parallel configuration are silently ignored.When
parallelis an object, onlyitemsandmaxConcurrentare handled. Other keys (e.g., typos likemaxConcurent) are silently ignored, which could lead to user confusion.🔎 Proposed fix to warn on unknown keys
case map[string]any: // Object configuration for key, val := range v { switch key { case "items": // ... existing code ... case "maxConcurrent": // ... existing code ... + + default: + return core.NewValidationError("parallel", key, fmt.Errorf("unknown key %q in parallel configuration", key)) } }internal/core/spec/builder.go (1)
220-234: Unused parameterctx.The
ctx BuildContextparameter is declared but not used in the function body.🔎 Proposed fix
-func normalizeStepData(ctx BuildContext, data []any) []any { +func normalizeStepData(_ BuildContext, data []any) []any {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
internal/core/names.go(0 hunks)internal/core/spec/builder.go(4 hunks)internal/core/spec/builder_test.go(1 hunks)internal/core/spec/builder_timeout_test.go(0 hunks)internal/core/spec/command.go(0 hunks)internal/core/spec/dag.go(1 hunks)internal/core/spec/definition.go(0 hunks)internal/core/spec/loader.go(10 hunks)internal/core/spec/params.go(0 hunks)internal/core/spec/schedule.go(0 hunks)internal/core/spec/step.go(1 hunks)internal/core/spec/types/continueon.go(1 hunks)internal/core/spec/types/continueon_test.go(1 hunks)internal/core/spec/types/doc.go(1 hunks)internal/core/spec/types/env.go(1 hunks)internal/core/spec/types/env_test.go(1 hunks)internal/core/spec/types/port.go(1 hunks)internal/core/spec/types/port_test.go(1 hunks)internal/core/spec/types/schedule.go(1 hunks)internal/core/spec/types/schedule_test.go(1 hunks)internal/core/spec/types/shell.go(1 hunks)internal/core/spec/types/shell_test.go(1 hunks)internal/core/spec/types/stringarray.go(1 hunks)internal/core/spec/types/stringarray_test.go(1 hunks)internal/core/spec/variables.go(2 hunks)internal/core/validator.go(1 hunks)internal/integration/gha_test.go(1 hunks)
💤 Files with no reviewable changes (6)
- internal/core/spec/builder_timeout_test.go
- internal/core/spec/schedule.go
- internal/core/spec/command.go
- internal/core/names.go
- internal/core/spec/definition.go
- internal/core/spec/params.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/core/spec/types/stringarray_test.gointernal/core/spec/types/port_test.gointernal/core/spec/types/shell.gointernal/core/spec/types/continueon_test.gointernal/core/spec/types/schedule_test.gointernal/core/spec/builder_test.gointernal/core/spec/types/env.gointernal/core/spec/types/doc.gointernal/integration/gha_test.gointernal/core/validator.gointernal/core/spec/types/shell_test.gointernal/core/spec/variables.gointernal/core/spec/dag.gointernal/core/spec/types/schedule.gointernal/core/spec/types/stringarray.gointernal/core/spec/types/port.gointernal/core/spec/types/continueon.gointernal/core/spec/loader.gointernal/core/spec/step.gointernal/core/spec/types/env_test.gointernal/core/spec/builder.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/core/spec/types/stringarray_test.gointernal/core/spec/types/port_test.gointernal/core/spec/types/continueon_test.gointernal/core/spec/types/schedule_test.gointernal/core/spec/builder_test.gointernal/integration/gha_test.gointernal/core/spec/types/shell_test.gointernal/core/spec/types/env_test.go
🧠 Learnings (4)
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/core/spec/types/stringarray_test.gointernal/core/spec/types/port_test.gointernal/core/spec/types/continueon_test.gointernal/core/spec/types/schedule_test.gointernal/integration/gha_test.gointernal/core/spec/types/shell_test.gointernal/core/spec/types/env_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/core/spec/types/continueon_test.gointernal/integration/gha_test.gointernal/core/spec/types/shell_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/core/spec/types/shell_test.go
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{yaml,yml} : Dagu YAML configurations should support minimal setup with sensible defaults and support for environment variable expansion and command substitution
Applied to files:
internal/core/spec/types/env.go
🧬 Code graph analysis (11)
internal/core/spec/types/stringarray_test.go (1)
internal/core/spec/types/stringarray.go (3)
StringOrArray(18-22)MailToValue(83-83)TagsValue(90-90)
internal/core/spec/types/port_test.go (1)
internal/core/spec/types/port.go (1)
PortValue(16-20)
internal/core/spec/types/continueon_test.go (2)
internal/core/spec/types/continueon.go (1)
ContinueOnValue(25-33)internal/core/status.go (1)
Failed(9-9)
internal/core/spec/types/schedule_test.go (1)
internal/core/spec/types/schedule.go (1)
ScheduleValue(22-28)
internal/core/spec/builder_test.go (1)
internal/core/spec/loader.go (1)
LoadYAML(174-188)
internal/core/spec/types/shell_test.go (2)
internal/core/spec/types/shell.go (1)
ShellValue(21-26)internal/runtime/builtin/command/shell.go (1)
Shell(13-20)
internal/core/spec/dag.go (11)
internal/core/spec/types/shell.go (1)
ShellValue(21-26)internal/core/spec/types/stringarray.go (1)
StringOrArray(18-22)internal/core/spec/types/schedule.go (1)
ScheduleValue(22-28)internal/core/spec/types/env.go (1)
EnvValue(29-33)internal/core/dag.go (12)
MailOn(485-488)TypeChain(27-27)TypeGraph(25-25)TypeAgent(29-29)AuthConfig(392-400)HandlerOnInit(520-520)HandlerOnExit(524-524)HandlerOnSuccess(521-521)HandlerOnFailure(522-522)HandlerOnCancel(523-523)OTelConfig(507-514)MailConfig(499-504)internal/core/spec/types/port.go (1)
PortValue(16-20)internal/core/spec/builder.go (5)
BuildContext(13-22)BuildFlagOnlyMetadata(49-49)BuildFlagAllowBuildErrors(50-50)BuildFlagSkipSchemaValidation(51-51)BuildFlagNoEval(48-48)internal/common/cmdutil/cmdutil.go (2)
GetShellCommand(35-61)SplitCommand(142-169)internal/common/fileutil/fileutil.go (1)
ResolvePathOrBlank(187-194)internal/core/spec/errors.go (2)
ErrInvalidStepData(14-14)ErrStepsMustBeArrayOrMap(15-15)internal/core/spec/loader.go (1)
TypedUnionDecodeHook(556-584)
internal/core/spec/loader.go (8)
internal/core/spec/builder.go (1)
BuildContext(13-22)internal/core/dag.go (1)
DAG(33-135)internal/core/spec/types/shell.go (1)
ShellValue(21-26)internal/core/spec/types/stringarray.go (1)
StringOrArray(18-22)internal/core/spec/types/schedule.go (1)
ScheduleValue(22-28)internal/core/spec/types/env.go (1)
EnvValue(29-33)internal/core/spec/types/continueon.go (1)
ContinueOnValue(25-33)internal/core/spec/types/port.go (1)
PortValue(16-20)
internal/core/spec/step.go (8)
internal/core/spec/types/shell.go (1)
ShellValue(21-26)internal/core/spec/types/stringarray.go (1)
StringOrArray(18-22)internal/core/step.go (2)
ContinueOn(226-232)ExecutorConfig(107-115)internal/core/params.go (2)
Params(36-45)NewSimpleParams(48-50)internal/core/spec/types/env.go (1)
EnvValue(29-33)internal/core/spec/builder.go (2)
StepBuildContext(25-28)BuildContext(13-22)internal/common/cmdutil/cmdutil.go (2)
SplitCommand(142-169)JoinCommandArgs(20-22)internal/core/spec/errors.go (8)
ErrTimeoutSecMustBeNonNegative(31-31)ErrInvalidSignal(21-21)ErrStepCommandIsEmpty(29-29)ErrStepCommandMustBeArrayOrString(30-30)ErrExecutorTypeMustBeString(23-23)ErrExecutorConfigValueMustBeMap(24-24)ErrExecutorHasInvalidKey(25-25)ErrExecutorConfigMustBeStringOrMap(26-26)
internal/core/spec/types/env_test.go (1)
internal/core/spec/types/env.go (1)
EnvValue(29-33)
internal/core/spec/builder.go (2)
internal/core/dag.go (1)
SecretRef(139-148)internal/core/spec/loader.go (1)
TypedUnionDecodeHook(556-584)
⏰ 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 (65)
internal/integration/gha_test.go (1)
4-4: LGTM! Appropriate environment guard for CI-specific test.The addition of the environment check correctly ensures this GitHub Actions-specific test only runs in the appropriate CI environment. The placement before
t.Parallel()follows Go testing best practices.Also applies to: 12-14
internal/core/validator.go (1)
9-29: LGTM!The DAG name validation logic is well-structured:
- Regex compiled once at package init is idiomatic Go
- Empty names correctly allowed (caller provides via context/filename)
- Length and character validation are properly sequenced
- The regex pattern correctly enforces alphanumeric, underscore, dash, and dot characters
internal/core/spec/types/doc.go (1)
1-15: LGTM!Clear and well-structured package documentation that explains the purpose and design principles of the typed union types. The four design principles provide good guidance for implementing and using types in this package.
internal/core/spec/types/schedule_test.go (2)
12-119: LGTM! Comprehensive test coverage for ScheduleValue.The test suite thoroughly exercises all YAML unmarshalling scenarios including simple strings, arrays, maps with various keys, error conditions, and zero-value behavior. The tests properly validate accessor methods and error messages.
121-160: Good integration testing within structs.The nested struct tests verify that ScheduleValue integrates correctly with YAML unmarshalling when used as a struct field, covering both simple and complex schedule forms.
internal/core/spec/types/continueon_test.go (2)
12-166: LGTM! Thorough test coverage for ContinueOnValue.The test suite comprehensively covers all unmarshalling scenarios including string forms (with case insensitivity and whitespace handling), map forms with various fields, error conditions, and zero-value behavior. All accessor methods are properly validated.
168-206: Good struct integration tests.The tests verify proper integration of ContinueOnValue within structs, covering both string and map forms, as well as the not-set scenario.
internal/core/spec/types/stringarray_test.go (3)
12-73: LGTM! Comprehensive StringOrArray unmarshalling tests.The test suite covers all key scenarios including single strings, arrays, empty cases, and proper distinction between IsZero (not set) and IsEmpty (set but empty). The preservation of empty strings for validation layer handling is intentional and well-documented.
75-124: Good struct integration and edge case coverage.The tests properly verify StringOrArray behavior within structs and correctly distinguish between not-set vs. explicitly-set-to-empty-array scenarios, which is important for dependency handling.
126-158: Sufficient coverage for type aliases.The minimal tests for MailToValue and TagsValue are appropriate since they are type aliases and inherit all behavior from StringOrArray.
internal/core/spec/types/shell_test.go (2)
12-90: LGTM! Thorough ShellValue unmarshalling tests.The test suite comprehensively covers all unmarshalling scenarios including string forms (with and without inline args), array forms, zero-value behavior, and IsArray distinction. The preservation of environment variable syntax is correctly tested.
92-118: Good struct integration tests.The tests properly verify ShellValue behavior when embedded in structs, covering both set and not-set scenarios.
internal/core/spec/types/shell.go (1)
1-99: Implementation is functionally correct otherwise.The ShellValue implementation properly handles all expected YAML input forms (string, array, nil) and provides appropriate accessors. The preservation of environment variable syntax and the IsArray flag are well-implemented.
internal/core/spec/types/env_test.go (2)
12-166: LGTM! Comprehensive EnvValue unmarshalling tests.The test suite thoroughly covers all unmarshalling scenarios including map forms, array forms (with ordering preserved), string forms with KEY=value syntax, mixed forms, numeric value stringification, and values containing equals signs. The preservation of environment variable references for later expansion is correctly tested.
168-212: Good struct integration tests.The tests properly verify EnvValue behavior within structs for both map and array forms, as well as the not-set scenario.
internal/core/spec/types/port.go (1)
1-65: Implementation is otherwise correct.The PortValue implementation properly handles string and integer port values, provides appropriate accessors, and handles the zero-value case correctly.
internal/core/spec/types/stringarray.go (2)
40-50: Intentional stringification for numeric compatibility.The implementation stringifies non-string array elements (line 47), which is intentional to support numeric tags and maintain compatibility. The comment clearly documents this behavior.
1-90: LGTM! Clean StringOrArray implementation.The implementation properly handles all expected YAML input forms, provides appropriate accessors to distinguish between zero-value and empty-but-set, and includes well-documented type aliases for domain-specific uses (MailToValue, TagsValue).
internal/core/spec/types/schedule.go (4)
1-28: Well-structured type definition with clear documentation.The
ScheduleValuetype follows a consistent pattern with the other typed union types in this PR, with good YAML examples in the documentation.
30-76: LGTM - Comprehensive type handling in UnmarshalYAML.The implementation correctly handles all expected YAML input forms. The
[]stringcase (lines 59-62) provides good defensive coverage for programmatic use, even though YAML typically produces[]any.
78-97: Good validation with unknown key rejection.The map parsing correctly validates keys and provides clear error messages for unknown keys.
118-137: Clean accessor implementations following consistent patterns.Value receivers are appropriate for these read-only accessors, consistent with other types in this package.
internal/core/spec/loader.go (4)
17-17: Import added for new types package.The import aligns with the typed union decode hook usage below.
540-552: Decode function updated to return new manifest type.The transition from
*definitionto*dagaligns with the architectural refactoring. TheTypedUnionDecodeHookintegration enables custom YAML unmarshalling for typed union fields.
586-603: Double serialization is necessary but adds overhead.The
decodeViaYAMLhelper correctly enables customUnmarshalYAMLmethods to be invoked. The double serialization (data → YAML bytes → target type) is the cost of bridging mapstructure with go-yaml's custom unmarshalers.For performance-critical scenarios, consider caching or alternative approaches, but this is acceptable for configuration loading which happens infrequently.
348-380: Multi-document loading flow updated correctly.The transition to manifest-based decoding and method-based building is consistent throughout the function. Error handling and context management are preserved.
internal/core/spec/types/env.go (4)
29-39: Clear type definitions for environment variable handling.The
EnvEntrystruct provides a clean representation for key-value pairs, andEnvValuefollows the established pattern for typed union types.
77-96: Array parsing handles mixed formats correctly.The function supports both map and string entries in arrays, with clear error messages including the index position.
98-105: Simple and effective value stringification.Using
%vformat is appropriate for converting various YAML types to environment variable string values.
69-75: Map iteration order inparseMapis non-deterministic; users requiring ordered definitions should use the array syntax.The iteration order over maps is not specified and is not guaranteed to be the same from one iteration to the next. The
parseMapfunction iterates directly over the map argument, which means environment variable ordering cannot be guaranteed when using map form in YAML. This is acceptable since the documentation explicitly recommends the array form for ordered definitions, which is preserved through the array structure while supporting both map and string formats.internal/core/spec/dag.go (6)
18-105: Comprehensive intermediate representation mirroring YAML structure.The
dagstruct and associated helper types provide a clean intermediate representation that maps directly to the YAML schema. The use of typed union types (types.ShellValue,types.EnvValue, etc.) enables flexible YAML parsing.
214-306: Well-structured build method with transformer pattern.The transformer pattern provides clean separation of concerns and consistent error handling. The
metaflag for each transformer correctly distinguishes metadata-only fields from full build requirements.The error aggregation with
BuildFlagAllowBuildErrorssupport allows graceful degradation when loading DAGs with errors.
402-419: Environment variable builder correctly populates build context.The method stores env vars in
ctx.buildEnvfor cross-transformer communication (e.g., allowing params to reference env vars likeP2=${A001}). SinceBuildContextcontains a map field, modifications are visible to subsequent transformers within the same build call.
988-1071: Comprehensive step building with support for multiple formats.The method correctly handles:
- Array of steps with chain dependency injection
- Nested arrays for parallel step groups
- Map-based step definitions with names as keys
The use of
TypedUnionDecodeHookfor map decoding (line 1051) ensures consistent handling of typed union fields within steps.
325-341: Type validation with clear defaults and future-proofing.Defaulting to
TypeChainwhen type is empty, and reservingTypeAgentfor future use with a clear error message, provides good forward compatibility.
566-603: Robust working directory resolution with sensible fallbacks.The cascade of fallbacks (explicit → default → file directory → current directory → home directory) provides resilient behavior. The fallback to
UserHomeDirwhenGetwdfails is reasonable for edge cases.internal/core/spec/types/continueon.go (5)
25-33: Clear type definition for continue-on configuration.The
ContinueOnValuestruct captures all supported continuation conditions with a consistent pattern matching other typed union types.
35-69: Good case-insensitive string handling.Using
strings.ToLower(strings.TrimSpace(v))for the string shorthand allows flexible input like "Skipped", "FAILED", etc.
71-109: Good backward compatibility with "failure" alias.Supporting both "failed" and "failure" (line 80) provides flexibility for users. Unknown keys are properly rejected with clear error messages.
135-186: Integer parsing handles various YAML numeric representations.The functions correctly handle Go's type variations from YAML parsing (
int,int64,float64,uint64). Thenolint:goseccomments appropriately acknowledge the uint64 overflow risk, which is acceptable since exit codes are small numbers (typically 0-255).Note: The
float64tointconversion truncates rather than rounds, which is the expected behavior for exit codes parsed from YAML.
188-207: Clean accessor implementations.The accessors follow the established pattern with value receivers for read-only access.
internal/core/spec/step.go (17)
1-14: LGTM!Imports are well-organized and all appear to be used throughout the file.
16-85: LGTM!The step struct is comprehensive with good documentation. The deprecation notices for
DirandRunare clear.
87-106: LGTM!Using
anytype for fields that accept multiple input types (bool/string/number) is appropriate for flexible YAML parsing, with proper type validation in the corresponding build methods.
108-160: LGTM!The transformer pipeline pattern is clean and maintainable. Error aggregation provides good user experience by reporting all validation issues at once. The comment on line 138 explaining the ordering constraint is helpful.
174-198: LGTM!Shell parsing correctly handles both array and string forms, with proper error context on parse failures.
200-209: LGTM!Timeout validation and duration conversion are correct.
211-220: LGTM!The distinction between unset and explicitly empty dependencies is correctly handled, which is important for chain-type DAG behavior.
222-234: LGTM!Clean mapping from the typed
ContinueOnValuetocore.ContinueOn.
236-305: LGTM!The retry policy parsing handles various YAML input types correctly. The backoff validation ensuring either 0 (disabled) or >1.0 (exponential growth) is a good constraint.
405-415: LGTM!Signal validation is correct.
417-429: LGTM!Output field parsing correctly handles both variable (
$VAR) and direct forms.
431-439: LGTM!Environment variable mapping is straightforward and correct.
441-454: LGTM!Correctly handles both
preconditionandpreconditionsfields, merging them together.
527-546: LGTM!Parameter parsing and conversion to
core.SimpleParamsis clean.
548-605: LGTM!Executor parsing correctly handles string, map, and nil forms with appropriate inference from DAG context.
671-723: LGTM!Sub-DAG handling correctly manages the deprecated
Runfield with warnings, parameter serialization, and executor type selection based on parallel configuration.
725-767: LGTM!Parallel items parsing handles various value types appropriately, and using
DeterministicMapensures consistent ordering for reproducible builds.internal/core/spec/builder.go (7)
12-78: LGTM!
BuildContext,StepBuildContext,BuildFlag, andBuildOptsare well-structured with clear documentation.
80-148: LGTM!Precondition parsing is comprehensive, handling all expected input forms with proper validation.
186-217: LGTM!Type-based step name generation provides meaningful auto-generated names with uniqueness guarantee.
236-255: LGTM!The refactored
buildStepFromRawcleanly integrates the newsteptype withTypedUnionDecodeHookfor proper unmarshaling of custom types.
257-294: LGTM!Chain dependency injection correctly handles the preference for ID over Name and checks both to avoid duplicate entries.
296-305: LGTM!The alternate key lookup logic is correct and handles the ID/Name relationship properly.
150-184: LGTM!Secret reference parsing includes proper validation for required fields and duplicate detection. Error messages include the index for easy identification. The
secretReftype is properly defined ininternal/core/spec/dag.goand the validation pattern is consistent with the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/core/spec/step.go (1)
380-402: Consider extracting backoff parsing to a shared helper.The backoff parsing logic (type switch, default multiplier for
true, validation for values in (0, 1.0]) is duplicated betweenbuildRetryPolicy(lines 283-304) andbuildRepeatPolicy(lines 381-402).🔎 Suggested helper function
// parseBackoff parses a backoff value that can be bool or numeric. // Returns the parsed value and any validation error. func parseBackoff(fieldName string, value any) (float64, error) { var backoff float64 switch v := value.(type) { case bool: if v { backoff = 2.0 // Default multiplier when true } case int: backoff = float64(v) case int64: backoff = float64(v) case float64: backoff = v default: return 0, fmt.Errorf("invalid type for %s: %T", fieldName, v) } if backoff > 0 && backoff <= 1.0 { return 0, fmt.Errorf("%s must be greater than 1.0 for exponential growth", fieldName) } return backoff, nil }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/core/spec/schedule.go(0 hunks)internal/core/spec/step.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/core/spec/schedule.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/core/spec/step.go
🧬 Code graph analysis (1)
internal/core/spec/step.go (8)
internal/core/spec/types/shell.go (1)
ShellValue(21-26)internal/core/spec/types/stringarray.go (1)
StringOrArray(18-22)internal/core/spec/types/continueon.go (1)
ContinueOnValue(25-33)internal/core/params.go (2)
Params(36-45)NewSimpleParams(48-50)internal/core/spec/types/env.go (1)
EnvValue(29-33)internal/core/spec/builder.go (2)
StepBuildContext(25-28)BuildContext(13-22)internal/common/cmdutil/cmdutil.go (2)
SplitCommand(142-169)JoinCommandArgs(20-22)internal/core/spec/errors.go (7)
ErrInvalidSignal(21-21)ErrStepCommandIsEmpty(29-29)ErrStepCommandMustBeArrayOrString(30-30)ErrExecutorTypeMustBeString(23-23)ErrExecutorConfigValueMustBeMap(24-24)ErrExecutorHasInvalidKey(25-25)ErrExecutorConfigMustBeStringOrMap(26-26)
⏰ 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 (11)
internal/core/spec/step.go (11)
1-15: LGTM!Package declaration and imports are well-organized, with standard library imports separated from internal packages.
17-86: LGTM!The
stepstruct is well-documented with clear field descriptions. The use of typed values from thetypespackage (ShellValue,StringOrArray, etc.) for structured parsing, combined withanyfor flexible YAML unmarshaling where needed, is appropriate for this transformation layer.
88-107: LGTM!The policy structs use
anyappropriately to support flexible YAML input types (bool, string, number), with validation deferred to the respective build methods.
109-161: LGTM!The transformer pipeline pattern is clean and maintainable. Error aggregation via
core.ErrorListprovides comprehensive validation feedback. The explicit ordering comment forparallelbeforesubDAGis helpful for future maintainers.
163-199: LGTM!The
buildWorkingDircorrectly prioritizesWorkingDirover the deprecatedDirfield. ThebuildShellmethod properly handles both array and string forms, usingcmdutil.SplitCommandfor string parsing.
201-235: LGTM!The timeout validation, dependency handling with explicit no-deps flag, and continue-on field mapping are all correctly implemented.
237-312: LGTM!The retry policy builder handles type flexibility well with appropriate overflow checks for
uint64conversions. The validation requiring bothlimitandintervalSecwhenretryPolicyis specified ensures complete configuration.Note: YAML parsers typically unmarshal integers as
intorfloat64depending on the library. The multiple integer type cases (int,int64,uint64) provide good coverage for different parsing behaviors.
412-461: LGTM!Signal validation, output variable handling (with or without
$prefix), environment variable formatting, and precondition merging from both singular and plural fields are all correctly implemented.
534-679: LGTM!The builders for params, executor, and parallel are well-implemented with comprehensive type handling. The executor inference from DAG context (container/SSH) when nil is a good default behavior.
681-733: LGTM!The sub-DAG builder correctly handles both
Calland the deprecatedRunfield with appropriate warnings. The executor type selection based on parallel configuration ensures correct execution mode. Parameter parsing withBuildFlagNoEvalprevents premature variable expansion.
735-777: LGTM!The
parseParallelItemsmethod handles various input types robustly and usescollections.DeterministicMapto ensure consistent parameter ordering, which is important for reproducible behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/core/spec/builder_test.go (1)
4399-4455: Comprehensive timeout validation tests.The test suite covers positive, zero (explicit and omitted), and negative timeout scenarios. All validation logic is properly tested.
Minor observation: The
NegativeTimeoutsubtest could also be markedt.Parallel()since it doesn't modify shared state or environment variables.Optional: Add t.Parallel() to NegativeTimeout test
// Negative timeout should fail validation t.Run("NegativeTimeout", func(t *testing.T) { + t.Parallel() + data := []byte(` steps:internal/core/spec/types/continueon_test.go (1)
105-121: Consider adding a standalone test formarkSuccess.While
markSuccessis tested here as part of the "all fields" scenario, the other boolean fields (skippedandfailed) have dedicated standalone tests (lines 51-67). For consistency, consider adding a standalone test case:t.Run("map form - markSuccess only", func(t *testing.T) { data := `markSuccess: true` var c types.ContinueOnValue err := yaml.Unmarshal([]byte(data), &c) require.NoError(t, err) assert.True(t, c.MarkSuccess()) assert.False(t, c.Skipped()) assert.False(t, c.Failed()) })This is purely optional and improves test symmetry.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/core/spec/builder_test.go(2 hunks)internal/core/spec/loader_test.go(2 hunks)internal/core/spec/types/continueon_test.go(1 hunks)internal/core/spec/types/port.go(1 hunks)internal/core/spec/types/port_test.go(1 hunks)internal/core/spec/types/shell_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/core/spec/types/port_test.go
- internal/core/spec/types/port.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/core/spec/types/continueon_test.gointernal/core/spec/loader_test.gointernal/core/spec/types/shell_test.gointernal/core/spec/builder_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/core/spec/types/continueon_test.gointernal/core/spec/loader_test.gointernal/core/spec/types/shell_test.gointernal/core/spec/builder_test.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{yaml,yml} : Dagu workflows must use YAML format with support for JSON Schema for IDE auto-completion
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{yaml,yml} : Dagu YAML configurations should support minimal setup with sensible defaults and support for environment variable expansion and command substitution
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/core/spec/types/continueon_test.gointernal/core/spec/types/shell_test.gointernal/core/spec/builder_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/core/spec/types/continueon_test.gointernal/core/spec/types/shell_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/types/continueon_test.go
🧬 Code graph analysis (3)
internal/core/spec/types/continueon_test.go (2)
internal/core/spec/types/continueon.go (1)
ContinueOnValue(25-33)internal/core/step.go (1)
ContinueOn(226-232)
internal/core/spec/loader_test.go (3)
internal/core/spec/loader.go (9)
Load(150-171)OnlyMetadata(72-76)WithDAGsDir(90-94)WithAllowBuildErrors(101-105)SkipSchemaValidation(108-112)WithBaseConfig(44-48)WithSkipBaseHandlers(117-121)WithParams(51-62)WithoutEval(65-69)internal/core/dag.go (2)
MailOn(485-488)HandlerOn(476-482)internal/core/execution/context.go (1)
WithParams(169-173)
internal/core/spec/types/shell_test.go (1)
internal/core/spec/types/shell.go (1)
ShellValue(21-26)
⏰ 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 (11)
internal/core/spec/types/shell_test.go (1)
1-143: Excellent test coverage for ShellValue!The test suite is comprehensive and well-structured:
- Covers all input forms (string, array, inline, multiline)
- Tests edge cases (empty, null, zero-value)
- Validates error handling for invalid types
- Includes integration tests with struct embedding
- Follows Go testing best practices with testify/require and assert
internal/core/spec/loader_test.go (3)
54-77: Good expansion of metadata-only test coverage.The test now explicitly verifies that non-metadata fields from YAML are correctly ignored in metadata-only mode, while defaults are still applied by initialization. This improves clarity and correctness validation.
630-753: Comprehensive coverage of loader options.The new test function thoroughly validates various loader options with clean test setup and appropriate use of parallel execution. Each subtest clearly verifies the expected behavior.
755-772: Correct handling of environment manipulation in test.Good decision to avoid
t.Parallel()since this test usest.Setenv(), which modifies the environment and would cause issues with concurrent tests. The test properly validates that environment variable expansion is disabled with theWithoutEvaloption.internal/core/spec/builder_test.go (5)
1478-1507: Good validation test coverage for retry policy.The new tests properly validate that both
limitandintervalSecare required fields whenretryPolicyis specified. The tests follow the established pattern and use appropriate assertions.
4457-4524: Excellent handler test coverage.The tests comprehensively validate handler functionality including all handler types, backward compatibility with deprecated
cancel, and proper conflict detection betweenabortandcancel. Well-structured and thorough.
4526-4807: Exceptional test coverage for parallel execution.This comprehensive test suite covers all aspects of parallel execution configuration:
- Multiple input forms (string, array, object)
- Various value types (strings, numbers, objects, booleans)
- Configuration options (maxConcurrent as int and float)
- Error handling for invalid types and values
- Executor type inference
The tests are well-organized and thoroughly validate the complex parallel execution feature.
4809-4856: Good coverage of registry authentication formats.The tests validate different input formats for registry authentication (JSON strings and per-registry maps) and properly test error handling for invalid types. This complements the existing registry auth tests with additional scenarios.
4858-4915: Solid integration tests for build options.The tests properly validate build flag behavior:
WithoutEvalcorrectly avoids parallelization due to environment modificationWithAllowBuildErrorsverifies error collection without failing the buildSkipSchemaValidationconfirms schema validation can be bypassedGood integration test coverage that complements the loader option tests.
internal/core/spec/types/continueon_test.go (2)
1-10: LGTM! Clean package structure and imports.The external test package
types_testappropriately tests the public API, and the imports follow the coding guidelines by usingstretchr/testify/requireandassert.
216-254: LGTM! Good integration testing.The struct integration tests appropriately verify that
ContinueOnValueworks correctly when embedded in a configuration struct, covering string form, map form, and the unset scenario withIsZero().
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/core/spec/types/schedule_test.go (1)
162-216: Thorough edge case coverage.This additional test suite effectively covers important edge cases including the
Value()method, null/empty distinctions, and various invalid type scenarios. The tests complement the main suite well.💡 Optional: Consider consolidating similar error cases
The error test cases (lines 194-215) test similar invalid type scenarios. If you prefer more concise test code, these could optionally be consolidated into a table-driven test with test cases defining the input and expected error substring. However, the current structure is clear and perfectly acceptable.
internal/core/spec/types/env_test.go (2)
65-76: Complete the test assertions.The "mixed array" test only verifies the count of entries but doesn't validate the actual key-value pairs or their order. Since this test specifically checks mixed formats (maps and strings), it should verify that both formats are parsed correctly.
🔎 Suggested assertions to add
t.Run("mixed array - maps and strings", func(t *testing.T) { data := ` - KEY1: value1 - KEY2=value2 - KEY3: value3 ` var e types.EnvValue err := yaml.Unmarshal([]byte(data), &e) require.NoError(t, err) entries := e.Entries() require.Len(t, entries, 3) + assert.Equal(t, "KEY1", entries[0].Key) + assert.Equal(t, "value1", entries[0].Value) + assert.Equal(t, "KEY2", entries[1].Key) + assert.Equal(t, "value2", entries[1].Value) + assert.Equal(t, "KEY3", entries[2].Key) + assert.Equal(t, "value3", entries[2].Value) })
243-265: Consider table-driven tests for invalid type cases.The three invalid type tests (lines 243-265) follow an identical pattern and could be consolidated into a single table-driven test to reduce duplication and improve maintainability.
🔎 Example table-driven approach
- t.Run("invalid type in array - number", func(t *testing.T) { - data := `[123]` - var e types.EnvValue - err := yaml.Unmarshal([]byte(data), &e) - require.Error(t, err) - assert.Contains(t, err.Error(), "expected map or string") - }) - - t.Run("invalid type in array - boolean", func(t *testing.T) { - data := `[true]` - var e types.EnvValue - err := yaml.Unmarshal([]byte(data), &e) - require.Error(t, err) - assert.Contains(t, err.Error(), "expected map or string") - }) - - t.Run("invalid type - number", func(t *testing.T) { - data := `123` - var e types.EnvValue - err := yaml.Unmarshal([]byte(data), &e) - require.Error(t, err) - assert.Contains(t, err.Error(), "must be map or array") - }) + tests := []struct { + name string + data string + errContains string + }{ + { + name: "invalid type in array - number", + data: `[123]`, + errContains: "expected map or string", + }, + { + name: "invalid type in array - boolean", + data: `[true]`, + errContains: "expected map or string", + }, + { + name: "invalid type - number", + data: `123`, + errContains: "must be map or array", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var e types.EnvValue + err := yaml.Unmarshal([]byte(tt.data), &e) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errContains) + }) + }internal/core/spec/types/stringarray_test.go (1)
22-34: Add IsEmpty() and IsZero() assertions for consistency.These test cases verify
Values()but omitIsEmpty()andIsZero()checks. For consistency with other subtests (lines 13-20, 36-44) and to ensure the type's state methods behave correctly, consider adding these assertions.🔎 Suggested assertions to add
For the "array of strings inline" test:
assert.Equal(t, []string{"step1", "step2", "step3"}, s.Values()) + assert.False(t, s.IsEmpty()) + assert.False(t, s.IsZero())For the "multiline array" test:
assert.Equal(t, []string{"step1", "step2"}, s.Values()) + assert.False(t, s.IsEmpty()) + assert.False(t, s.IsZero())
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/core/spec/types/continueon_test.gointernal/core/spec/types/env_test.gointernal/core/spec/types/port_test.gointernal/core/spec/types/schedule_test.gointernal/core/spec/types/shell_test.gointernal/core/spec/types/stringarray_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/core/spec/types/shell_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/core/spec/types/continueon_test.go
- internal/core/spec/types/port_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/core/spec/types/env_test.gointernal/core/spec/types/schedule_test.gointernal/core/spec/types/stringarray_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/core/spec/types/env_test.gointernal/core/spec/types/schedule_test.gointernal/core/spec/types/stringarray_test.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{yaml,yml} : Dagu workflows must use YAML format with support for JSON Schema for IDE auto-completion
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/core/spec/types/env_test.gointernal/core/spec/types/schedule_test.gointernal/core/spec/types/stringarray_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/core/spec/types/env_test.go
🧬 Code graph analysis (2)
internal/core/spec/types/env_test.go (1)
internal/core/spec/types/env.go (1)
EnvValue(29-33)
internal/core/spec/types/schedule_test.go (1)
internal/core/spec/types/schedule.go (1)
ScheduleValue(22-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 (8)
internal/core/spec/types/schedule_test.go (3)
1-10: Excellent package structure and imports.Using the
types_testpackage for black-box testing is a best practice, and the imports are clean and appropriate.
12-119: Comprehensive test coverage for YAML unmarshalling.This test function thoroughly covers all supported formats (single string, arrays, maps with start/stop/restart keys) and includes proper error path testing. The use of subtests with descriptive names makes the test suite highly maintainable.
121-160: Well-designed integration tests.These tests verify that
ScheduleValuecorrectly integrates as a struct field, which is essential for validating the real-world usage pattern in DAG configurations. The test cases appropriately cover simple, complex, and omitted schedule scenarios.internal/core/spec/types/env_test.go (3)
1-10: LGTM!Package declaration and imports follow Go conventions. Using
types_testpackage for black-box testing is appropriate, and the use of testify aligns with project guidelines.
168-212: LGTM!The struct integration tests provide solid coverage of how EnvValue behaves when embedded in a config struct. The tests correctly verify both the zero-value case and different YAML representations (map vs array).
214-242: LGTM!These tests provide excellent coverage of the Value() method's behavior with different YAML types and the null/zero-value handling. The type assertions properly validate the raw value storage.
internal/core/spec/types/stringarray_test.go (2)
1-204: Excellent test coverage and structure.The test suite is comprehensive and well-organized, following Go best practices with table-driven subtests. It thoroughly exercises StringOrArray unmarshaling behavior across scalars, sequences, empty values, and error cases, and validates both the base type and its aliases (MailToValue, TagsValue). The tests align with the coding guidelines for test structure and assertions.
184-203: No changes needed. The tests correctly verify the intentional stringification behavior for StringOrArray. Array elements are explicitly converted to strings for compatibility (as documented in the implementation), while bare non-string scalars are rejected. This asymmetric design is documented in code comments and the tests properly cover both the happy path and error cases.
| t.Run("empty array", func(t *testing.T) { | ||
| var s types.StringOrArray | ||
| err := yaml.Unmarshal([]byte(`[]`), &s) | ||
| require.NoError(t, err) | ||
| assert.Empty(t, s.Values()) | ||
| assert.True(t, s.IsEmpty()) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add IsZero() assertion for the empty array case.
The test verifies IsEmpty() but omits IsZero(). Based on the semantics demonstrated in line 121, an explicitly set empty array should have IsZero() == false (to distinguish "not set" from "set to empty"). Add this assertion to validate the behavior.
🔎 Suggested assertion to add
assert.Empty(t, s.Values())
assert.True(t, s.IsEmpty())
+ assert.False(t, s.IsZero()) // Was explicitly set🤖 Prompt for AI Agents
In internal/core/spec/types/stringarray_test.go around lines 46 to 52, the test
for the "empty array" case currently asserts Values() is empty and IsEmpty() is
true but omits asserting IsZero(); add an assertion that the
empty-but-explicitly-set array is not considered "zero" by calling
assert.False(t, s.IsZero()) (use the same testing/assert style as the
surrounding assertions) to validate the semantics that an explicitly set empty
array differs from an unset value.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/core/spec/types/stringarray_test.go (1)
49-55: Add IsZero() assertion for the empty array case.The test verifies
IsEmpty()but omitsIsZero(). An explicitly set empty array should haveIsZero() == falseto distinguish "not set" from "set to empty" (as demonstrated in line 167). Add this assertion to validate the behavior.🔎 Suggested assertion to add
{ name: "EmptyArray", input: "[]", wantValues: nil, checkIsEmpty: true, wantIsEmpty: true, + checkNotZero: true, },internal/core/spec/step.go (1)
578-599: String command case missingCmdArgsSysassignment.This was flagged in a previous review. The string command parsing (lines 592-598) sets
CmdWithArgs,Command, andArgsbut does not setCmdArgsSys, unlike the array command case (line 630). Runtime code ininternal/runtime/node.godepends onCmdArgsSysbeing populated.🔎 Proposed fix
result.CmdWithArgs = val cmd, args, err := cmdutil.SplitCommand(val) if err != nil { return core.NewValidationError("command", val, fmt.Errorf("failed to parse command: %w", err)) } result.Command = strings.TrimSpace(cmd) result.Args = args + result.CmdArgsSys = cmdutil.JoinCommandArgs(result.Command, result.Args)
🧹 Nitpick comments (10)
internal/core/spec/types/continueon_test.go (2)
25-26: Consider removing unusedcheckIsZerofield from test table.The
checkIsZerofield on line 25 is defined but never set totruein any test case within this table. The zero-value behavior is tested separately in the "ZeroValue" subtest at line 209. Consider removing this unused field to reduce confusion.🔎 Proposed fix
tests := []struct { name string input string wantErr bool errContains string wantSkipped bool wantFailed bool wantExitCode []int wantOutput []string wantMarkSuccess bool - checkIsZero bool checkNotZero bool }{And update the test runner accordingly:
require.NoError(t, err) - if tt.checkIsZero { - assert.True(t, c.IsZero()) - return - } if tt.checkNotZero {
324-327: Good addition of the "failure" alias test.This addresses the past review request. However, consider adding two additional edge cases for complete coverage of the "failure" alias behavior:
- Combined
failure: truewithfailed: truein the same map (both should result inFailed()being true)- String form
"failure"should be rejected (only "skipped" and "failed" are valid string values)🔎 Suggested additional test cases
{ name: "FailureAndFailedCombined", input: "failure: true\nfailed: true", wantFailed: true, }, { name: "StringFailureRejected", input: "failure", wantErr: true, errContains: "expected 'skipped' or 'failed'", },internal/core/spec/types/env_test.go (1)
46-53: Incomplete assertion forArrayOfStringstest case.The
wantEntriesmap only verifiesKEY1but notKEY2, even thoughwantEntryCountis 2. This appears to be an oversight.🔎 Proposed fix
{ name: "ArrayOfStrings", input: ` - KEY1=value1 - KEY2=value2 `, wantEntryCount: 2, - wantEntries: map[string]string{"KEY1": "value1"}, + wantEntries: map[string]string{"KEY1": "value1", "KEY2": "value2"}, },internal/core/spec/schema_test.go (3)
565-599: Missingt.Parallel()at top-level and CWD change affects test isolation.
TestBuildParamsSchemaResolutionis missingt.Parallel()at the top level (unlike other tests in this file). The subtests change the current working directory withos.Chdir, which is inherently incompatible with parallel execution. While the cleanup restores CWD, this can still cause flakiness if run alongside other tests that depend on CWD.Consider documenting this limitation or refactoring to avoid CWD manipulation.
649-652: Silent error handling in CWD restoration.The deferred
os.Chdir(orig)discards the error with_ =. If the CWD restoration fails, subsequent tests could be affected without any indication. Consider logging the error or usingt.Cleanupwith error checking as done in the "FromWorkingDir" subtest.🔎 Proposed fix
- defer func() { _ = os.Chdir(orig) }() + t.Cleanup(func() { + if err := os.Chdir(orig); err != nil { + t.Errorf("failed to restore working directory: %v", err) + } + })
497-503: Consider usingt.TempDir()for automatic cleanup.Using
os.CreateTempwith manualdefer os.Removeis less idiomatic thant.TempDir()which handles cleanup automatically. Other tests in this file already uset.TempDir().🔎 Proposed fix
- tmpFile, err := os.CreateTemp("", "test-schema-*.json") - require.NoError(t, err) - defer func() { _ = os.Remove(tmpFile.Name()) }() - - _, err = tmpFile.WriteString(schemaContent) - require.NoError(t, err) - require.NoError(t, tmpFile.Close()) + tmpDir := t.TempDir() + schemaPath := filepath.Join(tmpDir, "schema.json") + require.NoError(t, os.WriteFile(schemaPath, []byte(schemaContent), 0600))Then update the YAML template to use
schemaPathinstead oftmpFile.Name().internal/core/spec/step.go (3)
298-312: Shell parsing performed twice for each step.
parseStepShellInternalis called separately forbuildStepShellandbuildStepShellArgs, parsing the same shell value twice. Consider caching the result or combining these into a single transformer that sets both fields.
390-412: Backoff parsing logic duplicated between retry and repeat policies.The backoff parsing and validation (bool → 2.0 default, numeric conversion, >1.0 validation) is nearly identical in
buildStepRetryPolicyandbuildStepRepeatPolicy. Consider extracting a shared helper function.🔎 Example helper
func parseBackoff(field string, value any) (float64, error) { switch v := value.(type) { case bool: if v { return 2.0, nil // Default multiplier } return 0, nil case int: return float64(v), nil case int64: return float64(v), nil case float64: return v, nil default: return 0, fmt.Errorf("invalid type for %s: %T", field, v) } }Also applies to: 489-511
771-772:float64tointconversion truncates without warning.When
maxConcurrentis specified as a float (e.g.,5.9), it's silently truncated to5. This is likely fine for configuration, but consider validating that the float is a whole number or documenting this behavior.internal/core/spec/dag.go (1)
621-629: Document the limitation with parameter-derived environment variables.The code intentionally ignores
envsderived from parameters (line 629), with a comment noting "This is a limitation of the current transformer design." While this is acknowledged, it's worth clarifying:
- Impact: Parameters won't be automatically available as environment variables during the DAG build process
- Workaround: If parameter-to-env propagation is needed, it should be handled at a different layer
Consider either:
- Adding a TODO comment with a tracking issue reference
- Documenting this limitation in user-facing documentation
💡 Suggested improvement
// Note: envs from params are handled separately - they should be appended to Env // This is a limitation of the current transformer design; we may need to handle this specially + // TODO(tracking-issue): Implement parameter-to-environment propagation in the build pipeline _ = envs
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
internal/core/spec/builder_test.gointernal/core/spec/dag.gointernal/core/spec/dag_test.gointernal/core/spec/loader_test.gointernal/core/spec/params.gointernal/core/spec/params_test.gointernal/core/spec/schema.gointernal/core/spec/schema_test.gointernal/core/spec/step.gointernal/core/spec/step_test.gointernal/core/spec/types/continueon_test.gointernal/core/spec/types/env_test.gointernal/core/spec/types/port_test.gointernal/core/spec/types/schedule_test.gointernal/core/spec/types/shell_test.gointernal/core/spec/types/stringarray_test.gointernal/core/spec/variables_test.go
💤 Files with no reviewable changes (1)
- internal/core/spec/params.go
✅ Files skipped from review due to trivial changes (1)
- internal/core/spec/step_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/core/spec/types/shell_test.go
- internal/core/spec/types/port_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/core/spec/variables_test.gointernal/core/spec/types/schedule_test.gointernal/core/spec/loader_test.gointernal/core/spec/types/continueon_test.gointernal/core/spec/params_test.gointernal/core/spec/dag_test.gointernal/core/spec/dag.gointernal/core/spec/schema.gointernal/core/spec/types/env_test.gointernal/core/spec/schema_test.gointernal/core/spec/types/stringarray_test.gointernal/core/spec/step.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/core/spec/variables_test.gointernal/core/spec/types/schedule_test.gointernal/core/spec/loader_test.gointernal/core/spec/types/continueon_test.gointernal/core/spec/params_test.gointernal/core/spec/dag_test.gointernal/core/spec/types/env_test.gointernal/core/spec/schema_test.gointernal/core/spec/types/stringarray_test.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{yaml,yml} : Dagu workflows must use YAML format with support for JSON Schema for IDE auto-completion
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/core/spec/variables_test.gointernal/core/spec/types/schedule_test.gointernal/core/spec/loader_test.gointernal/core/spec/types/continueon_test.gointernal/core/spec/params_test.gointernal/core/spec/dag_test.gointernal/core/spec/types/env_test.gointernal/core/spec/schema_test.gointernal/core/spec/types/stringarray_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/core/spec/variables_test.gointernal/core/spec/types/continueon_test.gointernal/core/spec/params_test.gointernal/core/spec/types/env_test.gointernal/core/spec/schema_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/variables_test.gointernal/core/spec/types/continueon_test.gointernal/core/spec/params_test.gointernal/core/spec/dag_test.go
🧬 Code graph analysis (8)
internal/core/spec/types/schedule_test.go (1)
internal/core/spec/types/schedule.go (1)
ScheduleValue(22-28)
internal/core/spec/loader_test.go (1)
internal/core/spec/loader.go (10)
Load(150-171)OnlyMetadata(72-76)LoadYAMLWithOpts(191-217)WithDAGsDir(90-94)WithAllowBuildErrors(101-105)SkipSchemaValidation(108-112)WithBaseConfig(44-48)WithSkipBaseHandlers(117-121)WithParams(51-62)WithoutEval(65-69)
internal/core/spec/types/continueon_test.go (1)
internal/core/spec/types/continueon.go (1)
ContinueOnValue(25-33)
internal/core/spec/dag_test.go (6)
internal/core/spec/builder.go (2)
BuildContext(13-22)BuildOpts(56-73)internal/core/spec/types/port.go (1)
PortValue(16-20)internal/core/spec/types/stringarray.go (1)
StringOrArray(18-22)internal/core/dag.go (6)
TypeChain(27-27)TypeGraph(25-25)MailOn(485-488)AuthConfig(392-400)OTelConfig(507-514)SecretRef(139-148)internal/core/container.go (4)
PullPolicyMissing(92-92)PullPolicyAlways(90-90)StartupCommand(54-54)WaitForHealthy(62-62)internal/core/spec/types/shell.go (1)
ShellValue(21-26)
internal/core/spec/schema.go (1)
internal/common/fileutil/fileutil.go (2)
ResolvePath(153-183)FileExists(44-47)
internal/core/spec/types/env_test.go (1)
internal/core/spec/types/env.go (1)
EnvValue(29-33)
internal/core/spec/types/stringarray_test.go (1)
internal/core/spec/types/stringarray.go (3)
StringOrArray(18-22)MailToValue(83-83)TagsValue(90-90)
internal/core/spec/step.go (8)
internal/core/spec/types/shell.go (1)
ShellValue(21-26)internal/core/step.go (2)
ContinueOn(226-232)ExecutorConfig(107-115)internal/core/params.go (2)
Params(36-45)NewSimpleParams(48-50)internal/core/spec/types/env.go (1)
EnvValue(29-33)internal/core/spec/builder.go (2)
StepBuildContext(25-28)BuildContext(13-22)internal/core/spec/dag.go (1)
Transformer(217-220)internal/common/cmdutil/cmdutil.go (2)
SplitCommand(142-169)JoinCommandArgs(20-22)internal/core/spec/errors.go (7)
ErrInvalidSignal(21-21)ErrStepCommandIsEmpty(29-29)ErrStepCommandMustBeArrayOrString(30-30)ErrExecutorTypeMustBeString(23-23)ErrExecutorConfigValueMustBeMap(24-24)ErrExecutorHasInvalidKey(25-25)ErrExecutorConfigMustBeStringOrMap(26-26)
⏰ 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 (24)
internal/core/spec/types/stringarray_test.go (4)
104-172: LGTM!Excellent coverage of StringOrArray behavior when embedded in a struct. The test correctly distinguishes between unset values and explicitly set empty arrays (line 167), demonstrating proper zero-value semantics.
174-203: LGTM!Appropriate coverage for the MailToValue alias. Since it's a type alias for StringOrArray, detailed behavior is tested in the main test function.
205-234: LGTM!Appropriate coverage for the TagsValue alias. The test structure is consistent with TestMailToValue and adequately validates the alias behavior.
236-309: LGTM!Excellent coverage of edge cases including numeric conversion, mixed types, invalid input handling, and raw value access. The tests comprehensively validate StringOrArray behavior across various scenarios.
internal/core/spec/types/continueon_test.go (3)
1-10: LGTM!Imports are appropriate. Using
types_testpackage for black-box testing is good practice, and the use ofstretchr/testify/requireandassertfollows the coding guidelines.
218-281: LGTM!Good integration test covering
ContinueOnValueembedded within a struct. Tests the practical use case including when the field is not set (wantIsZero).
283-303: LGTM!Good coverage of the
Value()accessor for both string and map forms.internal/core/spec/variables_test.go (2)
13-82: LGTM! Well-structured table-driven test.The test correctly handles map iteration order non-determinism by using content-based verification (lines 68-79), covers multiple primitive types, and properly uses
t.Parallel()throughout.
281-288: LGTM! Clean test helper.Correctly uses
t.Helper()and provides a concise way to constructEnvValueinstances from YAML for testing.internal/core/spec/types/env_test.go (1)
1-162: Well-structured test coverage for EnvValue.Good use of table-driven tests with comprehensive coverage of map form, array forms, error cases, zero-value behavior, and in-struct unmarshalling. The test structure follows coding guidelines with
t.Parallel()andstretchr/testify.internal/core/spec/schema_test.go (1)
1-833: Comprehensive schema resolution test coverage.Excellent test coverage for schema extraction, URL/file loading, resolution precedence (CWD vs workingDir vs DAG dir), validation, and defaults. Good use of
httptestfor HTTP mocking and table-driven tests.internal/core/spec/schema.go (2)
94-153: Well-designed schema file resolution with clear precedence.The tiered resolution strategy (CWD → workingDir → dagDir) is well-documented and the error aggregation provides good debugging information. The implementation correctly handles edge cases like whitespace-only paths.
155-174: Clean implementation of schema reference extraction.The function safely handles non-map params, missing keys, and non-string values without panicking.
internal/core/spec/step.go (1)
182-213: Well-structured step build pipeline.Good separation of concerns with the transformer pipeline for simple fields and explicit calls for complex transformations. Error aggregation via
core.ErrorListprovides comprehensive feedback.internal/core/spec/types/schedule_test.go (1)
1-305: Comprehensive test coverage for ScheduleValue.Excellent table-driven tests covering all input forms (string, array, map with start/stop/restart), error cases, and edge cases like null values and empty strings. Good use of
t.Parallel()and proper assertions withstretchr/testify.internal/core/spec/loader_test.go (3)
30-70: LGTM! Good refactoring to table-driven tests.The consolidation of error test cases into a table-driven approach improves maintainability and follows Go testing best practices.
72-95: LGTM! MetadataOnly test correctly validates behavior.The test properly verifies that:
- Non-metadata fields from YAML are not populated in metadata-only mode
- Default values are still applied via InitializeDefaults
- Steps are not loaded
The assertions and comments clearly document the expected behavior.
668-810: LGTM! Comprehensive loader option tests.The new
TestLoadWithLoaderOptionstest function provides excellent coverage of various loader options including:
- WithDAGsDir for sub-DAG discovery
- WithAllowBuildErrors for capturing build-time errors
- SkipSchemaValidation for bypassing schema validation
- WithSkipBaseHandlers for excluding base config handlers
- WithParamsAsList for list-based parameter handling
The tests are well-structured and properly use
t.Parallel().internal/core/spec/params_test.go (1)
1-662: LGTM! Comprehensive parameter parsing test suite.This new test file provides excellent coverage of parameter handling across all parsing functions:
paramPairformatting (String/Escaped)- String, list, and map parameter parsing
- Parameter value parsing with various types
- Parameter overriding by name and position
- End-to-end parameter parsing with Eval/NoEval modes
- Parameter value evaluation with variable substitution
The tests are well-structured using table-driven patterns and properly use
t.Parallel()for concurrent execution.internal/core/spec/dag_test.go (3)
15-44: LGTM! Well-designed helper functions for test data creation.The helper functions (
testBuildContext,portValue,stringOrArray,stringOrArrayList) provide clean utilities for creating test data. Using YAML unmarshaling in the helpers ensures test data accurately reflects real-world parsing behavior.
46-1761: LGTM! Comprehensive DAG building test coverage.This test file provides excellent coverage of all DAG building functions with:
- Extensive tests for all field builders (type, name, group, timeout, tags, workers, containers, SSH, secrets, handlers, etc.)
- Good balance of success and error test cases
- Proper use of table-driven tests for similar scenarios
- Correct handling of
t.Setenvtests (avoidingt.Parallel()where needed)- Clear test names and assertions
The tests validate both the transformation logic and error handling for the new manifest-based spec build flow.
1113-1200: Implement missing shell helper functions for tests.The tests reference
shellValueandshellValueArrayhelper functions (lines 1113, 1119, 1157, 1172, 1187) that are not defined in the file. Implement these helpers following the pattern of existing helpers likeportValue,stringOrArray, andstringOrArrayListalready in the test file. These should constructtypes.ShellValueobjects from string and array inputs respectively.internal/core/spec/dag.go (2)
215-323: LGTM! Well-designed transformer pipeline.The transformer pipeline provides a clean, extensible architecture:
- Generic
Transformerinterface with type-safe implementations- Clear separation of metadata transformers (always run) vs full transformers (run when not in metadata-only mode)
- Consistent error handling via
wrapTransformError- Reflective field setting allows declarative transformer registration
This design makes it easy to add new DAG fields by simply adding a new transformer to the appropriate list.
382-1157: LGTM! Comprehensive and well-structured builder functions.The individual builder functions demonstrate good practices:
- Type validation: buildType validates against allowed values and provides clear error messages for unsupported types (lines 384-397)
- Flexible input handling: Registry auths support multiple formats (string, map[string]any, map[any]any) with appropriate type conversions (lines 801-871)
- Environment expansion: Consistently respects
BuildFlagNoEvalacross functions (e.g., lines 666-687, 715-716)- Backward compatibility: Container builder handles deprecated
WorkDirfield (lines 792-796)- Clear error reporting: Uses
core.ValidationErrorwith field names and values for debugging (e.g., line 755)- Deprecation handling: Handlers support both
Abort(canonical) andCancel(deprecated) with conflict detection (lines 946-962)The builders provide a solid foundation for the new manifest-based DAG spec transformation.
| req, err := http.NewRequest("GET", schemaURL, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create request: %w", err) | ||
| } | ||
|
|
||
| resp, err := http.DefaultClient.Do(req) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
HTTP request lacks timeout, risking indefinite blocking.
loadSchemaFromURL uses http.DefaultClient which has no default timeout. If the remote server is unresponsive, this could block the DAG loading process indefinitely.
🔎 Proposed fix
+import (
+ "context"
+ "time"
+ // ... other imports
+)
+const schemaFetchTimeout = 30 * time.Second
func loadSchemaFromURL(schemaURL string) (data []byte, err error) {
// Validate URL to prevent potential security issues (and satisfy linter :P)
parsedURL, err := url.Parse(schemaURL)
if err != nil {
return nil, fmt.Errorf("invalid URL: %w", err)
}
if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" {
return nil, fmt.Errorf("unsupported URL scheme: %s", parsedURL.Scheme)
}
- req, err := http.NewRequest("GET", schemaURL, nil)
+ ctx, cancel := context.WithTimeout(context.Background(), schemaFetchTimeout)
+ defer cancel()
+
+ req, err := http.NewRequestWithContext(ctx, "GET", schemaURL, nil)
if err != nil {
return nil, fmt.Errorf("failed to create request: %w", err)
}
- resp, err := http.DefaultClient.Do(req)
+ client := &http.Client{Timeout: schemaFetchTimeout}
+ resp, err := client.Do(req)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| req, err := http.NewRequest("GET", schemaURL, nil) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create request: %w", err) | |
| } | |
| resp, err := http.DefaultClient.Do(req) | |
| if err != nil { | |
| return nil, err | |
| ctx, cancel := context.WithTimeout(context.Background(), schemaFetchTimeout) | |
| defer cancel() | |
| req, err := http.NewRequestWithContext(ctx, "GET", schemaURL, nil) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create request: %w", err) | |
| } | |
| client := &http.Client{Timeout: schemaFetchTimeout} | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| return nil, err |
🤖 Prompt for AI Agents
In internal/core/spec/schema.go around lines 71-78, the code uses
http.DefaultClient.Do without a timeout which can block indefinitely; replace
that call by creating a dedicated http.Client with a reasonable Timeout (e.g.
5–15s) or perform the request with a context tied to context.WithTimeout and use
http.NewRequestWithContext, then call client.Do(ctxReq); ensure you cancel the
context (if using WithTimeout) and still handle/close resp.Body as before.
| t.Run("WithEvaluation", func(t *testing.T) { | ||
| ctx := BuildContext{ | ||
| ctx: context.Background(), | ||
| opts: BuildOpts{}, | ||
| } | ||
|
|
||
| input := map[string]any{"GREETING": "hello"} | ||
| result, err := loadVariables(ctx, input) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, "hello", result["GREETING"]) | ||
| }) |
There was a problem hiding this comment.
Inconsistent parallelization: missing t.Parallel() in subtests.
The WithEvaluation and VariableReference subtests don't use t.Parallel(), while all other subtests in this function do. Since each creates a fresh BuildContext with no apparent shared state, this appears unintentional.
🔎 Suggested fix
t.Run("WithEvaluation", func(t *testing.T) {
+ t.Parallel()
ctx := BuildContext{ t.Run("VariableReference", func(t *testing.T) {
+ t.Parallel()
ctx := BuildContext{Also applies to: 235-250
🤖 Prompt for AI Agents
In internal/core/spec/variables_test.go around lines 208-218 (and also for the
similar subtest at lines 235-250), the subtests "WithEvaluation" and
"VariableReference" are missing t.Parallel() while other subtests run in
parallel; add t.Parallel() as the first statement inside each of these subtest
functions so they run concurrently like the others, ensuring no shared state is
used or created before calling t.Parallel().
| t.Run("WithEvaluation", func(t *testing.T) { | ||
| ctx := BuildContext{ | ||
| ctx: context.Background(), | ||
| opts: BuildOpts{}, | ||
| } | ||
|
|
||
| env := envValueFromYAML(t, ` | ||
| GREETING: hello | ||
| `) | ||
| result, err := loadVariablesFromEnvValue(ctx, env) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, "hello", result["GREETING"]) | ||
| }) |
There was a problem hiding this comment.
Inconsistent parallelization: missing t.Parallel() in subtests.
Same pattern as TestLoadVariables—the WithEvaluation and VariableReference subtests lack t.Parallel() while all others use it. Since each creates an independent BuildContext, this appears unintentional.
🔎 Suggested fix
t.Run("WithEvaluation", func(t *testing.T) {
+ t.Parallel()
ctx := BuildContext{ t.Run("VariableReference", func(t *testing.T) {
+ t.Parallel()
ctx := BuildContext{Also applies to: 373-387
🤖 Prompt for AI Agents
internal/core/spec/variables_test.go around lines 343-355 (and similarly
373-387): the two subtests "WithEvaluation" and "VariableReference" are missing
t.Parallel() causing inconsistent parallelization with other subtests; add a
call to t.Parallel() as the first statement inside each of these subtest
functions so they run in parallel like the others, ensuring no shared state is
introduced and tests remain independent.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/runtime/agent/agent.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/agent/agent.go
⏰ 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
| coordinatorCli := a.createCoordinatorClient(ctx) | ||
|
|
||
| // Resolve secrets if defined | ||
| a.dag.LoadDotEnv(ctx) |
There was a problem hiding this comment.
Handle error from LoadDotEnv call.
The call to a.dag.LoadDotEnv(ctx) at line 295 doesn't capture or handle potential errors. Similar dotenv loading operations in Go return errors that should be checked, particularly for file I/O operations. Failures would be silently ignored, potentially leading to incomplete environment setup affecting downstream DAG execution and secret resolution.
If LoadDotEnv returns an error, add appropriate error handling:
if err := a.dag.LoadDotEnv(ctx); err != nil {
logger.Warn(ctx, "Failed to load .env file", tag.Error(err))
// Or return err if .env loading should be mandatory
}🤖 Prompt for AI Agents
In internal/runtime/agent/agent.go around line 295 the call to
a.dag.LoadDotEnv(ctx) ignores any returned error; update the call to capture the
error and handle it appropriately by checking if err != nil and then either log
a warning with the error (e.g., logger.Warn(ctx, "Failed to load .env file",
tag.Error(err))) or return the error if loading must be mandatory so upstream
callers can handle it.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1499 +/- ##
==========================================
+ Coverage 60.13% 61.75% +1.61%
==========================================
Files 196 203 +7
Lines 21916 22146 +230
==========================================
+ Hits 13180 13677 +497
+ Misses 7341 7095 -246
+ Partials 1395 1374 -21
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
Refactor
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.