From ed1c243afc1c391acd13cbbd6f16e9c1adc5a91f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 21 Jan 2026 22:16:52 +0000 Subject: [PATCH 1/7] Initial plan From 3b116c260f651fc1100f8383f28e9f4e5d429b9c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 21 Jan 2026 22:21:53 +0000 Subject: [PATCH 2/7] Initial investigation complete - identify validation gap for safe-outputs target field Co-authored-by: eaftan <4733401+eaftan@users.noreply.github.com> --- .github/workflows/poem-bot.lock.yml | 35 +++++++++++++++++-- .github/workflows/sub-issue-closer.lock.yml | 35 +++++++++++++++++-- .github/workflows/workflow-generator.lock.yml | 35 +++++++++++++++++-- .../workflow-health-manager.lock.yml | 35 +++++++++++++++++-- 4 files changed, 132 insertions(+), 8 deletions(-) diff --git a/.github/workflows/poem-bot.lock.yml b/.github/workflows/poem-bot.lock.yml index eab3364ab55..b9177c59a3b 100644 --- a/.github/workflows/poem-bot.lock.yml +++ b/.github/workflows/poem-bot.lock.yml @@ -437,12 +437,19 @@ jobs: "name": "add_labels" }, { - "description": "Update an existing GitHub issue's status, title, or body. Use this to modify issue properties after creation. Only the fields you specify will be updated; other fields remain unchanged. CONSTRAINTS: Maximum 2 issue(s) can be updated. Target: *.", + "description": "Update an existing GitHub issue's status, title, labels, assignees, milestone, or body. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. CONSTRAINTS: Maximum 2 issue(s) can be updated. Target: *.", "inputSchema": { "additionalProperties": false, "properties": { + "assignees": { + "description": "Replace the issue assignees with this list of GitHub usernames (e.g., ['octocat', 'mona']).", + "items": { + "type": "string" + }, + "type": "array" + }, "body": { - "description": "New issue body to replace the existing content. Use Markdown formatting.", + "description": "Issue body content in Markdown. For 'replace', this becomes the entire body. For 'append'/'prepend', this content is added with a separator and an attribution footer. For 'replace-island', only the run-specific section is updated.", "type": "string" }, "issue_number": { @@ -452,6 +459,30 @@ jobs: "string" ] }, + "labels": { + "description": "Replace the issue labels with this list (e.g., ['bug', 'campaign:foo']). Labels must exist in the repository.", + "items": { + "type": "string" + }, + "type": "array" + }, + "milestone": { + "description": "Milestone number to assign (e.g., 1). Use null to clear.", + "type": [ + "number", + "string" + ] + }, + "operation": { + "description": "How to update the issue body: 'append' (default - add to end with separator), 'prepend' (add to start with separator), 'replace' (overwrite entire body), or 'replace-island' (update a run-specific section).", + "enum": [ + "replace", + "append", + "prepend", + "replace-island" + ], + "type": "string" + }, "status": { "description": "New issue status: 'open' to reopen a closed issue, 'closed' to close an open issue.", "enum": [ diff --git a/.github/workflows/sub-issue-closer.lock.yml b/.github/workflows/sub-issue-closer.lock.yml index 64eaa1fb68f..9146aa528c5 100644 --- a/.github/workflows/sub-issue-closer.lock.yml +++ b/.github/workflows/sub-issue-closer.lock.yml @@ -194,12 +194,19 @@ jobs: "name": "add_comment" }, { - "description": "Update an existing GitHub issue's status, title, or body. Use this to modify issue properties after creation. Only the fields you specify will be updated; other fields remain unchanged. CONSTRAINTS: Maximum 20 issue(s) can be updated. Target: *.", + "description": "Update an existing GitHub issue's status, title, labels, assignees, milestone, or body. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. CONSTRAINTS: Maximum 20 issue(s) can be updated. Target: *.", "inputSchema": { "additionalProperties": false, "properties": { + "assignees": { + "description": "Replace the issue assignees with this list of GitHub usernames (e.g., ['octocat', 'mona']).", + "items": { + "type": "string" + }, + "type": "array" + }, "body": { - "description": "New issue body to replace the existing content. Use Markdown formatting.", + "description": "Issue body content in Markdown. For 'replace', this becomes the entire body. For 'append'/'prepend', this content is added with a separator and an attribution footer. For 'replace-island', only the run-specific section is updated.", "type": "string" }, "issue_number": { @@ -209,6 +216,30 @@ jobs: "string" ] }, + "labels": { + "description": "Replace the issue labels with this list (e.g., ['bug', 'campaign:foo']). Labels must exist in the repository.", + "items": { + "type": "string" + }, + "type": "array" + }, + "milestone": { + "description": "Milestone number to assign (e.g., 1). Use null to clear.", + "type": [ + "number", + "string" + ] + }, + "operation": { + "description": "How to update the issue body: 'append' (default - add to end with separator), 'prepend' (add to start with separator), 'replace' (overwrite entire body), or 'replace-island' (update a run-specific section).", + "enum": [ + "replace", + "append", + "prepend", + "replace-island" + ], + "type": "string" + }, "status": { "description": "New issue status: 'open' to reopen a closed issue, 'closed' to close an open issue.", "enum": [ diff --git a/.github/workflows/workflow-generator.lock.yml b/.github/workflows/workflow-generator.lock.yml index 70bed8c56fe..1b84af8606b 100644 --- a/.github/workflows/workflow-generator.lock.yml +++ b/.github/workflows/workflow-generator.lock.yml @@ -230,12 +230,19 @@ jobs: "name": "assign_to_agent" }, { - "description": "Update an existing GitHub issue's status, title, or body. Use this to modify issue properties after creation. Only the fields you specify will be updated; other fields remain unchanged. CONSTRAINTS: Maximum 1 issue(s) can be updated. Target: ${{ github.event.issue.number }}.", + "description": "Update an existing GitHub issue's status, title, labels, assignees, milestone, or body. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. CONSTRAINTS: Maximum 1 issue(s) can be updated. Target: ${{ github.event.issue.number }}.", "inputSchema": { "additionalProperties": false, "properties": { + "assignees": { + "description": "Replace the issue assignees with this list of GitHub usernames (e.g., ['octocat', 'mona']).", + "items": { + "type": "string" + }, + "type": "array" + }, "body": { - "description": "New issue body to replace the existing content. Use Markdown formatting.", + "description": "Issue body content in Markdown. For 'replace', this becomes the entire body. For 'append'/'prepend', this content is added with a separator and an attribution footer. For 'replace-island', only the run-specific section is updated.", "type": "string" }, "issue_number": { @@ -245,6 +252,30 @@ jobs: "string" ] }, + "labels": { + "description": "Replace the issue labels with this list (e.g., ['bug', 'campaign:foo']). Labels must exist in the repository.", + "items": { + "type": "string" + }, + "type": "array" + }, + "milestone": { + "description": "Milestone number to assign (e.g., 1). Use null to clear.", + "type": [ + "number", + "string" + ] + }, + "operation": { + "description": "How to update the issue body: 'append' (default - add to end with separator), 'prepend' (add to start with separator), 'replace' (overwrite entire body), or 'replace-island' (update a run-specific section).", + "enum": [ + "replace", + "append", + "prepend", + "replace-island" + ], + "type": "string" + }, "status": { "description": "New issue status: 'open' to reopen a closed issue, 'closed' to close an open issue.", "enum": [ diff --git a/.github/workflows/workflow-health-manager.lock.yml b/.github/workflows/workflow-health-manager.lock.yml index 2a2c1d809a0..21d56abaab8 100644 --- a/.github/workflows/workflow-health-manager.lock.yml +++ b/.github/workflows/workflow-health-manager.lock.yml @@ -251,12 +251,19 @@ jobs: "name": "add_comment" }, { - "description": "Update an existing GitHub issue's status, title, or body. Use this to modify issue properties after creation. Only the fields you specify will be updated; other fields remain unchanged. CONSTRAINTS: Maximum 5 issue(s) can be updated.", + "description": "Update an existing GitHub issue's status, title, labels, assignees, milestone, or body. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. CONSTRAINTS: Maximum 5 issue(s) can be updated.", "inputSchema": { "additionalProperties": false, "properties": { + "assignees": { + "description": "Replace the issue assignees with this list of GitHub usernames (e.g., ['octocat', 'mona']).", + "items": { + "type": "string" + }, + "type": "array" + }, "body": { - "description": "New issue body to replace the existing content. Use Markdown formatting.", + "description": "Issue body content in Markdown. For 'replace', this becomes the entire body. For 'append'/'prepend', this content is added with a separator and an attribution footer. For 'replace-island', only the run-specific section is updated.", "type": "string" }, "issue_number": { @@ -266,6 +273,30 @@ jobs: "string" ] }, + "labels": { + "description": "Replace the issue labels with this list (e.g., ['bug', 'campaign:foo']). Labels must exist in the repository.", + "items": { + "type": "string" + }, + "type": "array" + }, + "milestone": { + "description": "Milestone number to assign (e.g., 1). Use null to clear.", + "type": [ + "number", + "string" + ] + }, + "operation": { + "description": "How to update the issue body: 'append' (default - add to end with separator), 'prepend' (add to start with separator), 'replace' (overwrite entire body), or 'replace-island' (update a run-specific section).", + "enum": [ + "replace", + "append", + "prepend", + "replace-island" + ], + "type": "string" + }, "status": { "description": "New issue status: 'open' to reopen a closed issue, 'closed' to close an open issue.", "enum": [ From 5125192e6a74dc0d0420c49b413549e53ef14b6a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 21 Jan 2026 22:27:20 +0000 Subject: [PATCH 3/7] Add compile-time validation for safe-outputs target field - Validates target values during compilation to catch errors early - Valid values: "triggering", "*", positive integers, GitHub expressions - Rejects invalid values like "event", "0", negative numbers, random strings - Includes comprehensive unit and integration tests Co-authored-by: eaftan <4733401+eaftan@users.noreply.github.com> --- pkg/workflow/compiler.go | 15 + .../safe_outputs_target_validation.go | 166 ++++++ ...puts_target_validation_integration_test.go | 265 ++++++++++ .../safe_outputs_target_validation_test.go | 494 ++++++++++++++++++ 4 files changed, 940 insertions(+) create mode 100644 pkg/workflow/safe_outputs_target_validation.go create mode 100644 pkg/workflow/safe_outputs_target_validation_integration_test.go create mode 100644 pkg/workflow/safe_outputs_target_validation_test.go diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 8b889a5acc6..b1d31b601d3 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -212,6 +212,21 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath return errors.New(formattedErr) } + // Validate safe-outputs target configuration + log.Printf("Validating safe-outputs target fields") + if err := validateSafeOutputsTarget(workflowData.SafeOutputs); err != nil { + formattedErr := console.FormatError(console.CompilerError{ + Position: console.ErrorPosition{ + File: markdownPath, + Line: 1, + Column: 1, + }, + Type: "error", + Message: err.Error(), + }) + return errors.New(formattedErr) + } + // Validate safe-outputs allowed-domains configuration log.Printf("Validating safe-outputs allowed-domains") if err := validateSafeOutputsAllowedDomains(workflowData.SafeOutputs); err != nil { diff --git a/pkg/workflow/safe_outputs_target_validation.go b/pkg/workflow/safe_outputs_target_validation.go new file mode 100644 index 00000000000..2d4ec991a6f --- /dev/null +++ b/pkg/workflow/safe_outputs_target_validation.go @@ -0,0 +1,166 @@ +package workflow + +import ( + "fmt" + "regexp" + "strconv" + "strings" + + "github.com/githubnext/gh-aw/pkg/logger" +) + +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 isPositiveInteger(target) { + safeOutputsTargetValidationLog.Printf("Target for %s is a valid number: %s", configName, target) + return nil + } + + // 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 }}\")\n\nDid you mean to use \"${{ github.event.issue.number }}\" instead of \"event\"?", + configName, + target, + ) +} + +// isGitHubExpression checks if a string contains a GitHub Actions expression +func isGitHubExpression(s string) bool { + // GitHub Actions expressions are in the format ${{ ... }} + return strings.Contains(s, "${{") && strings.Contains(s, "}}") +} + +// isPositiveInteger checks if a string is a positive integer +func isPositiveInteger(s string) bool { + // Must not be empty + if s == "" { + return false + } + + // Must not have leading zeros (except "0" itself, but that's not positive) + if len(s) > 1 && s[0] == '0' { + return false + } + + // Must be numeric and > 0 + num, err := strconv.ParseInt(s, 10, 64) + return err == nil && num > 0 +} + +// isValidTargetExpression validates complex GitHub Actions expressions +// This is a helper for more sophisticated validation if needed in the future +func isValidTargetExpression(expr string) bool { + // Basic regex to validate GitHub Actions expression syntax + // More sophisticated validation could be added here + pattern := `^\$\{\{[^}]+\}\}$` + matched, _ := regexp.MatchString(pattern, expr) + return matched +} diff --git a/pkg/workflow/safe_outputs_target_validation_integration_test.go b/pkg/workflow/safe_outputs_target_validation_integration_test.go new file mode 100644 index 00000000000..cea67b6e315 --- /dev/null +++ b/pkg/workflow/safe_outputs_target_validation_integration_test.go @@ -0,0 +1,265 @@ +package workflow + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/githubnext/gh-aw/pkg/testutil" +) + +// TestSafeOutputsTargetValidation_InvalidEvent tests that the compiler +// rejects an invalid target value like "event" at compile time +func TestSafeOutputsTargetValidation_InvalidEvent(t *testing.T) { + tmpDir := testutil.TempDir(t, "target-validation-test") + + // Create a workflow with invalid target: "event" + workflowContent := `--- +on: + issues: + types: [opened] +permissions: + contents: read + issues: write +engine: claude +features: + dangerous-permissions-write: true +strict: false +safe-outputs: + update-issue: + body: null + max: 1 + target: "event" +--- + +# Test Invalid Target + +This workflow should fail to compile because "event" is not a valid target value. +` + + workflowPath := filepath.Join(tmpDir, "test-invalid-target.md") + if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler(false, "", "test") + + // Attempt to compile the workflow - should fail with validation error + err := compiler.CompileWorkflow(workflowPath) + if err == nil { + t.Fatal("Expected compilation to fail with invalid target 'event', but it succeeded") + } + + // Verify error message contains helpful information + errMsg := err.Error() + if !strings.Contains(errMsg, "invalid target value") { + t.Errorf("Error message should mention 'invalid target value', got: %s", errMsg) + } + if !strings.Contains(errMsg, "update-issue") { + t.Errorf("Error message should mention 'update-issue', got: %s", errMsg) + } + if !strings.Contains(errMsg, "event") { + t.Errorf("Error message should mention the invalid value 'event', got: %s", errMsg) + } + // Should suggest valid values + if !strings.Contains(errMsg, "triggering") { + t.Errorf("Error message should suggest valid value 'triggering', got: %s", errMsg) + } +} + +// TestSafeOutputsTargetValidation_ValidValues tests that the compiler +// accepts valid target values +func TestSafeOutputsTargetValidation_ValidValues(t *testing.T) { + tests := []struct { + name string + target string + }{ + { + name: "triggering", + target: "triggering", + }, + { + name: "wildcard", + target: `"*"`, + }, + { + name: "explicit number", + target: `"123"`, + }, + { + name: "github expression", + target: "${{ github.event.issue.number }}", + }, + { + name: "empty (defaults to triggering)", + target: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "target-validation-valid-test") + + targetLine := "" + if tt.target != "" { + targetLine = " target: " + tt.target + } + + workflowContent := `--- +on: + issues: + types: [opened] +permissions: + contents: read + issues: write +engine: claude +features: + dangerous-permissions-write: true +strict: false +safe-outputs: + update-issue: + body: null + max: 1 +` + targetLine + ` +--- + +# Test Valid Target + +This workflow should compile successfully. +` + + workflowPath := filepath.Join(tmpDir, "test-valid-target.md") + if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler(false, "", "test") + + // Compile the workflow - should succeed + err := compiler.CompileWorkflow(workflowPath) + if err != nil { + t.Fatalf("Expected compilation to succeed with valid target %q, but got error: %v", tt.target, err) + } + }) + } +} + +// TestSafeOutputsTargetValidation_MultipleConfigs tests validation across +// multiple safe-output configurations +func TestSafeOutputsTargetValidation_MultipleConfigs(t *testing.T) { + tmpDir := testutil.TempDir(t, "target-validation-multiple-test") + + // Create a workflow with multiple safe-output configs, one with invalid target + workflowContent := `--- +on: + issues: + types: [opened] +permissions: + contents: read + issues: write + pull-requests: write +engine: claude +features: + dangerous-permissions-write: true +strict: false +safe-outputs: + update-issue: + body: null + target: "triggering" + close-issue: + target: "invalid-value" + add-labels: + target: "*" +--- + +# Test Multiple Configs + +This workflow should fail because close-issue has an invalid target. +` + + workflowPath := filepath.Join(tmpDir, "test-multiple-configs.md") + if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler(false, "", "test") + + // Attempt to compile the workflow - should fail + err := compiler.CompileWorkflow(workflowPath) + if err == nil { + t.Fatal("Expected compilation to fail with invalid target in close-issue, but it succeeded") + } + + // Verify error mentions close-issue + errMsg := err.Error() + if !strings.Contains(errMsg, "close-issue") { + t.Errorf("Error message should mention 'close-issue', got: %s", errMsg) + } + if !strings.Contains(errMsg, "invalid-value") { + t.Errorf("Error message should mention the invalid value, got: %s", errMsg) + } +} + +// TestSafeOutputsTargetValidation_InvalidNumericValues tests that +// zero and negative numbers are rejected +func TestSafeOutputsTargetValidation_InvalidNumericValues(t *testing.T) { + tests := []struct { + name string + target string + }{ + { + name: "zero", + target: "0", + }, + { + name: "negative number", + target: "-1", + }, + { + name: "leading zeros", + target: "007", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "target-validation-invalid-numeric-test") + + workflowContent := `--- +on: + issues: + types: [opened] +permissions: + contents: read + issues: write +engine: claude +features: + dangerous-permissions-write: true +strict: false +safe-outputs: + update-issue: + body: null + target: "` + tt.target + `" +--- + +# Test Invalid Numeric Target + +This workflow should fail to compile. +` + + workflowPath := filepath.Join(tmpDir, "test-invalid-numeric.md") + if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler(false, "", "test") + + // Attempt to compile the workflow - should fail + err := compiler.CompileWorkflow(workflowPath) + if err == nil { + t.Fatalf("Expected compilation to fail with invalid numeric target %q, but it succeeded", tt.target) + } + }) + } +} diff --git a/pkg/workflow/safe_outputs_target_validation_test.go b/pkg/workflow/safe_outputs_target_validation_test.go new file mode 100644 index 00000000000..7573503ba06 --- /dev/null +++ b/pkg/workflow/safe_outputs_target_validation_test.go @@ -0,0 +1,494 @@ +package workflow + +import ( + "strings" + "testing" +) + +func TestValidateSafeOutputsTarget(t *testing.T) { + tests := []struct { + name string + config *SafeOutputsConfig + wantErr bool + errText string + }{ + { + name: "nil config", + config: nil, + wantErr: false, + }, + { + name: "empty config", + config: &SafeOutputsConfig{}, + wantErr: false, + }, + { + name: "valid triggering target", + config: &SafeOutputsConfig{ + UpdateIssues: &UpdateIssuesConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "triggering", + }, + }, + }, + }, + wantErr: false, + }, + { + name: "valid empty target (defaults to triggering)", + config: &SafeOutputsConfig{ + UpdateIssues: &UpdateIssuesConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "", + }, + }, + }, + }, + wantErr: false, + }, + { + name: "valid wildcard target", + config: &SafeOutputsConfig{ + UpdateIssues: &UpdateIssuesConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "*", + }, + }, + }, + }, + wantErr: false, + }, + { + name: "valid numeric target", + config: &SafeOutputsConfig{ + UpdateIssues: &UpdateIssuesConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "123", + }, + }, + }, + }, + wantErr: false, + }, + { + name: "valid GitHub expression", + config: &SafeOutputsConfig{ + UpdateIssues: &UpdateIssuesConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "${{ github.event.issue.number }}", + }, + }, + }, + }, + wantErr: false, + }, + { + name: "invalid target - event", + config: &SafeOutputsConfig{ + UpdateIssues: &UpdateIssuesConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "event", + }, + }, + }, + }, + wantErr: true, + errText: "invalid target value for update-issue: \"event\"", + }, + { + name: "invalid target - negative number", + config: &SafeOutputsConfig{ + CloseIssues: &CloseEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "-1", + }, + }, + }, + wantErr: true, + errText: "invalid target value for close-issue: \"-1\"", + }, + { + name: "invalid target - zero", + config: &SafeOutputsConfig{ + AddLabels: &AddLabelsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{}, + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "0", + }, + }, + }, + wantErr: true, + errText: "invalid target value for add-labels: \"0\"", + }, + { + name: "invalid target - leading zeros", + config: &SafeOutputsConfig{ + AddLabels: &AddLabelsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{}, + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "0123", + }, + }, + }, + wantErr: true, + errText: "invalid target value for add-labels: \"0123\"", + }, + { + name: "invalid target - random string", + config: &SafeOutputsConfig{ + UpdateDiscussions: &UpdateDiscussionsConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "random-string", + }, + }, + }, + }, + wantErr: true, + errText: "invalid target value for update-discussion: \"random-string\"", + }, + { + name: "multiple configs with valid targets", + config: &SafeOutputsConfig{ + UpdateIssues: &UpdateIssuesConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "triggering", + }, + }, + }, + CloseIssues: &CloseEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "*", + }, + }, + AddLabels: &AddLabelsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{}, + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "456", + }, + }, + }, + wantErr: false, + }, + { + name: "multiple configs with one invalid target", + config: &SafeOutputsConfig{ + UpdateIssues: &UpdateIssuesConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "triggering", + }, + }, + }, + CloseIssues: &CloseEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "invalid", + }, + }, + }, + wantErr: true, + errText: "invalid target value for close-issue: \"invalid\"", + }, + { + name: "valid target for update-pull-request", + config: &SafeOutputsConfig{ + UpdatePullRequests: &UpdatePullRequestsConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "${{ github.event.pull_request.number }}", + }, + }, + }, + }, + wantErr: false, + }, + { + name: "valid target for close-discussion", + config: &SafeOutputsConfig{ + CloseDiscussions: &CloseEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "789", + }, + }, + }, + wantErr: false, + }, + { + name: "valid target for add-reviewer", + config: &SafeOutputsConfig{ + AddReviewer: &AddReviewerConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{}, + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "*", + }, + }, + }, + wantErr: false, + }, + { + name: "valid target for assign-milestone", + config: &SafeOutputsConfig{ + AssignMilestone: &AssignMilestoneConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{}, + SafeOutputTargetConfig: SafeOutputTargetConfig{ + Target: "triggering", + }, + }, + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateSafeOutputsTarget(tt.config) + if (err != nil) != tt.wantErr { + t.Errorf("validateSafeOutputsTarget() error = %v, wantErr %v", err, tt.wantErr) + return + } + if tt.wantErr && tt.errText != "" { + if !strings.Contains(err.Error(), tt.errText) { + t.Errorf("validateSafeOutputsTarget() error = %v, should contain %q", err, tt.errText) + } + } + }) + } +} + +func TestValidateTargetValue(t *testing.T) { + tests := []struct { + name string + configName string + target string + wantErr bool + errText string + }{ + { + name: "empty target", + configName: "test-config", + target: "", + wantErr: false, + }, + { + name: "triggering", + configName: "test-config", + target: "triggering", + wantErr: false, + }, + { + name: "wildcard", + configName: "test-config", + target: "*", + wantErr: false, + }, + { + name: "positive integer", + configName: "test-config", + target: "42", + wantErr: false, + }, + { + name: "large number", + configName: "test-config", + target: "999999", + wantErr: false, + }, + { + name: "GitHub expression", + configName: "test-config", + target: "${{ github.event.issue.number }}", + wantErr: false, + }, + { + name: "complex GitHub expression", + configName: "test-config", + target: "${{ github.event.pull_request.number || github.event.issue.number }}", + wantErr: false, + }, + { + name: "invalid - event", + configName: "update-issue", + target: "event", + wantErr: true, + errText: "invalid target value for update-issue: \"event\"", + }, + { + name: "invalid - zero", + configName: "test-config", + target: "0", + wantErr: true, + errText: "invalid target value", + }, + { + name: "invalid - negative", + configName: "test-config", + target: "-5", + wantErr: true, + errText: "invalid target value", + }, + { + name: "invalid - float", + configName: "test-config", + target: "3.14", + wantErr: true, + errText: "invalid target value", + }, + { + name: "invalid - leading zeros", + configName: "test-config", + target: "007", + wantErr: true, + errText: "invalid target value", + }, + { + name: "invalid - random string", + configName: "test-config", + target: "something-else", + wantErr: true, + errText: "invalid target value", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateTargetValue(tt.configName, tt.target) + if (err != nil) != tt.wantErr { + t.Errorf("validateTargetValue() error = %v, wantErr %v", err, tt.wantErr) + return + } + if tt.wantErr && tt.errText != "" { + if !strings.Contains(err.Error(), tt.errText) { + t.Errorf("validateTargetValue() error = %v, should contain %q", err, tt.errText) + } + } + }) + } +} + +func TestIsGitHubExpression(t *testing.T) { + tests := []struct { + name string + s string + want bool + }{ + { + name: "simple expression", + s: "${{ github.event.issue.number }}", + want: true, + }, + { + name: "complex expression", + s: "${{ github.event.pull_request.number || github.event.issue.number }}", + want: true, + }, + { + name: "nested expression", + s: "${{ fromJSON(github.event.issue.body).number }}", + want: true, + }, + { + name: "not an expression", + s: "event", + want: false, + }, + { + name: "partial expression start", + s: "${{ incomplete", + want: false, + }, + { + name: "partial expression end", + s: "incomplete }}", + want: false, + }, + { + name: "empty string", + s: "", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isGitHubExpression(tt.s) + if got != tt.want { + t.Errorf("isGitHubExpression() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestIsPositiveInteger(t *testing.T) { + tests := []struct { + name string + s string + want bool + }{ + { + name: "positive integer", + s: "123", + want: true, + }, + { + name: "one", + s: "1", + want: true, + }, + { + name: "large number", + s: "999999999", + want: true, + }, + { + name: "zero", + s: "0", + want: false, + }, + { + name: "negative", + s: "-5", + want: false, + }, + { + name: "leading zeros", + s: "007", + want: false, + }, + { + name: "float", + s: "3.14", + want: false, + }, + { + name: "not a number", + s: "abc", + want: false, + }, + { + name: "empty string", + s: "", + want: false, + }, + { + name: "spaces", + s: " 123 ", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isPositiveInteger(tt.s) + if got != tt.want { + t.Errorf("isPositiveInteger(%q) = %v, want %v", tt.s, got, tt.want) + } + }) + } +} From 16f6150a9dcd1d24e19e02520ef8ff645abc6e30 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 21 Jan 2026 22:36:32 +0000 Subject: [PATCH 4/7] Clean up unused function and import in validation code Co-authored-by: eaftan <4733401+eaftan@users.noreply.github.com> --- pkg/workflow/safe_outputs_target_validation.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/pkg/workflow/safe_outputs_target_validation.go b/pkg/workflow/safe_outputs_target_validation.go index 2d4ec991a6f..8284fb05437 100644 --- a/pkg/workflow/safe_outputs_target_validation.go +++ b/pkg/workflow/safe_outputs_target_validation.go @@ -2,7 +2,6 @@ package workflow import ( "fmt" - "regexp" "strconv" "strings" @@ -154,13 +153,3 @@ func isPositiveInteger(s string) bool { num, err := strconv.ParseInt(s, 10, 64) return err == nil && num > 0 } - -// isValidTargetExpression validates complex GitHub Actions expressions -// This is a helper for more sophisticated validation if needed in the future -func isValidTargetExpression(expr string) bool { - // Basic regex to validate GitHub Actions expression syntax - // More sophisticated validation could be added here - pattern := `^\$\{\{[^}]+\}\}$` - matched, _ := regexp.MatchString(pattern, expr) - return matched -} From eded1a329b95dc25c189331bdc21ce754a8ce199 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 21 Jan 2026 22:39:26 +0000 Subject: [PATCH 5/7] Address code review feedback - improve validation and error messages - Make suggestion for "event" conditional (only shown for event-related invalid values) - Improve GitHub expression validation to check for proper bracket order - Add comprehensive tests for edge cases in expression validation Co-authored-by: eaftan <4733401+eaftan@users.noreply.github.com> --- .../safe_outputs_target_validation.go | 26 ++++++++++++++++--- .../safe_outputs_target_validation_test.go | 21 ++++++++++++--- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/pkg/workflow/safe_outputs_target_validation.go b/pkg/workflow/safe_outputs_target_validation.go index 8284fb05437..0a080777f97 100644 --- a/pkg/workflow/safe_outputs_target_validation.go +++ b/pkg/workflow/safe_outputs_target_validation.go @@ -123,18 +123,36 @@ func validateTargetValue(configName, target string) error { 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 }}\")\n\nDid you mean to use \"${{ github.event.issue.number }}\" instead of \"event\"?", + "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 contains a GitHub Actions expression +// 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 { - // GitHub Actions expressions are in the format ${{ ... }} - return strings.Contains(s, "${{") && strings.Contains(s, "}}") + // 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 } // isPositiveInteger checks if a string is a positive integer diff --git a/pkg/workflow/safe_outputs_target_validation_test.go b/pkg/workflow/safe_outputs_target_validation_test.go index 7573503ba06..e95442d8a0a 100644 --- a/pkg/workflow/safe_outputs_target_validation_test.go +++ b/pkg/workflow/safe_outputs_target_validation_test.go @@ -399,13 +399,18 @@ func TestIsGitHubExpression(t *testing.T) { want: false, }, { - name: "partial expression start", + name: "incomplete expression - missing opening", + s: "incomplete }}", + want: false, + }, + { + name: "incomplete expression - missing closing", s: "${{ incomplete", want: false, }, { - name: "partial expression end", - s: "incomplete }}", + name: "empty expression", + s: "${{}}", want: false, }, { @@ -413,6 +418,16 @@ func TestIsGitHubExpression(t *testing.T) { s: "", want: false, }, + { + name: "wrong order - closing before opening", + s: "}} some ${{ text", + want: false, + }, + { + name: "text with embedded markers but invalid order", + s: "text }} more ${{ stuff", + want: false, + }, } for _, tt := range tests { From 80b33181192b5f8957d4ff80849a8babd344b49b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 21 Jan 2026 23:55:15 +0000 Subject: [PATCH 6/7] Move isPositiveInteger to stringutil package - Moved IsPositiveInteger function from workflow validation to pkg/stringutil - Moved associated tests to stringutil_test.go - Updated safe_outputs_target_validation.go to use stringutil.IsPositiveInteger - Removed strconv import from validation file (no longer needed) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/stringutil/stringutil.go | 25 +++++++ pkg/stringutil/stringutil_test.go | 68 +++++++++++++++++++ .../safe_outputs_target_validation.go | 21 +----- .../safe_outputs_target_validation_test.go | 68 ------------------- 4 files changed, 95 insertions(+), 87 deletions(-) diff --git a/pkg/stringutil/stringutil.go b/pkg/stringutil/stringutil.go index a740fed0f5d..4dcfeb52280 100644 --- a/pkg/stringutil/stringutil.go +++ b/pkg/stringutil/stringutil.go @@ -3,6 +3,7 @@ package stringutil import ( "fmt" + "strconv" "strings" ) @@ -56,3 +57,27 @@ func ParseVersionValue(version any) string { return "" } } + +// IsPositiveInteger checks if a string is a positive integer. +// Returns true for strings like "1", "123", "999" but false for: +// - Zero ("0") +// - Negative numbers ("-5") +// - Numbers with leading zeros ("007") +// - Floating point numbers ("3.14") +// - Non-numeric strings ("abc") +// - Empty strings ("") +func IsPositiveInteger(s string) bool { + // Must not be empty + if s == "" { + return false + } + + // Must not have leading zeros (except "0" itself, but that's not positive) + if len(s) > 1 && s[0] == '0' { + return false + } + + // Must be numeric and > 0 + num, err := strconv.ParseInt(s, 10, 64) + return err == nil && num > 0 +} diff --git a/pkg/stringutil/stringutil_test.go b/pkg/stringutil/stringutil_test.go index 9824f2b444e..2f764494f9b 100644 --- a/pkg/stringutil/stringutil_test.go +++ b/pkg/stringutil/stringutil_test.go @@ -411,3 +411,71 @@ func TestParseVersionValue(t *testing.T) { }) } } + +func TestIsPositiveInteger(t *testing.T) { + tests := []struct { + name string + s string + want bool + }{ + { + name: "positive integer", + s: "123", + want: true, + }, + { + name: "one", + s: "1", + want: true, + }, + { + name: "large number", + s: "999999999", + want: true, + }, + { + name: "zero", + s: "0", + want: false, + }, + { + name: "negative", + s: "-5", + want: false, + }, + { + name: "leading zeros", + s: "007", + want: false, + }, + { + name: "float", + s: "3.14", + want: false, + }, + { + name: "not a number", + s: "abc", + want: false, + }, + { + name: "empty string", + s: "", + want: false, + }, + { + name: "spaces", + s: " 123 ", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsPositiveInteger(tt.s) + if got != tt.want { + t.Errorf("IsPositiveInteger(%q) = %v, want %v", tt.s, got, tt.want) + } + }) + } +} diff --git a/pkg/workflow/safe_outputs_target_validation.go b/pkg/workflow/safe_outputs_target_validation.go index 0a080777f97..f329a45aaa4 100644 --- a/pkg/workflow/safe_outputs_target_validation.go +++ b/pkg/workflow/safe_outputs_target_validation.go @@ -2,10 +2,10 @@ package workflow import ( "fmt" - "strconv" "strings" "github.com/githubnext/gh-aw/pkg/logger" + "github.com/githubnext/gh-aw/pkg/stringutil" ) var safeOutputsTargetValidationLog = logger.New("workflow:safe_outputs_target_validation") @@ -118,7 +118,7 @@ func validateTargetValue(configName, target string) error { } // Check if it's a positive integer - if isPositiveInteger(target) { + if stringutil.IsPositiveInteger(target) { safeOutputsTargetValidationLog.Printf("Target for %s is a valid number: %s", configName, target) return nil } @@ -154,20 +154,3 @@ func isGitHubExpression(s string) bool { // and there must be something between them return openIndex >= 0 && closeIndex > openIndex+3 } - -// isPositiveInteger checks if a string is a positive integer -func isPositiveInteger(s string) bool { - // Must not be empty - if s == "" { - return false - } - - // Must not have leading zeros (except "0" itself, but that's not positive) - if len(s) > 1 && s[0] == '0' { - return false - } - - // Must be numeric and > 0 - num, err := strconv.ParseInt(s, 10, 64) - return err == nil && num > 0 -} diff --git a/pkg/workflow/safe_outputs_target_validation_test.go b/pkg/workflow/safe_outputs_target_validation_test.go index e95442d8a0a..d1244637a2a 100644 --- a/pkg/workflow/safe_outputs_target_validation_test.go +++ b/pkg/workflow/safe_outputs_target_validation_test.go @@ -439,71 +439,3 @@ func TestIsGitHubExpression(t *testing.T) { }) } } - -func TestIsPositiveInteger(t *testing.T) { - tests := []struct { - name string - s string - want bool - }{ - { - name: "positive integer", - s: "123", - want: true, - }, - { - name: "one", - s: "1", - want: true, - }, - { - name: "large number", - s: "999999999", - want: true, - }, - { - name: "zero", - s: "0", - want: false, - }, - { - name: "negative", - s: "-5", - want: false, - }, - { - name: "leading zeros", - s: "007", - want: false, - }, - { - name: "float", - s: "3.14", - want: false, - }, - { - name: "not a number", - s: "abc", - want: false, - }, - { - name: "empty string", - s: "", - want: false, - }, - { - name: "spaces", - s: " 123 ", - want: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := isPositiveInteger(tt.s) - if got != tt.want { - t.Errorf("isPositiveInteger(%q) = %v, want %v", tt.s, got, tt.want) - } - }) - } -} From 64f8453d14273696d36435d8fb1e8d21a0acc59f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 22 Jan 2026 00:23:19 +0000 Subject: [PATCH 7/7] Format code and complete merge of main branch - Resolved merge conflicts in pkg/stringutil files - Kept both IsPositiveInteger and StripANSIEscapeCodes functions - Recompiled all workflows successfully - All tests passing after merge Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/stringutil/stringutil_test.go | 128 +++++++++--------- .../safe_outputs_target_validation.go | 2 +- 2 files changed, 65 insertions(+), 65 deletions(-) diff --git a/pkg/stringutil/stringutil_test.go b/pkg/stringutil/stringutil_test.go index 03e624b84d5..4057542aa74 100644 --- a/pkg/stringutil/stringutil_test.go +++ b/pkg/stringutil/stringutil_test.go @@ -570,69 +570,69 @@ func BenchmarkStripANSIEscapeCodes_WithCodes(b *testing.B) { } func TestIsPositiveInteger(t *testing.T) { -tests := []struct { -name string -s string -want bool -}{ -{ -name: "positive integer", -s: "123", -want: true, -}, -{ -name: "one", -s: "1", -want: true, -}, -{ -name: "large number", -s: "999999999", -want: true, -}, -{ -name: "zero", -s: "0", -want: false, -}, -{ -name: "negative", -s: "-5", -want: false, -}, -{ -name: "leading zeros", -s: "007", -want: false, -}, -{ -name: "float", -s: "3.14", -want: false, -}, -{ -name: "not a number", -s: "abc", -want: false, -}, -{ -name: "empty string", -s: "", -want: false, -}, -{ -name: "spaces", -s: " 123 ", -want: false, -}, -} + tests := []struct { + name string + s string + want bool + }{ + { + name: "positive integer", + s: "123", + want: true, + }, + { + name: "one", + s: "1", + want: true, + }, + { + name: "large number", + s: "999999999", + want: true, + }, + { + name: "zero", + s: "0", + want: false, + }, + { + name: "negative", + s: "-5", + want: false, + }, + { + name: "leading zeros", + s: "007", + want: false, + }, + { + name: "float", + s: "3.14", + want: false, + }, + { + name: "not a number", + s: "abc", + want: false, + }, + { + name: "empty string", + s: "", + want: false, + }, + { + name: "spaces", + s: " 123 ", + want: false, + }, + } -for _, tt := range tests { -t.Run(tt.name, func(t *testing.T) { -got := IsPositiveInteger(tt.s) -if got != tt.want { -t.Errorf("IsPositiveInteger(%q) = %v, want %v", tt.s, got, tt.want) -} -}) -} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsPositiveInteger(tt.s) + if got != tt.want { + t.Errorf("IsPositiveInteger(%q) = %v, want %v", tt.s, got, tt.want) + } + }) + } } diff --git a/pkg/workflow/safe_outputs_target_validation.go b/pkg/workflow/safe_outputs_target_validation.go index f329a45aaa4..c8c0a8078aa 100644 --- a/pkg/workflow/safe_outputs_target_validation.go +++ b/pkg/workflow/safe_outputs_target_validation.go @@ -149,7 +149,7 @@ func isGitHubExpression(s string) bool { // 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