feat: add action schema v2#2122
Open
yottahmd wants to merge 21 commits into
Open
Conversation
# Conflicts: # internal/core/spec/step_types.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 a canonical v2 step execution model using ChangesV2 Execution Model Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 7
🧹 Nitpick comments (1)
internal/core/spec/builder.go (1)
356-371: ⚡ Quick win
normalizeStepExecutionRawis always called even when the input is already normalised.When
buildStepFromSpecis invoked frombuildStepFromRaw, therawargument is already the output ofnormalizeStepExecutionRaw. The function is called a second time here unconditionally, but its result is only consumed whenhasRun || hasActionis true. If normalisation removesrun/actionkeys (the expected behaviour), those keys will be absent, the inner block never executes, and the second call's result is silently discarded.Consider moving the call inside the guard to make the intent explicit and avoid redundant work on the hot step-building path:
♻️ Suggested restructure
- if raw != nil { - _, hasRun := raw["run"] - _, hasAction := raw["action"] - normalizedRaw, err := normalizeStepExecutionRaw(raw, ctx.customStepTypes) - if err != nil { - return nil, err - } - if hasRun || hasAction { - normalizedSpec, err := decodeStep(normalizedRaw) - if err != nil { - return nil, err - } - st = normalizedSpec - raw = normalizedRaw - } - } + if raw != nil { + _, hasRun := raw["run"] + _, hasAction := raw["action"] + if hasRun || hasAction { + normalizedRaw, err := normalizeStepExecutionRaw(raw, ctx.customStepTypes) + if err != nil { + return nil, err + } + normalizedSpec, err := decodeStep(normalizedRaw) + if err != nil { + return nil, err + } + st = normalizedSpec + raw = normalizedRaw + } + }Note: if
normalizeStepExecutionRawvalidates legacy fields even in the absence ofrun/actionand you rely on those errors being surfaced here, the unconditional call is intentional and the above refactor would suppress them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/spec/builder.go` around lines 356 - 371, The code unconditionally calls normalizeStepExecutionRaw inside buildStepFromSpec even though buildStepFromRaw already provided normalized input, causing wasted work and discarded results when run/action are absent; move the call to normalizeStepExecutionRaw so it only runs inside the hasRun || hasAction guard (i.e., call normalizeStepExecutionRaw just before decodeStep and assign normalizedRaw to raw only when hasRun || hasAction), and preserve existing error propagation from normalizeStepExecutionRaw; if you intentionally rely on its validation even when run/action are missing, add a comment explaining that and keep the unconditional call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmn/schema/dag.schema.json`:
- Around line 1919-1968: The schema for steps is too permissive compared to the
loader's normalizeStepExecutionRaw logic: update the allOf/if-then blocks so the
schema rejects the same invalid combinations and enforces the same
action-specific "with" shapes; specifically, mirror normalizeStepExecutionRaw by
adding constraints that disallow "exec" and "shell_args" when "run" is present
(use a "not": {"required": ["run","exec"]} / similar for "shell_args"), disallow
"command"/"call"/"script" when "action" is present, and add additional
action-specific if-then clauses (e.g., for "validate" and any other action types
handled in code) that require "with" and point to the correct action config defs
(in addition to the existing dagRunActionConfig and httpRequestActionConfig),
and ensure the "run" branch continues to reference runWithConfig; align property
names and required sets to match normalizeStepExecutionRaw exactly.
- Around line 5479-5499: The JSON schema for customActionDefinition does not
include output_schema even though validateCustomActionSpec and
buildCustomStepFromSpec accept and apply actions.*.output_schema; add an
"output_schema" property to the customActionDefinition properties (mirroring
input_schema) with type "object", additionalProperties: true and a descriptive
"description" so the schema permits actions.*.output_schema without breaking
validation; update the customActionDefinition properties block to include this
new symbol.
In `@internal/core/spec/deprecation.go`:
- Around line 47-64: The loop in DeprecatedSyntaxWarnings currently returns nil
on a non-EOF decode error, discarding warnings already collected; change the
error branch so that instead of returning nil you return the accumulated
warnings slice (i.e., return warnings) so partial warnings from earlier decoded
documents are preserved. Locate the decoder.Decode(&doc) error handling in the
DeprecatedSyntaxWarnings function and replace the "return nil" with "return
warnings" while leaving the EOF break behavior unchanged.
In `@internal/core/spec/step_types.go`:
- Around line 368-383: The validation currently only ensures exactly one of
spec.Template["run"] or spec.Template["action"] is present; extend the check in
the same block (after the run/action exclusivity check) to scan spec.Template
for legacy execution keys (e.g., "type", "command", "shell_args" and any other
deprecated execution keys) and if any are present return
core.NewValidationError(fmt.Sprintf("actions.%s.template", name), spec.Template,
fmt.Errorf("template contains deprecated execution keys: %v", invalidKeys)) so
the error surface lists the offending keys; keep the existing NewValidationError
usage and message style and ensure the scan references spec.Template and the
same name variable.
- Around line 925-928: The normalization call uses
normalizeStepExecutionRaw(mergedRaw, nil) which passes a nil custom-action
registry and causes rendered action references like customActionTemplate.action
-> "action: my.custom.action" to resolve as unknown; update the call to pass the
active custom-action registry (the same registry used to validate/build custom
types) instead of nil so normalizeStepExecutionRaw can resolve referenced custom
actions during decode/build (preserve existing error wrapping that uses
customType.Name).
In `@internal/runtime/builtin/http/http.go`:
- Around line 90-95: newHTTP currently falls back to reqCfg.Method and
reqCfg.URL but doesn't validate that method and url are non-empty; this can lead
to obscure runtime errors ("parse empty URL"/"invalid method"). After the
existing fallback (the block that sets method = reqCfg.Method and url =
reqCfg.URL), add a guard in newHTTP that checks if method == "" or url == "" and
return an explicit error (including which field is missing) (use the same
error-returning convention as newHTTP) so callers get a clear diagnostic instead
of a low-level resty/http error.
In `@README.md`:
- Around line 497-499: The table row is ambiguous because it lists "run" as if
it were an action name; update the table so it clearly distinguishes the YAML
field `run:` from action names like `docker.run` and `ssh.run` — for example,
change the first column text to "`run:` field" or "Field / Keyword" (instead of
"Action") and/or add a short parenthetical note in that row like "(YAML `run:`
field for shell steps)" so readers won't try `action: run`; ensure the change
updates the row containing `run` and keeps other rows referencing `docker.run`
and `ssh.run` unchanged.
---
Nitpick comments:
In `@internal/core/spec/builder.go`:
- Around line 356-371: The code unconditionally calls normalizeStepExecutionRaw
inside buildStepFromSpec even though buildStepFromRaw already provided
normalized input, causing wasted work and discarded results when run/action are
absent; move the call to normalizeStepExecutionRaw so it only runs inside the
hasRun || hasAction guard (i.e., call normalizeStepExecutionRaw just before
decodeStep and assign normalizedRaw to raw only when hasRun || hasAction), and
preserve existing error propagation from normalizeStepExecutionRaw; if you
intentionally rely on its validation even when run/action are missing, add a
comment explaining that and keep the unconditional call.
🪄 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: 1b679a32-c8cc-4814-a3aa-a648b00f7fdb
📒 Files selected for processing (19)
README.mdinternal/cmd/validate.gointernal/cmd/validate_test.gointernal/cmn/schema/dag.schema.jsoninternal/cmn/schema/dag_schema_test.gointernal/core/parallel_test.gointernal/core/spec/builder.gointernal/core/spec/dag.gointernal/core/spec/deprecation.gointernal/core/spec/loader.gointernal/core/spec/step.gointernal/core/spec/step_types.gointernal/core/spec/step_v2.gointernal/core/spec/step_v2_cleanup_test.gointernal/core/spec/step_v2_test.gointernal/core/validator.gointernal/core/validator_test.gointernal/runtime/builtin/http/config.gointernal/runtime/builtin/http/http.go
# Conflicts: # internal/core/spec/step_types.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
run/actionsyntax while keeping legacy syntax only in compatibility/deprecation testsTesting
go test ./internal/cmd ./internal/core/spec ./internal/cmn/schema ./internal/service/frontend/api/v1 ./internal/runtime/executor ./internal/persis/filedag ./internal/agent ./internal/intg/embed -count=1go test ./internal/intg/distr -run 'Test(CustomStepTypes_|.*SubDAG|Parallel|Lifecycle|Params|BaseConfig)' -count=1go test ./internal/intg -run 'TestRetryRestoresHarnessConfigFromBaseConfig|TestSSHExecutorIntegration/(ErrorHandling_InvalidWorkingDirectory|StepLevelSSHConfig)|TestTemplateExecutor/(SlimSprigOverlapBehavior|SlimSprigMissingKeyBoundary|DataFromPriorStep|RelativeOutputPath|ArtifactOutputAutoEnablesArtifacts)|TestCommandExecution_DollarEscape/ScriptWithoutShell_Direct|TestDAGExecution/(PerlScript|SpecialVars|CommandErrorIncludesLastStderrLine)' -count=1 -vgo test ./internal/intg -count=1 -jsongo test ./internal/... -count=1pnpm test -- src/features/dags/components/dag-editor/__tests__/customStepSchema.test.ts src/features/dags/components/dag-details/__tests__/DAGStepTableRow.test.tsx src/features/dags/components/dag-details/__tests__/HarnessStepSummary.test.tsx src/features/dags/components/visualization/__tests__/TimelineChart.test.tsx src/features/dags/components/visualization/__tests__/timelineItems.test.tsSummary by CodeRabbit
Release Notes
New Features
runandactionfields with full backward compatibility.Documentation
Bug Fixes