feat(spec): add build-time custom step types#1987
Merged
Conversation
# Conflicts: # internal/core/spec/builder.go # internal/core/spec/step.go
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces support for custom step type definitions in DAGs, enabling reusable templated step configurations. It extends the JSON schema to define Changes
Sequence DiagramsequenceDiagram
participant DAG as DAG Definition
participant Loader as Loader
participant Registry as Step Type Registry
participant Builder as Builder
participant Step as Built Step
participant Executor as Executor
DAG->>Loader: LoadYAML (with base config)
Loader->>Loader: Extract step_types from base + current
Loader->>Registry: Build custom step type registry
Loader->>Loader: Decode steps/handlers with raw payloads
Loader->>Builder: processDAGDocument (with ctx containing registry)
Builder->>Builder: For each step: decodeStep(raw)
Builder->>Registry: Resolve custom type (if type matches custom)
Registry->>Registry: Render template with inputs
Registry->>Registry: Merge defaults + template + call-site
Registry->>Builder: Return merged specification
Builder->>Builder: buildConcreteStep with merged spec
Builder->>Step: Create core.Step with custom_type metadata
Loader->>Executor: Return built DAG with custom steps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmn/schema/dag.schema.json`:
- Around line 4820-4826: customStepTypes currently allows arbitrary property
names, which can create definitions that cannot be referenced by step.type
(which enforces the regex ^[A-Za-z][A-Za-z0-9_-]*$); update the customStepTypes
definition to constrain keys by adding a propertyNames schema with the same
pattern (^[A-Za-z][A-Za-z0-9_-]*$) so keys match the step.type/executorType
naming rules and are referenceable.
- Around line 1139-1150: The new mutual-exclusion constraints for
"exec"/"command" and "exec"/"script" were added as a second "allOf" key which
overwrites the earlier one; instead merge these two "not": {"required": [...]}
entries into the existing "allOf" array so there is a single combined allOf
including both constraints. Locate the JSON object that already defines "allOf"
and append the two {"not": {"required":["exec","command"]}} and {"not":
{"required":["exec","script"]}} clauses to that same array (referencing the
"allOf", "exec", "command", and "script" symbols) so the exclusivity rules are
preserved. Ensure you remove the duplicate top-level "allOf" definition so the
schema has only one "allOf" array containing all constraints.
In `@internal/core/spec/defaults.go`:
- Around line 170-210: mergeDefaults currently skips override values when they
are zero-valued (e.g., TimeoutSec==0 or Agent.Prompt==""), so explicit clears in
override are lost; fix by making scalar/string fields that must support
explicit-zero semantics nullable (e.g., change TimeoutSec to *int, Agent.Prompt
to *string, or add explicit "set" booleans) and update mergeDefaults to copy
override fields when the override pointer/flag is present (override.TimeoutSec
!= nil -> merged.TimeoutSec = override.TimeoutSec, similarly use nil checks for
override.Agent and let mergeAgentDefaults treat nil vs present empty string
appropriately), and ensure Env/Preconditions logic also treats an
explicitly-present empty value as an override rather than skipping it.
In `@internal/core/spec/step_types.go`:
- Around line 108-128: validateCustomStepTypeSpec trims names but
registry.entries uses the raw map keys, causing unreachable entries and missed
duplicate detection; before calling validateCustomStepTypeSpec and before
checking/storing into registry.entries in the loops over base and local,
normalize the key (e.g., strings.TrimSpace and any other normalization used by
validateCustomStepTypeSpec/Lookup) into a variable like normalizedName, use
normalizedName for the duplicate-exists check and as the map key when assigning
registry.entries[normalizedName] = def, and continue to call
validateCustomStepTypeSpec with the original spec but use normalizedName for all
dedupe/storage/lookup logic so whitespace-only duplicates are rejected and
lookups work consistently.
In `@internal/intg/distr/step_types_test.go`:
- Around line 92-107: The test creates a fixture with newTestFixture(...) that
currently allows the worker to inherit the scheduler's base config, letting the
custom greet step type be resolved locally; update the fixture invocation to add
withWorkerBaseConfigPath("/nonexistent/base.yaml") so the worker cannot use a
local base config and the embedded base config propagation is exercised (i.e.,
modify the newTestFixture call in this test to include
withWorkerBaseConfigPath("/nonexistent/base.yaml") alongside
withLogPersistence(), withBaseConfigPath(...), and withLabels(...)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e8e9aec-37a2-474a-9645-074eb5d780b1
📒 Files selected for processing (14)
internal/cmn/schema/dag.schema.jsoninternal/core/spec/builder.gointernal/core/spec/dag.gointernal/core/spec/defaults.gointernal/core/spec/defaults_test.gointernal/core/spec/loader.gointernal/core/spec/step.gointernal/core/spec/step_types.gointernal/core/spec/step_types_feature_test.gointernal/intg/distr/step_types_test.gointernal/intg/step_types_test.gointernal/persis/filebaseconfig/default_base_config.gointernal/service/coordinator/handler.gointernal/test/helper.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
step_typesdefinitions so DAGs and base config can define reusable builtin-backed step templates with validated input schemasexecsupport for direct argv execution and preserve default precedence, handler overrides, and DAG-level executor inheritance for expanded custom stepsTest plan
go test ./internal/core/spec -count=1go test ./internal/intg -run 'TestCustomStepTypes' -count=1go test ./internal/intg/distr -run 'TestCustomStepTypes' -count=1Summary by CodeRabbit
Release Notes
execfield: Execute commands with explicit argument arrays, avoiding shell parsing overhead