Raise error-message compliance for workflow validation batch (5 files) - #52174
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates workflow validation errors to provide expected inputs and corrective examples.
Changes:
- Expands validation error guidance across five production files.
- Introduces structured pricing validation errors.
- Adds or strengthens error-message tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/validation_helpers.go |
Expands shared validator errors. |
pkg/workflow/validation_helpers_test.go |
Checks range-error examples. |
pkg/workflow/utc_offset.go |
Unifies UTC offset guidance. |
pkg/workflow/utc_offset_test.go |
Tests malformed-offset messages. |
pkg/workflow/templatables.go |
Improves scalar coercion errors. |
pkg/workflow/templatables_bool_or_int_test.go |
Checks corrective examples. |
pkg/workflow/safe_outputs_validation.go |
Expands policy validation guidance. |
pkg/workflow/safe_outputs_validation_merge_pull_request_test.go |
Tests merge-policy examples. |
pkg/workflow/safe_outputs_urls_validation_test.go |
Tests URL-policy examples. |
pkg/workflow/safe_outputs_target_validation_test.go |
Tests target examples. |
pkg/workflow/model_costs_pricing_validation.go |
Adds structured pricing errors. |
pkg/workflow/model_costs_pricing_validation_test.go |
Checks pricing examples. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (3)
pkg/workflow/validation_helpers.go:154
- This revised error breaks the exact assertion in
validation_helpers_test.go:704, which still expects the old message ending atmount kind 2. Update that assertion for the new contract somake test-unitcan pass.
return fmt.Errorf("internal error: onInvalid callback returned nil for mount kind %d. Expected a non-nil error for invalid mount kinds. Example: return an error like \"safe-outputs.mounts[0] has an invalid entry\" when kind is invalid", kind)
pkg/workflow/templatables.go:79
- The non-expression-string path has the same hard-coded
timeout-minutesguidance even when this type is decodingmax-ai-credits,max-daily-ai-credits,max-tool-denials,timeout, orstartup-timeout. Use the actual consuming field or a field-neutral example.
return fmt.Errorf("timeout-minutes must be an integer or a GitHub Actions expression, got string %q. Expected an integer literal or an expression string. Example: timeout-minutes: 30 or timeout-minutes: ${{ inputs.timeout }}", s)
pkg/workflow/model_costs_pricing_validation.go:68
- This example omits required
inputandoutputrates, so copying it produces a different validation error beforecache_writeis checked. Include both required positive rates to make the suggested YAML valid.
"Set cache_write to a positive value when you configure it.\n\nExample:\nmodels:\n default-ai-credits-pricing:\n cache_write: 0.000001",
- Files reviewed: 12/12 changed files
- Comments generated: 5
- Review effort level: Balanced
| func validateMountEntries(mounts []string, onValid func(int, mountParts), onInvalid func(int, string, mountParts, mountValidationKind) error) error { | ||
| if onInvalid == nil { | ||
| return errors.New("internal error: onInvalid callback must not be nil") | ||
| return errors.New("internal error: onInvalid callback must not be nil. Expected a callback that returns an error for each invalid mount entry. Example: provide an onInvalid callback that returns an error for non-OK mount kinds") |
| var templatablesLog = logger.New("workflow:templatables") | ||
|
|
||
| const templatableBoolErrorExample = "value must be a boolean or a GitHub Actions expression (e.g. '${{ inputs.flag }}')" | ||
| const templatableBoolErrorExample = "value must be a boolean or a GitHub Actions expression. Expected true, false, or an expression string. Example: enabled: true or enabled: ${{ inputs.flag }}" |
| if err := json.Unmarshal(data, &s); err != nil { | ||
| templatablesLog.Printf("TemplatableInt32 rejected: not number or string: %s", data) | ||
| return fmt.Errorf("timeout-minutes must be an integer or a GitHub Actions expression (e.g. '${{ inputs.timeout }}'), got %s", data) | ||
| return fmt.Errorf("timeout-minutes must be an integer or a GitHub Actions expression, got %s. Expected an integer literal or an expression string. Example: timeout-minutes: 30 or timeout-minutes: ${{ inputs.timeout }}", data) |
| "models.default-ai-credits-pricing.cache_read", | ||
| fmt.Sprintf("%g", *p.CachedInput), | ||
| fmt.Sprintf("cache_read must be a positive value when set, got %g. Expected a value greater than 0.", *p.CachedInput), | ||
| "Set cache_read to a positive value when you configure it.\n\nExample:\nmodels:\n default-ai-credits-pricing:\n cache_read: 0.000001", |
| if value < min || value > max { | ||
| return fmt.Errorf("%s must be between %d and %d, got %d", | ||
| fieldName, min, max, value) | ||
| return fmt.Errorf("%s must be between %d and %d, got %d. Expected an integer in this inclusive range. Example: %s: %d", |
…bles, complete pricing examples Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all 5 review threads in 20ce11b:
All targeted tests and |
🤖 Triage: PR #52174
|
|
@copilot please fix the failed check below and run the pr-finisher skill. Failed checks: Latest maintainer request: Please refresh the branch context if needed, then hand back once lint is green.
|
The lint failure was from commit 20ce11b, before this branch was merged with main. The merge commit 0c71805 already brought in main's fix for the ST1005 violations in call_workflow_validation.go. Verified locally with |
|
Hey 👋 — this PR looks great! It's a well-focused batch addressing error-message compliance across 5 validation files, exactly as scoped in issue #52171. The changes are surgical (error message text only, no logic changes), tests are comprehensive (23 new test cases in The PR follows the core team agentic workflow process and aligns with the squad plan. Ready for review! ✨
|
This PR updates a targeted batch of workflow validation errors to match the repository’s error-message standard: clear failure, expected input, and a concrete correction example. Scope is limited to the five files identified by the lint-error-messages run, with no validation flow/control changes.
validation_helpers.go(shared validators)utc_offset.go(UTC parsing/normalization)utc: "-08:00").templatables.go(templatable scalar coercion)safe_outputs_validation.go(safe-outputs policy validation)safe-outputs.urlsandtargeterrors to enumerate accepted values plus valid YAML examples.targetexamples use valid handler keys in YAML snippets.model_costs_pricing_validation.go(default AI credits pricing)NewValidationError(...)with structured reason + suggestion.input,output,cache_read, andcache_write.Example of the new pattern applied: