Remove timeout_minutes from schema and add labels validation #14860
Conversation
- Removed timeout_minutes field completely from main_workflow_schema.json - Added validateLabels() function to validate labels in frontmatter - Added labels validation call in validateWorkflowData() - Updated tests to reflect timeout_minutes removal - Added comprehensive tests for labels validation Co-authored-by: pelikhan <[email protected]>
- Use require.Error for error assertions in tests - Reformat schema JSON with prettier - All tests pass and linter is happy Co-authored-by: pelikhan <[email protected]>
- Remove redundant whitespace-only check (unreachable code) - Remove noisy log messages for normal code paths - Simplify validation logic per code review suggestions Co-authored-by: pelikhan <[email protected]>
|
🎯 Smoke Test: Copilot - §21888809314 Test Results
Overall: PASS ✅ cc:
|
There was a problem hiding this comment.
Pull request overview
This PR removes the underscored timeout_minutes field from the main workflow frontmatter schema to eliminate ambiguity with timeout-minutes, and adds compile-time validation for workflow labels to reject empty entries and leading/trailing whitespace that JSON Schema cannot enforce.
Changes:
- Removed
timeout_minutesfrompkg/parser/schemas/main_workflow_schema.jsonand updated deprecated-field tests to reflect full removal. - Added
validateLabels()and wired it into the compiler validation pipeline. - Added unit tests covering label validation edge cases.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/schemas/main_workflow_schema.json | Removes timeout_minutes from schema to enforce timeout-minutes only. |
| pkg/parser/schema_deprecated_test.go | Updates deprecated-field expectations to ensure timeout_minutes is no longer treated as deprecated. |
| pkg/workflow/strict_mode_deprecated_test.go | Updates integration tests to expect schema “unknown property” behavior for removed field. |
| pkg/workflow/labels_validation.go | Introduces validateLabels() implementation. |
| pkg/workflow/labels_validation_test.go | Adds unit tests for label validation behavior. |
| pkg/workflow/compiler.go | Hooks label validation into validateWorkflowData(). |
Comments suppressed due to low confidence (1)
pkg/workflow/labels_validation.go:21
- validateLabels() returns early when ParsedFrontmatter is nil. Since ParsedFrontmatter parsing is non-fatal and can fail for otherwise-valid frontmatter shapes (notably
engineobject format), invalidlabelsvalues may bypass validation entirely. It’d be more robust to extract/validate labels directly from raw frontmatter (or from FrontmatterYAML) as a fallback when ParsedFrontmatter is unavailable.
func validateLabels(workflowData *WorkflowData) error {
// If ParsedFrontmatter is nil or Labels is empty, nothing to validate
if workflowData == nil || workflowData.ParsedFrontmatter == nil {
return nil
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // validateLabels validates the labels field in the workflow frontmatter. | ||
| // It checks that: | ||
| // 1. Labels is an array (if present) | ||
| // 2. Each label is a non-empty string | ||
| // 3. Labels don't have leading or trailing whitespace |
There was a problem hiding this comment.
The doc comment says validateLabels checks that "Labels is an array", but the function only ever sees a typed []string (and skips validation when ParsedFrontmatter is nil). Consider updating the comment to describe the actual preconditions (ParsedFrontmatter successfully parsed) or adjusting the implementation to validate the raw frontmatter value/type.
This issue also appears on line 17 of the same file.
| // validateLabels validates the labels field in the workflow frontmatter. | |
| // It checks that: | |
| // 1. Labels is an array (if present) | |
| // 2. Each label is a non-empty string | |
| // 3. Labels don't have leading or trailing whitespace | |
| // validateLabels validates the labels field parsed from the workflow frontmatter. | |
| // It assumes that ParsedFrontmatter has been successfully populated and that | |
| // Labels is already a []string. It checks that: | |
| // 1. Each label is a non-empty string | |
| // 2. Labels don't have leading or trailing whitespace |
| func TestValidateLabels_NilFrontmatter(t *testing.T) { | ||
| // Test with nil ParsedFrontmatter | ||
| workflowData := &WorkflowData{ | ||
| ParsedFrontmatter: nil, | ||
| } | ||
|
|
||
| err := validateLabels(workflowData) | ||
| assert.NoError(t, err, "Should handle nil frontmatter gracefully") | ||
| } |
There was a problem hiding this comment.
This test asserts that nil ParsedFrontmatter should always pass validation. Given ParsedFrontmatter can be nil for valid workflows (e.g., engine object form) this effectively locks in a behavior where label validation is skipped in those cases. If the intent is to enforce label trimming consistently, adjust this test (and implementation) to validate labels via a raw-frontmatter/FrontmatterYAML fallback instead of treating nil ParsedFrontmatter as "nothing to validate".
| // Validate labels configuration | ||
| log.Printf("Validating labels") | ||
| if err := validateLabels(workflowData); err != nil { | ||
| return formatCompilerError(markdownPath, "error", err.Error(), err) | ||
| } |
There was a problem hiding this comment.
validateLabels() relies on workflowData.ParsedFrontmatter, but ParsedFrontmatter is best-effort and is explicitly set to nil when ParseFrontmatterConfig fails (e.g., when engine uses the object form engine: { id: ... }). That means this new validation will silently no-op for many valid workflows. Consider validating labels from raw frontmatter (e.g., parse workflowData.FrontmatterYAML / keep the original frontmatter map on WorkflowData), or make ParseFrontmatterConfig tolerate the engine object form so ParsedFrontmatter is still populated.
- Document enhanced compile error messages (#14901) - Document sandbox.agent: false option (#14888) - Document breaking change: timeout_minutes removed (#14860) - Update migration guide with breaking changes - Update codemod documentation Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Resolves schema ambiguity where both
timeout_minutesandtimeout-minuteswere accepted, and adds runtime validation for labels that the schema alone couldn't enforce.Changes
Schema cleanup:
timeout_minutesfield entirely frommain_workflow_schema.jsonLabels validation:
validateLabels()to compiler pipeline checking for:ParsedFrontmatterparses successfully (may be skipped for simple workflows withon: pushdue to type mismatch, but schema already enforcesminLength: 1)Example
Note: Breaking change for workflows using
timeout_minutes- they must migrate totimeout-minutes.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.