diff --git a/.changeset/patch-allow-wildcard-target-repo-safe-outputs.md b/.changeset/patch-allow-wildcard-target-repo-safe-outputs.md new file mode 100644 index 0000000000..052feaf1e3 --- /dev/null +++ b/.changeset/patch-allow-wildcard-target-repo-safe-outputs.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Fixed safe-output handler parsing to allow `target-repo: "*"` across add-comment, create-issue, create-discussion, close-entity, add-reviewer, and create-pull-request handlers so wildcard-targeted handlers are preserved in `GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG`. diff --git a/actions/setup/js/add_comment.test.cjs b/actions/setup/js/add_comment.test.cjs index 54061e4a3c..3a61c249c4 100644 --- a/actions/setup/js/add_comment.test.cjs +++ b/actions/setup/js/add_comment.test.cjs @@ -287,6 +287,38 @@ describe("add_comment", () => { expect(result.itemNumber).toBe(42); expect(result.isDiscussion).toBe(false); }); + + it('should allow commenting on any repo when target-repo is "*"', async () => { + const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8"); + + let capturedOwner = null; + let capturedRepo = null; + mockGithub.rest.issues.createComment = async params => { + capturedOwner = params.owner; + capturedRepo = params.repo; + return { + data: { + id: 12345, + html_url: `https://github.com/${params.owner}/${params.repo}/issues/${params.issue_number}#issuecomment-12345`, + }, + }; + }; + + const handler = await eval(`(async () => { ${addCommentScript}; return await main({ "target-repo": "*", target: "triggering" }); })()`); + + const message = { + type: "add_comment", + item_number: 5, + repo: "other-org/other-repo", + body: "Cross-repo comment", + }; + + const result = await handler(message, {}); + + expect(result.success).toBe(true); + expect(capturedOwner).toBe("other-org"); + expect(capturedRepo).toBe("other-repo"); + }); }); describe("discussion support", () => { diff --git a/actions/setup/js/close_issue.test.cjs b/actions/setup/js/close_issue.test.cjs index 9e56df8dd2..c9b3166f5c 100644 --- a/actions/setup/js/close_issue.test.cjs +++ b/actions/setup/js/close_issue.test.cjs @@ -598,6 +598,48 @@ describe("close_issue", () => { expect(updateCalls[0].repo).toBe("gh-aw"); }); + it("should close issue in any repo when target-repo is wildcard *", async () => { + const handler = await main({ + max: 10, + "target-repo": "*", + }); + const updateCalls = []; + + mockGithub.rest.issues.get = async params => ({ + data: { + number: params.issue_number, + title: "Test Issue", + labels: [], + html_url: `https://github.com/${params.owner}/${params.repo}/issues/${params.issue_number}`, + state: "open", + }, + }); + + mockGithub.rest.issues.update = async params => { + updateCalls.push(params); + return { + data: { + number: params.issue_number, + title: "Test Issue", + html_url: `https://github.com/${params.owner}/${params.repo}/issues/${params.issue_number}`, + }, + }; + }; + + const result = await handler( + { + issue_number: 200, + repo: "any-org/any-repo", + body: "Closing", + }, + {} + ); + + expect(result.success).toBe(true); + expect(updateCalls[0].owner).toBe("any-org"); + expect(updateCalls[0].repo).toBe("any-repo"); + }); + it("should use default state_reason 'COMPLETED' when not specified", async () => { const handler = await main({ max: 10 }); const updateCalls = []; diff --git a/actions/setup/js/create_discussion_labels.test.cjs b/actions/setup/js/create_discussion_labels.test.cjs index 8537c39039..8e7befbe86 100644 --- a/actions/setup/js/create_discussion_labels.test.cjs +++ b/actions/setup/js/create_discussion_labels.test.cjs @@ -433,4 +433,24 @@ describe("create_discussion with labels", () => { expect(result.error).toContain("Cannot add more than 10 labels"); expect(result.error).toContain("received 11"); }); + + it('should create discussion in any repo when target-repo is "*"', async () => { + const config = { + "target-repo": "*", + category: "general", + }; + + const handler = await createDiscussionMain(config); + + const message = { + title: "Cross-repo Discussion", + body: "Discussion body", + repo: "other-org/other-repo", + }; + + const result = await handler(message, {}); + + expect(result.success).toBe(true); + expect(result.number).toBe(42); + }); }); diff --git a/actions/setup/js/create_issue.test.cjs b/actions/setup/js/create_issue.test.cjs index e6d71690e4..2c2d206923 100644 --- a/actions/setup/js/create_issue.test.cjs +++ b/actions/setup/js/create_issue.test.cjs @@ -315,6 +315,34 @@ describe("create_issue", () => { }) ); }); + it("should create issue in specified repo when target-repo is wildcard *", async () => { + const handler = await main({ + "target-repo": "*", + }); + await handler({ + title: "Test", + repo: "any-org/any-repo", + }); + + expect(mockGithub.rest.issues.create).toHaveBeenCalledWith( + expect.objectContaining({ + owner: "any-org", + repo: "any-repo", + }) + ); + }); + + it("should reject invalid repo slug when target-repo is wildcard *", async () => { + const handler = await main({ + "target-repo": "*", + }); + const result = await handler({ + title: "Test", + repo: "bare-repo-without-slash", + }); + + expect(result.success).toBe(false); + }); }); describe("temporary ID management", () => { diff --git a/actions/setup/js/create_pull_request.test.cjs b/actions/setup/js/create_pull_request.test.cjs index 337ec29467..0028dee491 100644 --- a/actions/setup/js/create_pull_request.test.cjs +++ b/actions/setup/js/create_pull_request.test.cjs @@ -893,3 +893,114 @@ describe("create_pull_request - configured reviewers", () => { expect(global.github.rest.pulls.requestReviewers).toHaveBeenCalledWith(expect.objectContaining({ reviewers: ["user1", "user2"] })); }); }); + +describe("create_pull_request - wildcard target-repo", () => { + let tempDir; + let originalEnv; + + beforeEach(() => { + originalEnv = { ...process.env }; + process.env.GH_AW_WORKFLOW_ID = "test-workflow"; + process.env.GITHUB_REPOSITORY = "test-owner/test-repo"; + process.env.GITHUB_BASE_REF = "main"; + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "create-pr-wildcard-test-")); + + global.core = { + info: vi.fn(), + warning: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + setFailed: vi.fn(), + setOutput: vi.fn(), + startGroup: vi.fn(), + endGroup: vi.fn(), + summary: { + addRaw: vi.fn().mockReturnThis(), + write: vi.fn().mockResolvedValue(undefined), + }, + }; + global.github = { + rest: { + pulls: { + create: vi.fn().mockResolvedValue({ data: { number: 99, html_url: "https://github.com/any-org/any-repo/pull/99", node_id: "PR_99" } }), + requestReviewers: vi.fn().mockResolvedValue({}), + }, + repos: { + get: vi.fn().mockResolvedValue({ data: { default_branch: "main" } }), + }, + issues: { + addLabels: vi.fn().mockResolvedValue({}), + }, + }, + graphql: vi.fn(), + }; + global.context = { + eventName: "workflow_dispatch", + repo: { owner: "test-owner", repo: "test-repo" }, + payload: {}, + }; + global.exec = { + exec: vi.fn().mockResolvedValue(0), + getExecOutput: vi.fn().mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }), + }; + + delete require.cache[require.resolve("./create_pull_request.cjs")]; + }); + + afterEach(() => { + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) { + delete process.env[key]; + } + } + Object.assign(process.env, originalEnv); + + if (tempDir && fs.existsSync(tempDir)) { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + + delete global.core; + delete global.github; + delete global.context; + delete global.exec; + vi.clearAllMocks(); + }); + + it('should create PR in any repo when target-repo is "*"', async () => { + const { main } = require("./create_pull_request.cjs"); + const handler = await main({ "target-repo": "*", allow_empty: true }); + + const result = await handler( + { + title: "Test PR", + body: "Test body", + repo: "any-org/any-repo", + }, + {} + ); + + expect(result.success).toBe(true); + expect(global.github.rest.pulls.create).toHaveBeenCalledWith( + expect.objectContaining({ + owner: "any-org", + repo: "any-repo", + }) + ); + }); + + it('should reject invalid repo slug when target-repo is "*"', async () => { + const { main } = require("./create_pull_request.cjs"); + const handler = await main({ "target-repo": "*", allow_empty: true }); + + const result = await handler( + { + title: "Test PR", + body: "Test body", + repo: "not-a-valid-slug", + }, + {} + ); + + expect(result.success).toBe(false); + }); +}); diff --git a/actions/setup/js/repo_helpers.test.cjs b/actions/setup/js/repo_helpers.test.cjs index f6e58b83a3..0063791d97 100644 --- a/actions/setup/js/repo_helpers.test.cjs +++ b/actions/setup/js/repo_helpers.test.cjs @@ -506,5 +506,45 @@ describe("repo_helpers", () => { expect(result.allowedRepos.has("org/repo-2")).toBe(true); expect(result.allowedRepos.has("org/repo-3")).toBe(true); }); + + it('should pass through wildcard "*" as defaultTargetRepo', async () => { + const { resolveTargetRepoConfig } = await import("./repo_helpers.cjs"); + const config = { "target-repo": "*" }; + + const result = resolveTargetRepoConfig(config); + + expect(result.defaultTargetRepo).toBe("*"); + expect(result.allowedRepos.size).toBe(0); + }); + }); + + describe('resolveAndValidateRepo with wildcard target-repo "*"', () => { + it("should allow any valid owner/repo slug when defaultTargetRepo is *", async () => { + const { resolveAndValidateRepo } = await import("./repo_helpers.cjs"); + const result = resolveAndValidateRepo({ repo: "any-org/any-repo" }, "*", new Set(), "test"); + + expect(result.success).toBe(true); + expect(result.repo).toBe("any-org/any-repo"); + expect(result.repoParts).toEqual({ owner: "any-org", repo: "any-repo" }); + }); + + it("should use item.repo when defaultTargetRepo is * and no repo in item", async () => { + const { resolveAndValidateRepo } = await import("./repo_helpers.cjs"); + // When item has no repo and defaultTargetRepo is "*", itemRepo becomes "*" + // which fails parseRepoSlug — so the caller should always supply item.repo with * + const result = resolveAndValidateRepo({}, "*", new Set(), "test"); + + // "*" is not a valid owner/repo slug (no slash with two parts) + expect(result.success).toBe(false); + }); + + it("should reject bare repo name (no slash) when defaultTargetRepo is *", async () => { + const { resolveAndValidateRepo } = await import("./repo_helpers.cjs"); + const result = resolveAndValidateRepo({ repo: "bare-repo" }, "*", new Set(), "test"); + + // bare-repo qualifies using defaultRepo's org, but "*" has no org → falls back to bare name + // which is invalid as owner/repo + expect(result.success).toBe(false); + }); }); }); diff --git a/pkg/workflow/add_comment.go b/pkg/workflow/add_comment.go index e39efd9104..15aa7b5417 100644 --- a/pkg/workflow/add_comment.go +++ b/pkg/workflow/add_comment.go @@ -67,11 +67,6 @@ func (c *Compiler) parseCommentsConfig(outputMap map[string]any) *AddCommentsCon config.Max = defaultIntStr(1) } - // Validate target-repo (wildcard "*" is not allowed) - if validateTargetRepoSlug(config.TargetRepoSlug, addCommentLog) { - return nil // Invalid configuration, return nil to cause validation error - } - // Validate discussion field - must be true if present if config.Discussion != nil && !*config.Discussion { addCommentLog.Print("Invalid discussion: must be true if present") diff --git a/pkg/workflow/add_comment_target_repo_integration_test.go b/pkg/workflow/add_comment_target_repo_integration_test.go index 59b5b8ffbf..797d8e5f4b 100644 --- a/pkg/workflow/add_comment_target_repo_integration_test.go +++ b/pkg/workflow/add_comment_target_repo_integration_test.go @@ -68,6 +68,22 @@ func TestAddCommentTargetRepoIntegration(t *testing.T) { shouldHaveTargetRepo: false, // Trial mode sets env var, not config expectedTargetRepoValue: "", // Not checked }, + { + name: "target-repo wildcard should be in handler config", + frontmatter: map[string]any{ + "name": "Test Workflow", + "engine": "copilot", + "safe-outputs": map[string]any{ + "add-comment": map[string]any{ + "max": 1, + "target": "*", + "target-repo": "*", + }, + }, + }, + shouldHaveTargetRepo: true, + expectedTargetRepoValue: "*", + }, { name: "no target-repo and no trial should not have target-repo in handler config", frontmatter: map[string]any{ diff --git a/pkg/workflow/add_comment_target_repo_test.go b/pkg/workflow/add_comment_target_repo_test.go index 6ba7bede49..c4376397d1 100644 --- a/pkg/workflow/add_comment_target_repo_test.go +++ b/pkg/workflow/add_comment_target_repo_test.go @@ -30,15 +30,17 @@ func TestAddCommentsConfigTargetRepo(t *testing.T) { shouldBeNil: false, }, { - name: "target-repo with wildcard should be rejected", + name: "target-repo with wildcard is allowed", configMap: map[string]any{ "add-comment": map[string]any{ - "max": 5, - "target": "123", + "max": 1, + "target": "*", "target-repo": "*", }, }, - shouldBeNil: true, // Configuration should be nil due to validation + expectedTarget: "*", + expectedRepo: "*", + shouldBeNil: false, // Wildcard "*" is a valid target-repo for add-comment }, { name: "target-repo without target field", diff --git a/pkg/workflow/add_reviewer.go b/pkg/workflow/add_reviewer.go index 243d734689..8d27898c15 100644 --- a/pkg/workflow/add_reviewer.go +++ b/pkg/workflow/add_reviewer.go @@ -44,11 +44,6 @@ func (c *Compiler) parseAddReviewerConfig(outputMap map[string]any) *AddReviewer config.Max = defaultIntStr(3) } - // Validate target-repo (wildcard "*" is not allowed for safe outputs) - if validateTargetRepoSlug(config.TargetRepoSlug, addReviewerLog) { - return nil - } - addReviewerLog.Printf("Parsed add-reviewer config: allowed_reviewers=%d, target=%s", len(config.Reviewers), config.Target) return &config diff --git a/pkg/workflow/close_entity_helpers.go b/pkg/workflow/close_entity_helpers.go index e00d9f7144..221da19abb 100644 --- a/pkg/workflow/close_entity_helpers.go +++ b/pkg/workflow/close_entity_helpers.go @@ -119,12 +119,6 @@ func (c *Compiler) parseCloseEntityConfig(outputMap map[string]any, params Close logger.Printf("Set default max to 1 for %s", params.ConfigKey) } - // Validate target-repo (wildcard "*" is not allowed) - if validateTargetRepoSlug(config.TargetRepoSlug, logger) { - logger.Printf("Invalid target-repo slug for %s", params.ConfigKey) - return nil - } - logger.Printf("Parsed %s configuration: max=%s, target=%s", params.ConfigKey, *config.Max, config.Target) return &config diff --git a/pkg/workflow/config_parsing_helpers_test.go b/pkg/workflow/config_parsing_helpers_test.go index a8452453fe..bad24e1705 100644 --- a/pkg/workflow/config_parsing_helpers_test.go +++ b/pkg/workflow/config_parsing_helpers_test.go @@ -426,7 +426,7 @@ func TestParsePRReviewCommentsConfigWithHelpers(t *testing.T) { } } -// Test wildcard validation (should return nil for invalid config) +// Test wildcard target-repo is now allowed for all create/close handlers func TestParseIssuesConfigWithWildcardTargetRepo(t *testing.T) { compiler := &Compiler{} @@ -437,8 +437,10 @@ func TestParseIssuesConfigWithWildcardTargetRepo(t *testing.T) { } result := compiler.parseIssuesConfig(outputMap) - if result != nil { - t.Errorf("expected nil for wildcard target-repo, got %+v", result) + if result == nil { + t.Errorf("expected non-nil config for wildcard target-repo, got nil") + } else if result.TargetRepoSlug != "*" { + t.Errorf("expected TargetRepoSlug to be \"*\", got %q", result.TargetRepoSlug) } } @@ -451,8 +453,10 @@ func TestParsePullRequestsConfigWithWildcardTargetRepo(t *testing.T) { } result := compiler.parsePullRequestsConfig(outputMap) - if result != nil { - t.Errorf("expected nil for wildcard target-repo, got %+v", result) + if result == nil { + t.Errorf("expected non-nil config for wildcard target-repo, got nil") + } else if result.TargetRepoSlug != "*" { + t.Errorf("expected TargetRepoSlug to be \"*\", got %q", result.TargetRepoSlug) } } @@ -461,12 +465,15 @@ func TestParseDiscussionsConfigWithWildcardTargetRepo(t *testing.T) { outputMap := map[string]any{ "create-discussion": map[string]any{ "target-repo": "*", + "category": "General", }, } result := compiler.parseDiscussionsConfig(outputMap) - if result != nil { - t.Errorf("expected nil for wildcard target-repo, got %+v", result) + if result == nil { + t.Errorf("expected non-nil config for wildcard target-repo, got nil") + } else if result.TargetRepoSlug != "*" { + t.Errorf("expected TargetRepoSlug to be \"*\", got %q", result.TargetRepoSlug) } } @@ -479,8 +486,10 @@ func TestParseCommentsConfigWithWildcardTargetRepo(t *testing.T) { } result := compiler.parseCommentsConfig(outputMap) - if result != nil { - t.Errorf("expected nil for wildcard target-repo, got %+v", result) + if result == nil { + t.Errorf("expected non-nil config for wildcard target-repo, got nil") + } else if result.TargetRepoSlug != "*" { + t.Errorf("expected TargetRepoSlug to be \"*\", got %q", result.TargetRepoSlug) } } diff --git a/pkg/workflow/create_discussion.go b/pkg/workflow/create_discussion.go index 2223eb7d3b..4c740ee620 100644 --- a/pkg/workflow/create_discussion.go +++ b/pkg/workflow/create_discussion.go @@ -85,11 +85,6 @@ func (c *Compiler) parseDiscussionsConfig(outputMap map[string]any) *CreateDiscu discussionLog.Print("Using default fallback-to-issue: true") } - // Validate target-repo (wildcard "*" is not allowed) - if validateTargetRepoSlug(config.TargetRepoSlug, discussionLog) { - return nil // Invalid configuration, return nil to cause validation error - } - // Normalize and validate category naming convention config.Category = normalizeDiscussionCategory(config.Category, discussionLog, c.markdownPath) diff --git a/pkg/workflow/create_issue.go b/pkg/workflow/create_issue.go index 278f3ddc6e..e70e5f90d4 100644 --- a/pkg/workflow/create_issue.go +++ b/pkg/workflow/create_issue.go @@ -77,11 +77,6 @@ func (c *Compiler) parseIssuesConfig(outputMap map[string]any) *CreateIssuesConf config.Max = defaultIntStr(1) } - // Validate target-repo (wildcard "*" is not allowed) - if validateTargetRepoSlug(config.TargetRepoSlug, createIssueLog) { - return nil // Invalid configuration, return nil to cause validation error - } - // Log expires if configured or explicitly disabled if expiresDisabled { createIssueLog.Print("Issue expiration explicitly disabled") diff --git a/pkg/workflow/create_pull_request.go b/pkg/workflow/create_pull_request.go index 1533f9340f..bb7648f727 100644 --- a/pkg/workflow/create_pull_request.go +++ b/pkg/workflow/create_pull_request.go @@ -100,11 +100,6 @@ func (c *Compiler) parsePullRequestsConfig(outputMap map[string]any) *CreatePull config = CreatePullRequestsConfig{} } - // Validate target-repo (wildcard "*" is not allowed) - if validateTargetRepoSlug(config.TargetRepoSlug, createPRLog) { - return nil // Invalid configuration, return nil to cause validation error - } - // Log expires if configured if config.Expires > 0 { createPRLog.Printf("Pull request expiration configured: %d hours", config.Expires) diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go index 01aeb75d82..f095b41310 100644 --- a/pkg/workflow/validation_helpers.go +++ b/pkg/workflow/validation_helpers.go @@ -104,19 +104,6 @@ func formatList(items []string) string { return fmt.Sprintf("%s, and %s", formatList(items[:len(items)-1]), items[len(items)-1]) } -// validateTargetRepoSlug validates that a target-repo slug is not a wildcard. -// Returns true if the value is invalid (i.e., equals "*"). -// This helper is used when the target-repo has already been parsed into a struct field. -func validateTargetRepoSlug(targetRepoSlug string, log *logger.Logger) bool { - if targetRepoSlug == "*" { - if log != nil { - log.Print("Invalid target-repo: wildcard '*' is not allowed") - } - return true // Return true to indicate validation error - } - return false -} - // validateStringEnumField checks that a config field, if present, contains one // of the allowed string values. Non-string values and unrecognised strings are // removed from the map (treated as absent) and a warning is logged. Use this