-
Notifications
You must be signed in to change notification settings - Fork 36
Validate safe-outputs target field at compile time #11112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+970
−0
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
ed1c243
Initial plan
Copilot 3b116c2
Initial investigation complete - identify validation gap for safe-out…
Copilot 5125192
Add compile-time validation for safe-outputs target field
Copilot 16f6150
Clean up unused function and import in validation code
Copilot eded1a3
Address code review feedback - improve validation and error messages
Copilot 80b3318
Move isPositiveInteger to stringutil package
Copilot 04bc3e4
Merge main branch
Copilot 64f8453
Format code and complete merge of main branch
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,156 @@ | ||
| package workflow | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/githubnext/gh-aw/pkg/logger" | ||
| "github.com/githubnext/gh-aw/pkg/stringutil" | ||
| ) | ||
|
|
||
| var safeOutputsTargetValidationLog = logger.New("workflow:safe_outputs_target_validation") | ||
|
|
||
| // validateSafeOutputsTarget validates target fields in all safe-outputs configurations | ||
| // Valid target values: | ||
| // - "" (empty/default) - uses "triggering" behavior | ||
| // - "triggering" - targets the triggering issue/PR/discussion | ||
| // - "*" - targets any item specified in the output | ||
| // - A positive integer as a string (e.g., "123") | ||
| // - A GitHub Actions expression (e.g., "${{ github.event.issue.number }}") | ||
| func validateSafeOutputsTarget(config *SafeOutputsConfig) error { | ||
| if config == nil { | ||
| return nil | ||
| } | ||
|
|
||
| safeOutputsTargetValidationLog.Print("Validating safe-outputs target fields") | ||
|
|
||
| // List of configs to validate - each with a name for error messages | ||
| type targetConfig struct { | ||
| name string | ||
| target string | ||
| } | ||
|
|
||
| var configs []targetConfig | ||
|
|
||
| // Collect all target fields from various safe-output configurations | ||
| if config.UpdateIssues != nil { | ||
| configs = append(configs, targetConfig{"update-issue", config.UpdateIssues.Target}) | ||
| } | ||
| if config.UpdateDiscussions != nil { | ||
| configs = append(configs, targetConfig{"update-discussion", config.UpdateDiscussions.Target}) | ||
| } | ||
| if config.UpdatePullRequests != nil { | ||
| configs = append(configs, targetConfig{"update-pull-request", config.UpdatePullRequests.Target}) | ||
| } | ||
| if config.CloseIssues != nil { | ||
| configs = append(configs, targetConfig{"close-issue", config.CloseIssues.Target}) | ||
| } | ||
| if config.CloseDiscussions != nil { | ||
| configs = append(configs, targetConfig{"close-discussion", config.CloseDiscussions.Target}) | ||
| } | ||
| if config.ClosePullRequests != nil { | ||
| configs = append(configs, targetConfig{"close-pull-request", config.ClosePullRequests.Target}) | ||
| } | ||
| if config.AddLabels != nil { | ||
| configs = append(configs, targetConfig{"add-labels", config.AddLabels.Target}) | ||
| } | ||
| if config.RemoveLabels != nil { | ||
| configs = append(configs, targetConfig{"remove-labels", config.RemoveLabels.Target}) | ||
| } | ||
| if config.AddReviewer != nil { | ||
| configs = append(configs, targetConfig{"add-reviewer", config.AddReviewer.Target}) | ||
| } | ||
| if config.AssignMilestone != nil { | ||
| configs = append(configs, targetConfig{"assign-milestone", config.AssignMilestone.Target}) | ||
| } | ||
| if config.AssignToAgent != nil { | ||
| configs = append(configs, targetConfig{"assign-to-agent", config.AssignToAgent.Target}) | ||
| } | ||
| if config.AssignToUser != nil { | ||
| configs = append(configs, targetConfig{"assign-to-user", config.AssignToUser.Target}) | ||
| } | ||
| if config.LinkSubIssue != nil { | ||
| configs = append(configs, targetConfig{"link-sub-issue", config.LinkSubIssue.Target}) | ||
| } | ||
| if config.HideComment != nil { | ||
| configs = append(configs, targetConfig{"hide-comment", config.HideComment.Target}) | ||
| } | ||
| if config.MarkPullRequestAsReadyForReview != nil { | ||
| configs = append(configs, targetConfig{"mark-pull-request-as-ready-for-review", config.MarkPullRequestAsReadyForReview.Target}) | ||
| } | ||
| if config.AddComments != nil { | ||
| configs = append(configs, targetConfig{"add-comment", config.AddComments.Target}) | ||
| } | ||
| if config.CreatePullRequestReviewComments != nil { | ||
| configs = append(configs, targetConfig{"create-pull-request-review-comment", config.CreatePullRequestReviewComments.Target}) | ||
| } | ||
| if config.PushToPullRequestBranch != nil { | ||
| configs = append(configs, targetConfig{"push-to-pull-request-branch", config.PushToPullRequestBranch.Target}) | ||
| } | ||
|
|
||
| // Validate each target field | ||
| for _, cfg := range configs { | ||
| if err := validateTargetValue(cfg.name, cfg.target); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| safeOutputsTargetValidationLog.Printf("Validated %d target fields", len(configs)) | ||
| return nil | ||
| } | ||
|
|
||
| // validateTargetValue validates a single target value | ||
| func validateTargetValue(configName, target string) error { | ||
| // Empty or "triggering" are always valid | ||
| if target == "" || target == "triggering" { | ||
| return nil | ||
| } | ||
|
|
||
| // "*" is valid (any item) | ||
| if target == "*" { | ||
| return nil | ||
| } | ||
|
|
||
| // Check if it's a GitHub Actions expression | ||
| if isGitHubExpression(target) { | ||
| safeOutputsTargetValidationLog.Printf("Target for %s is a GitHub Actions expression", configName) | ||
| return nil | ||
| } | ||
|
|
||
| // Check if it's a positive integer | ||
| if stringutil.IsPositiveInteger(target) { | ||
| safeOutputsTargetValidationLog.Printf("Target for %s is a valid number: %s", configName, target) | ||
| return nil | ||
| } | ||
|
|
||
| // Build a helpful suggestion based on the invalid value | ||
| suggestion := "" | ||
| if target == "event" || strings.Contains(target, "github.event") { | ||
| suggestion = "\n\nDid you mean to use \"${{ github.event.issue.number }}\" instead of \"" + target + "\"?" | ||
| } | ||
|
|
||
| // Invalid target value | ||
| return fmt.Errorf( | ||
| "invalid target value for %s: %q\n\nValid target values are:\n - \"triggering\" (default) - targets the triggering issue/PR/discussion\n - \"*\" - targets any item specified in the output\n - A positive integer (e.g., \"123\")\n - A GitHub Actions expression (e.g., \"${{ github.event.issue.number }}\")%s", | ||
| configName, | ||
| target, | ||
| suggestion, | ||
| ) | ||
| } | ||
|
|
||
| // isGitHubExpression checks if a string is a valid GitHub Actions expression | ||
| // A valid expression must have properly balanced ${{ and }} markers | ||
| func isGitHubExpression(s string) bool { | ||
| // Must contain both opening and closing markers | ||
| if !strings.Contains(s, "${{") || !strings.Contains(s, "}}") { | ||
| return false | ||
| } | ||
|
|
||
| // Basic validation: opening marker must come before closing marker | ||
| openIndex := strings.Index(s, "${{") | ||
| closeIndex := strings.Index(s, "}}") | ||
|
|
||
| // The closing marker must come after the opening marker | ||
| // and there must be something between them | ||
| return openIndex >= 0 && closeIndex > openIndex+3 | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot use expression parser to validate correct expression, there should be a validator already