refactor(core): validate config with json schema#1558
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughUpdated the jsonschema-go dependency and introduced a thread-safe schema registry for executor configurations. Added schema validation functions to the core system, integrated validation into the step build pipeline, and registered JSON schemas for nine executor types (archive, docker, gha, hitl, http, jq, mail, ssh). Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant EP as Executor Package<br/>(init)
participant Registry as Executor Schema<br/>Registry
participant StepPipeline as Step Build<br/>Pipeline
participant Validator as Validator
EP->>Registry: RegisterExecutorConfigSchema("type", schema)
Note over Registry: Store schema with lazy<br/>resolution support
App->>StepPipeline: Build Step
StepPipeline->>Validator: ValidateExecutorConfig(type, config)
alt Schema Registered
Validator->>Registry: Get schema for type
Registry->>Validator: Return schemaEntry
Validator->>Validator: Lazy resolve schema<br/>(first time only)
Validator->>Validator: Validate config
alt Valid
Validator-->>StepPipeline: nil (success)
else Invalid
Validator-->>StepPipeline: error with context
end
else No Schema
Validator-->>StepPipeline: nil (skip validation)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/runtime/builtin/docker/config_test.go (1)
10-58: Well-structured tests with good coverage.The table-driven test structure follows Go best practices and covers key validation scenarios. The test cases correctly verify the schema constraints defined in config.go (lines 305-318).
Consider adding test cases for additional edge cases:
- Invalid property types (e.g.,
"image": 123)- Unknown/additional properties
- Nested object validation (e.g., invalid
execobject structure)Optional: Additional test cases
+ { + name: "invalid image type", + config: map[string]any{"image": 123}, + wantErr: true, + }, + { + name: "invalid pull policy", + config: map[string]any{"image": "alpine", "pull": "invalid"}, + wantErr: true, + },internal/runtime/builtin/mail/config.go (1)
11-12: Thetofield is missing aTypedefinition.Unlike
fromwhich hasType: "string", thetofield has no type constraint. The description mentions it can be "string or array of strings", but without explicit type validation, any value type will be accepted. Consider usingOneOfto properly validate both cases:♻️ Suggested schema for polymorphic `to` field
- "to": {Description: "Recipient email address(es) - string or array of strings"}, + "to": { + Description: "Recipient email address(es) - string or array of strings", + OneOf: []*jsonschema.Schema{ + {Type: "string"}, + {Type: "array", Items: &jsonschema.Schema{Type: "string"}}, + }, + },internal/core/executor_schema_test.go (1)
1-9: Consider adding a test for schema resolution failure.The tests cover validation scenarios well, but there's no test for the error path when
entry.getResolved()fails (line 47-50 in executor_schema.go). This would ensure the "schema error for %s" message is properly formatted.♻️ Example test for resolution error
func TestValidateExecutorConfig_SchemaResolutionError(t *testing.T) { // Create a schema with invalid default that fails resolution schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ "value": {Type: "string", Default: 123}, // Invalid default type }, } RegisterExecutorConfigSchema("test_resolve_error", schema) err := ValidateExecutorConfig("test_resolve_error", map[string]any{}) require.Error(t, err) require.Contains(t, err.Error(), "schema error for test_resolve_error") }Based on learnings, tests should cover failure paths.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
go.modinternal/core/executor_schema.gointernal/core/executor_schema_test.gointernal/core/spec/step.gointernal/runtime/builtin/archive/config.gointernal/runtime/builtin/docker/config.gointernal/runtime/builtin/docker/config_test.gointernal/runtime/builtin/gha/config.gointernal/runtime/builtin/hitl/config.gointernal/runtime/builtin/http/config.gointernal/runtime/builtin/jq/config.gointernal/runtime/builtin/mail/config.gointernal/runtime/builtin/ssh/config.goschemas/dag.schema.json
🧰 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/builtin/docker/config_test.gointernal/runtime/builtin/archive/config.gointernal/runtime/builtin/mail/config.gointernal/runtime/builtin/ssh/config.gointernal/core/spec/step.gointernal/core/executor_schema.gointernal/runtime/builtin/hitl/config.gointernal/runtime/builtin/gha/config.gointernal/runtime/builtin/jq/config.gointernal/core/executor_schema_test.gointernal/runtime/builtin/docker/config.gointernal/runtime/builtin/http/config.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/builtin/docker/config_test.gointernal/core/executor_schema_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/runtime/builtin/docker/config_test.gointernal/core/executor_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: Applies to **/*.go : Repository linting relies on `golangci-lint`; prefer idiomatic Go patterns, minimal global state, and structured logging helpers in `internal/common`
Applied to files:
go.mod
📚 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/executor_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: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/core/executor_schema_test.go
🧬 Code graph analysis (10)
internal/runtime/builtin/docker/config_test.go (2)
internal/runtime/subcmd.go (1)
Run(258-280)internal/core/executor_schema.go (1)
ValidateExecutorConfig(38-57)
internal/runtime/builtin/archive/config.go (1)
internal/core/executor_schema.go (1)
RegisterExecutorConfigSchema(30-34)
internal/runtime/builtin/mail/config.go (1)
internal/core/executor_schema.go (1)
RegisterExecutorConfigSchema(30-34)
internal/runtime/builtin/ssh/config.go (1)
internal/core/executor_schema.go (1)
RegisterExecutorConfigSchema(30-34)
internal/core/spec/step.go (2)
internal/core/step.go (1)
ExecutorConfig(195-203)internal/core/executor_schema.go (1)
ValidateExecutorConfig(38-57)
internal/runtime/builtin/hitl/config.go (1)
internal/core/executor_schema.go (1)
RegisterExecutorConfigSchema(30-34)
internal/runtime/builtin/gha/config.go (2)
internal/common/logger/tag/tag.go (1)
Type(266-268)internal/core/executor_schema.go (1)
RegisterExecutorConfigSchema(30-34)
internal/core/executor_schema_test.go (1)
internal/core/executor_schema.go (2)
RegisterExecutorConfigSchema(30-34)ValidateExecutorConfig(38-57)
internal/runtime/builtin/docker/config.go (2)
internal/core/executor_schema.go (1)
RegisterExecutorConfigSchema(30-34)internal/common/logger/tag/tag.go (1)
Type(266-268)
internal/runtime/builtin/http/config.go (2)
internal/common/logger/tag/tag.go (1)
Type(266-268)internal/core/executor_schema.go (1)
RegisterExecutorConfigSchema(30-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Go Linter
- GitHub Check: Test on ubuntu-latest
🔇 Additional comments (26)
internal/runtime/builtin/hitl/config.go (1)
28-39: LGTM! Clean schema registration following the established pattern.The HITL executor schema is correctly defined and registered. The schema fields align with the
Configstruct and follow the same pattern used by other executors in this PR.As per coding guidelines, this follows idiomatic Go patterns with proper initialization and integration with the centralized schema registry.
internal/runtime/builtin/jq/config.go (1)
1-17: LGTM! Minimal and clean jq executor schema.The schema is well-defined with clear description for the
rawfield and follows the established registration pattern.As per coding guidelines, this uses idiomatic Go with proper package initialization.
internal/runtime/builtin/http/config.go (1)
8-32: LGTM! Comprehensive HTTP executor schema with appropriate field types.The schema correctly defines all HTTP executor configuration options with:
- Proper type definitions (integer, boolean, string, object)
- Clear descriptions for each field
- Correct use of
AdditionalPropertiesforheadersandqueryobjects to allow arbitrary string key-value pairsAs per coding guidelines, this follows idiomatic Go patterns and integrates properly with the centralized schema registry.
schemas/dag.schema.json (1)
1742-1742: No existing archive executor usage found in the codebase to verify breaking changes against.No YAML configurations using the archive executor were found in the repository despite searching example files, test configurations, and all documentation. While making
sourcerequired is technically a breaking change, there are no in-repo configurations that would be affected. If external DAG configurations using the archive executor without source exist, consider documenting this as a breaking change in release notes or migration guides.go.mod (1)
26-26: Dependency version is valid and secure.The jsonschema-go v0.4.2 release exists (published Dec 19, 2025) and has no known security advisories.
internal/core/spec/step.go (1)
327-333: LGTM! Well-integrated schema validation.The executor config validation is correctly placed after all transformations complete and the executor type is determined. The length check prevents unnecessary validation calls for empty configs (initialized on line 259), and error handling is consistent with other validators in the pipeline.
internal/runtime/builtin/docker/config.go (2)
282-285: Good backward compatibility support.Registering both "docker" and "container" executor types to the same schema ensures backward compatibility while maintaining consistent validation.
287-319: Schema validation complements existing validation logic.The JSON schema provides declarative validation for structure and basic constraints. The existing validation in
LoadConfigFromMap(lines 97-126) handles more complex runtime logic. This is a reasonable separation of concerns.Note: The schema validates structure (required fields, types, basic constraints) while
LoadConfigFromMaphandles mode-specific logic (e.g., exec-only-with-containerName on line 124-126). This layered approach is appropriate.internal/runtime/builtin/ssh/config.go (1)
98-116: LGTM! Schema aligns with Config struct.The schema definition correctly mirrors the SSH Config struct properties (lines 15-26) with appropriate types and helpful descriptions. The init() function properly registers the schema with the executor registry.
internal/runtime/builtin/archive/config.go (2)
89-109: Schema provides appropriate structural validation.The schema validates basic structure and constraints (required fields, types, minimum values), while
validateConfig(lines 51-87) handles operation-specific validation (e.g., destination required for create operations on line 64). This separation is appropriate.
111-115: Clean helper function and registration.The
ptrFloathelper clearly expresses intent for creating schema constraint pointers. The init() function properly registers the schema with the executor registry.internal/runtime/builtin/mail/config.go (1)
23-25: LGTM!The schema registration follows the established pattern and correctly integrates with the central registry.
internal/core/executor_schema_test.go (7)
11-22: LGTM!Clean test for valid configuration validation. Uses unique schema name and properly asserts no error.
24-37: LGTM!Good test validating type mismatch detection and error message quality.
39-43: LGTM!Essential backward compatibility test ensuring unregistered executors pass validation gracefully.
45-57: LGTM!Good coverage for empty config handling with optional fields.
59-77: LGTM!Solid concurrency test validating thread-safety of the registry. The 100-goroutine stress test is appropriate for detecting race conditions.
79-98: LGTM!Good coverage of enum validation for both valid and invalid values.
100-117: LGTM!Comprehensive test for required field enforcement, covering both missing and present cases.
internal/runtime/builtin/gha/config.go (3)
20-27: Verify ifportshould be an integer type.The
portfield is defined asType: "string", but ports are typically numeric. If the executor expects a number, consider usingType: "integer". If string is intentional (e.g., for port range syntax like"8080-8090"), consider adding aPatternconstraint for validation.
39-43: LGTM!Registering the schema under multiple aliases (
github_action,github-action,gha) is a good pattern for supporting different naming conventions while maintaining a single source of truth.
8-37: LGTM!Comprehensive schema definition covering all GHA executor configuration options with proper nested object structures for
artifactsandcapabilities.internal/core/executor_schema.go (4)
11-26: LGTM!Well-designed registry with appropriate concurrency primitives. The combination of
sync.RWMutexfor registry access andatomic.Pointerwithsync.Oncefor lazy schema resolution is idiomatic Go.
28-34: LGTM!Clean registration function. Since registration happens via
init()functions before any validation occurs, the simple replacement semantics are appropriate.
36-57: LGTM!Correct implementation with proper lock scoping. The read lock is released before calling
getResolved(), which has its own synchronization, avoiding potential lock ordering issues.
59-75: LGTM!The lazy resolution pattern using
sync.Oncecombined withatomic.Pointeris correctly implemented. Thesync.Onceensures thread-safe one-time initialization, and subsequent calls correctly return the cached result or error.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1558 +/- ##
==========================================
+ Coverage 63.70% 63.72% +0.02%
==========================================
Files 226 231 +5
Lines 25187 25238 +51
==========================================
+ Hits 16045 16084 +39
- Misses 7660 7665 +5
- Partials 1482 1489 +7
... and 3 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
Release Notes
New Features
Schema Updates
sourceproperty.✏️ Tip: You can customize this high-level summary in your review settings.