fix: harden DAG param validation and artifact inference#2049
Merged
Conversation
|
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 pull request introduces artifact auto-enablement by detecting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/core/spec/dag_params_inline_schema_test.go (1)
75-90: Add path assertion parity in the skip-validation variant.To keep the contract equally strict, also assert that the error is attributed to
params(like the non-skip variant).Suggested test tweak
_, err := LoadYAML(context.Background(), yaml, SkipSchemaValidation(), WithoutEval()) require.Error(t, err) + assert.Contains(t, err.Error(), "params") assert.Contains(t, err.Error(), "properties must be an object")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/spec/dag_params_inline_schema_test.go` around lines 75 - 90, The test TestBuildInlineSchemaParamPlan_SkipSchemaValidationStillRejectsPropertiesArrayShape currently only asserts the error message; update it to also assert the error path matches the params field like the non-skip variant by inspecting the returned error from LoadYAML (called with SkipSchemaValidation() and WithoutEval()) and asserting the error's path/field is "params" (or contains "params") so the test enforces the same path attribution as the non-skip case.
🤖 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/core/spec/dag.go`:
- Around line 870-872: The current reflect.String case uses a raw substring
check against dagRunArtifactsDirEnvKey which is too permissive; update the
reflect.String branch (the switch handling value kinds) to only treat strings as
env-var references when they match actual env-var reference forms (e.g.
$DAG_RUN_ARTIFACTS_DIR, ${DAG_RUN_ARTIFACTS_DIR}, or
env("DAG_RUN_ARTIFACTS_DIR")), not any string containing the token; implement
this by replacing strings.Contains(v.String(), dagRunArtifactsDirEnvKey) with a
pattern check (simple regex or explicit prefix/suffix checks) that verifies the
exact reference forms and use dagRunArtifactsDirEnvKey in the pattern. Ensure
other branches (Array/Slice) keep existing behavior.
---
Nitpick comments:
In `@internal/core/spec/dag_params_inline_schema_test.go`:
- Around line 75-90: The test
TestBuildInlineSchemaParamPlan_SkipSchemaValidationStillRejectsPropertiesArrayShape
currently only asserts the error message; update it to also assert the error
path matches the params field like the non-skip variant by inspecting the
returned error from LoadYAML (called with SkipSchemaValidation() and
WithoutEval()) and asserting the error's path/field is "params" (or contains
"params") so the test enforces the same path attribution as the non-skip case.
🪄 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: 865ac54a-1981-41cc-ba5a-5867bdd2cbcf
📒 Files selected for processing (10)
internal/cmn/schema/dag.schema.jsoninternal/cmn/schema/dag_schema_test.gointernal/core/spec/dag.gointernal/core/spec/dag_params.gointernal/core/spec/dag_params_inline_schema.gointernal/core/spec/dag_params_inline_schema_test.gointernal/core/spec/dag_test.gointernal/intg/template_test.gointernal/runtime/eval.gointernal/runtime/eval_test.go
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/core/spec/dag.go (1)
882-904: Add adefaultbranch for code clarity, though not currently required by exhaustive lint.The switch at line 882 omits a
defaultcase, and while the exhaustive linter does not currently flag this (despite covering only 5 of 27 possiblereflect.Kindvalues), addingdefault: return falsewould make the intent explicit that other kinds are unhandled.Suggested fix
switch v.Kind() { case reflect.String: return referencesArtifactsEnvVar(v.String()) case reflect.Array, reflect.Slice: for i := 0; i < v.Len(); i++ { if valueReferencesRunArtifactsDir(v.Index(i)) { return true } } case reflect.Map: iter := v.MapRange() for iter.Next() { if valueReferencesRunArtifactsDir(iter.Key()) || valueReferencesRunArtifactsDir(iter.Value()) { return true } } case reflect.Struct: for _, field := range v.Fields() { if valueReferencesRunArtifactsDir(field) { return true } } +default: + return false } return false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/spec/dag.go` around lines 882 - 904, The switch inside valueReferencesRunArtifactsDir currently handles String, Array/Slice, Map, and Struct kinds but lacks an explicit default; add a default case to the switch (in the valueReferencesRunArtifactsDir function) that returns false to make the intent explicit for all other reflect.Kind values and satisfy clarity/linting expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/core/spec/dag.go`:
- Around line 882-904: The switch inside valueReferencesRunArtifactsDir
currently handles String, Array/Slice, Map, and Struct kinds but lacks an
explicit default; add a default case to the switch (in the
valueReferencesRunArtifactsDir function) that returns false to make the intent
explicit for all other reflect.Kind values and satisfy clarity/linting
expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 665505d1-ed05-4a2c-afe4-6736a239fddd
📒 Files selected for processing (4)
internal/core/spec/dag.gointernal/core/spec/dag_params_inline_schema_test.gointernal/core/spec/dag_test.gointernal/runtime/eval.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/core/spec/dag_params_inline_schema_test.go
- internal/runtime/eval.go
- internal/core/spec/dag_test.go
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/spec/dag.go`:
- Around line 898-903: The code uses the non-existent method v.Fields() (inside
the reflect.Struct case) which causes a compile error; replace that iteration
with the standard reflect APIs by looping from i := 0; i < v.NumField(); i++ and
calling v.Field(i) for each field, then call valueReferencesRunArtifactsDir on
each retrieved field value (preserving the existing early-return behavior);
update the case block where v.Fields() is referenced to use NumField() and
Field(i).
In `@internal/core/spec/dparams_inline_schema.go`:
- Around line 18-64: The classifier fails to surface a present-but-nil
"properties" key (e.g., params: {type: "object", properties: null}) because
malformedProperties can be nil; update inlineJSONSchemaClassification to include
a propertiesPresent bool, set propertiesPresent=true when m["properties"] exists
(even if nil) inside classifyInlineJSONSchema, and then change
malformedInlineJSONSchemaShapeError to return a validation error when
classification.propertiesPresent is true but classification.valid is false (or
when malformedProperties is non-nil), ensuring present-but-malformed (including
null) properties produce the error; reference inlineJSONSchemaClassification,
classifyInlineJSONSchema, and malformedInlineJSONSchemaShapeError.
🪄 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: 6182a8a6-fb1e-4457-99ef-15e56741a13e
📒 Files selected for processing (16)
internal/cmn/schema/dag.schema.jsoninternal/cmn/schema/dag_schema_test.gointernal/core/spec/dag.gointernal/core/spec/dag_test.gointernal/core/spec/dparams.gointernal/core/spec/dparams_external_schema.gointernal/core/spec/dparams_inline.gointernal/core/spec/dparams_inline_schema.gointernal/core/spec/dparams_inline_schema_test.gointernal/core/spec/dparams_runtime.gointernal/core/spec/dparams_test.gointernal/intg/template_test.gointernal/runtime/eval.gointernal/runtime/eval_test.gointernal/runtime/node.gointernal/runtime/node_internal_test.go
Comment on lines
+898
to
+903
| case reflect.Struct: | ||
| for _, field := range v.Fields() { | ||
| if valueReferencesRunArtifactsDir(field) { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -type f -name "dag.go" -path "*/internal/core/spec/*" 2>/dev/null | head -5Repository: dagucloud/dagu
Length of output: 40
🏁 Script executed:
# Let's first list the repository structure to locate the file
git ls-files | grep -E "dag\.go" | head -10Repository: dagucloud/dagu
Length of output: 211
🏁 Script executed:
# Check the specific file and lines mentioned
sed -n '890,910p' internal/core/spec/dag.goRepository: dagucloud/dagu
Length of output: 499
🏁 Script executed:
sed -n '870,920p' internal/core/spec/dag.goRepository: dagucloud/dagu
Length of output: 1137
🏁 Script executed:
# Check if there are any type aliases or method receivers that might add Fields() to reflect.Value
rg "func.*Fields" internal/core/spec/dag.goRepository: dagucloud/dagu
Length of output: 40
🏁 Script executed:
# Check the Go version and see if there's any build output showing compilation errors
cat go.mod | head -20Repository: dagucloud/dagu
Length of output: 655
🏁 Script executed:
# Try to find if Fields() is defined anywhere as a method on reflect.Value
rg "func \(.*reflect\.Value.*\).*Fields" --type goRepository: dagucloud/dagu
Length of output: 40
🏁 Script executed:
# Double-check Go reflect package documentation by looking at reflect usage elsewhere
rg "\.NumField\(\)" internal/core/spec/dag.go | head -5Repository: dagucloud/dagu
Length of output: 40
🏁 Script executed:
# Verify that this code path is actually being used and not a dead code path
rg "valueReferencesRunArtifactsDir" internal/core/spec/dag.goRepository: dagucloud/dagu
Length of output: 373
reflect.Value.Fields() does not exist in Go's standard library and will cause a compilation error.
Replace v.Fields() with the standard API using NumField() and Field(i):
Suggested fix
case reflect.Struct:
- for _, field := range v.Fields() {
- if valueReferencesRunArtifactsDir(field) {
- return true
- }
- }
+ for i := 0; i < v.NumField(); i++ {
+ if valueReferencesRunArtifactsDir(v.Field(i)) {
+ return true
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/spec/dag.go` around lines 898 - 903, The code uses the
non-existent method v.Fields() (inside the reflect.Struct case) which causes a
compile error; replace that iteration with the standard reflect APIs by looping
from i := 0; i < v.NumField(); i++ and calling v.Field(i) for each field, then
call valueReferencesRunArtifactsDir on each retrieved field value (preserving
the existing early-return behavior); update the case block where v.Fields() is
referenced to use NumField() and Field(i).
Comment on lines
+18
to
+64
| type inlineJSONSchemaClassification struct { | ||
| valid bool | ||
| malformedProperties any | ||
| } | ||
|
|
||
| // isInlineJSONSchema returns true when params is a top-level JSON Schema object | ||
| // using the canonical object form. This keeps legacy params maps compatible. | ||
| func isInlineJSONSchema(input any) bool { | ||
| return classifyInlineJSONSchema(input).valid | ||
| } | ||
|
|
||
| func classifyInlineJSONSchema(input any) inlineJSONSchemaClassification { | ||
| m, ok := input.(map[string]any) | ||
| if !ok { | ||
| return false | ||
| return inlineJSONSchemaClassification{} | ||
| } | ||
| // External schema format takes precedence. | ||
| if _, ok := extractParamsSchemaDeclaration(input); ok { | ||
| return false | ||
| return inlineJSONSchemaClassification{} | ||
| } | ||
| typeName, ok := m["type"].(string) | ||
| if !ok || strings.TrimSpace(typeName) != "object" { | ||
| return false | ||
| return inlineJSONSchemaClassification{} | ||
| } | ||
| props, ok := m["properties"] | ||
| if !ok { | ||
| return false | ||
| return inlineJSONSchemaClassification{} | ||
| } | ||
| _, ok = props.(map[string]any) | ||
| return ok | ||
|
|
||
| if _, ok := props.(map[string]any); ok { | ||
| return inlineJSONSchemaClassification{valid: true} | ||
| } | ||
|
|
||
| return inlineJSONSchemaClassification{malformedProperties: props} | ||
| } | ||
|
|
||
| func malformedInlineJSONSchemaShapeError(input any) error { | ||
| classification := classifyInlineJSONSchema(input) | ||
| if classification.malformedProperties == nil { | ||
| return nil | ||
| } | ||
|
|
||
| return core.NewValidationError( | ||
| "params", | ||
| classification.malformedProperties, | ||
| fmt.Errorf("inline JSON Schema properties must be an object keyed by parameter name"), | ||
| ) |
There was a problem hiding this comment.
Track malformed properties presence separately.
malformedInlineJSONSchemaShapeError only fires when malformedProperties is non-nil, so params: { type: object, properties: null } still falls through to legacy parsing when schema validation is skipped. That leaves a malformed inline-schema shape unblocked.
🛠️ Suggested fix
type inlineJSONSchemaClassification struct {
- valid bool
- malformedProperties any
+ valid bool
+ malformedProperties bool
+ malformedPropertiesValue any
}
@@
if _, ok := props.(map[string]any); ok {
return inlineJSONSchemaClassification{valid: true}
}
- return inlineJSONSchemaClassification{malformedProperties: props}
+ return inlineJSONSchemaClassification{
+ malformedProperties: true,
+ malformedPropertiesValue: props,
+ }
}
@@
- if classification.malformedProperties == nil {
+ if !classification.malformedProperties {
return nil
}
@@
- classification.malformedProperties,
+ classification.malformedPropertiesValue,
fmt.Errorf("inline JSON Schema properties must be an object keyed by parameter name"),
)
}📝 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.
Suggested change
| type inlineJSONSchemaClassification struct { | |
| valid bool | |
| malformedProperties any | |
| } | |
| // isInlineJSONSchema returns true when params is a top-level JSON Schema object | |
| // using the canonical object form. This keeps legacy params maps compatible. | |
| func isInlineJSONSchema(input any) bool { | |
| return classifyInlineJSONSchema(input).valid | |
| } | |
| func classifyInlineJSONSchema(input any) inlineJSONSchemaClassification { | |
| m, ok := input.(map[string]any) | |
| if !ok { | |
| return false | |
| return inlineJSONSchemaClassification{} | |
| } | |
| // External schema format takes precedence. | |
| if _, ok := extractParamsSchemaDeclaration(input); ok { | |
| return false | |
| return inlineJSONSchemaClassification{} | |
| } | |
| typeName, ok := m["type"].(string) | |
| if !ok || strings.TrimSpace(typeName) != "object" { | |
| return false | |
| return inlineJSONSchemaClassification{} | |
| } | |
| props, ok := m["properties"] | |
| if !ok { | |
| return false | |
| return inlineJSONSchemaClassification{} | |
| } | |
| _, ok = props.(map[string]any) | |
| return ok | |
| if _, ok := props.(map[string]any); ok { | |
| return inlineJSONSchemaClassification{valid: true} | |
| } | |
| return inlineJSONSchemaClassification{malformedProperties: props} | |
| } | |
| func malformedInlineJSONSchemaShapeError(input any) error { | |
| classification := classifyInlineJSONSchema(input) | |
| if classification.malformedProperties == nil { | |
| return nil | |
| } | |
| return core.NewValidationError( | |
| "params", | |
| classification.malformedProperties, | |
| fmt.Errorf("inline JSON Schema properties must be an object keyed by parameter name"), | |
| ) | |
| type inlineJSONSchemaClassification struct { | |
| valid bool | |
| malformedProperties bool | |
| malformedPropertiesValue any | |
| } | |
| // isInlineJSONSchema returns true when params is a top-level JSON Schema object | |
| // using the canonical object form. This keeps legacy params maps compatible. | |
| func isInlineJSONSchema(input any) bool { | |
| return classifyInlineJSONSchema(input).valid | |
| } | |
| func classifyInlineJSONSchema(input any) inlineJSONSchemaClassification { | |
| m, ok := input.(map[string]any) | |
| if !ok { | |
| return inlineJSONSchemaClassification{} | |
| } | |
| // External schema format takes precedence. | |
| if _, ok := extractParamsSchemaDeclaration(input); ok { | |
| return inlineJSONSchemaClassification{} | |
| } | |
| typeName, ok := m["type"].(string) | |
| if !ok || strings.TrimSpace(typeName) != "object" { | |
| return inlineJSONSchemaClassification{} | |
| } | |
| props, ok := m["properties"] | |
| if !ok { | |
| return inlineJSONSchemaClassification{} | |
| } | |
| if _, ok := props.(map[string]any); ok { | |
| return inlineJSONSchemaClassification{valid: true} | |
| } | |
| return inlineJSONSchemaClassification{ | |
| malformedProperties: true, | |
| malformedPropertiesValue: props, | |
| } | |
| } | |
| func malformedInlineJSONSchemaShapeError(input any) error { | |
| classification := classifyInlineJSONSchema(input) | |
| if !classification.malformedProperties { | |
| return nil | |
| } | |
| return core.NewValidationError( | |
| "params", | |
| classification.malformedPropertiesValue, | |
| fmt.Errorf("inline JSON Schema properties must be an object keyed by parameter name"), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/spec/dparams_inline_schema.go` around lines 18 - 64, The
classifier fails to surface a present-but-nil "properties" key (e.g., params:
{type: "object", properties: null}) because malformedProperties can be nil;
update inlineJSONSchemaClassification to include a propertiesPresent bool, set
propertiesPresent=true when m["properties"] exists (even if nil) inside
classifyInlineJSONSchema, and then change malformedInlineJSONSchemaShapeError to
return a validation error when classification.propertiesPresent is true but
classification.valid is false (or when malformedProperties is non-nil), ensuring
present-but-malformed (including null) properties produce the error; reference
inlineJSONSchemaClassification, classifyInlineJSONSchema, and
malformedInlineJSONSchemaShapeError.
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
DAG_RUN_ARTIFACTS_DIRTesting
Summary by CodeRabbit
New Features
Improvements