diff --git a/.changeset/patch-fix-cross-repo-update-issue.md b/.changeset/patch-fix-cross-repo-update-issue.md new file mode 100644 index 00000000000..fe18262c31b --- /dev/null +++ b/.changeset/patch-fix-cross-repo-update-issue.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Fixed cross-repo `update-issue`/`update_pull_request` safe-outputs by honoring `target-repo` even without an explicit `repo` and allowing `target-repo: "*"` to validate other repos. diff --git a/actions/setup/js/repo_helpers.cjs b/actions/setup/js/repo_helpers.cjs index d0fc57e236d..1b9ea222862 100644 --- a/actions/setup/js/repo_helpers.cjs +++ b/actions/setup/js/repo_helpers.cjs @@ -169,6 +169,21 @@ function validateRepo(repo, defaultRepo, allowedRepos) { } } + // Wildcard default repo allows any target repo, but still require a valid owner/repo slug + if (defaultRepo === "*") { + const parsed = parseRepoSlug(qualifiedRepo); + if (!parsed) { + return { + valid: false, + error: `Repository '${repo}' is not a valid 'owner/repo' slug.`, + qualifiedRepo, + }; + } + // Normalize to a fully-qualified slug to honor the contract of RepoValidationResult + qualifiedRepo = `${parsed.owner}/${parsed.repo}`; + return { valid: true, error: null, qualifiedRepo }; + } + // Default repo is always allowed if (qualifiedRepo === defaultRepo) { return { valid: true, error: null, qualifiedRepo }; diff --git a/actions/setup/js/repo_helpers.test.cjs b/actions/setup/js/repo_helpers.test.cjs index 53e5fe1c841..f6e58b83a38 100644 --- a/actions/setup/js/repo_helpers.test.cjs +++ b/actions/setup/js/repo_helpers.test.cjs @@ -234,6 +234,35 @@ describe("repo_helpers", () => { expect(result.error).toBe(null); }); + it('should allow any repo when defaultRepo is "*" (wildcard target-repo config)', async () => { + const { validateRepo } = await import("./repo_helpers.cjs"); + const result = validateRepo("org/any-repo", "*", new Set()); + expect(result.valid).toBe(true); + expect(result.error).toBe(null); + }); + + it('should allow any repo when defaultRepo is "*" regardless of allowed list', async () => { + const { validateRepo } = await import("./repo_helpers.cjs"); + const result = validateRepo("org/some-repo", "*", new Set(["org/other-repo"])); + expect(result.valid).toBe(true); + expect(result.error).toBe(null); + expect(result.qualifiedRepo).toBe("org/some-repo"); + }); + + it('should reject invalid slug when defaultRepo is "*" (no slash)', async () => { + const { validateRepo } = await import("./repo_helpers.cjs"); + const result = validateRepo("repo-only", "*", new Set()); + expect(result.valid).toBe(false); + expect(result.error).toContain("not a valid 'owner/repo' slug"); + }); + + it('should reject invalid slug when defaultRepo is "*" (too many slashes)', async () => { + const { validateRepo } = await import("./repo_helpers.cjs"); + const result = validateRepo("owner/repo/extra", "*", new Set()); + expect(result.valid).toBe(false); + expect(result.error).toContain("not a valid 'owner/repo' slug"); + }); + it('should allow org-scoped wildcard "github/*"', async () => { const { validateRepo } = await import("./repo_helpers.cjs"); const allowedRepos = new Set(["github/*"]); diff --git a/actions/setup/js/update_handler_factory.cjs b/actions/setup/js/update_handler_factory.cjs index 80e8a3ced33..eb5124b6ce7 100644 --- a/actions/setup/js/update_handler_factory.cjs +++ b/actions/setup/js/update_handler_factory.cjs @@ -149,19 +149,23 @@ function createUpdateHandlerFactory(handlerConfig) { const item = message; - // Resolve cross-repo target: if message has a "repo" field, validate it against - // the allowed repos and use it as the effective context. This enables updating items - // in a different repository when github-token is configured with the required permissions. + // Resolve cross-repo target: always validate the target repository against the + // allowed repos and use it as the effective context. When item.repo is set it + // overrides the default; otherwise defaultTargetRepo is used. This mirrors the + // add_comment routing behaviour and ensures target-repo config is honoured even + // when the agent does not explicitly provide a repo field. // Using {any} type to allow partial context override (effectiveContext.repo may differ from context.repo). + const repoResult = resolveAndValidateRepo(item, defaultTargetRepo, allowedRepos, itemTypeName); + if (!repoResult.success) { + core.warning(repoResult.error); + return { success: false, error: repoResult.error }; + } /** @type {any} */ - let effectiveContext = context; - if (item.repo) { - const repoResult = resolveAndValidateRepo(item, defaultTargetRepo, allowedRepos, itemTypeName); - if (!repoResult.success) { - core.warning(repoResult.error); - return { success: false, error: repoResult.error }; - } - effectiveContext = { ...context, repo: repoResult.repoParts }; + const effectiveContext = { ...context, repo: repoResult.repoParts }; + // Log cross-repo routing when the agent explicitly set a repo field, + // or when the resolved repo differs from the current workflow's repository. + const workflowRepo = `${context.repo.owner}/${context.repo.repo}`; + if (item.repo || repoResult.repo !== workflowRepo) { core.info(`Cross-repo update: targeting ${repoResult.repo}`); } diff --git a/actions/setup/js/update_handler_factory.test.cjs b/actions/setup/js/update_handler_factory.test.cjs index 7a2edfbfc71..e8a94e91d13 100644 --- a/actions/setup/js/update_handler_factory.test.cjs +++ b/actions/setup/js/update_handler_factory.test.cjs @@ -512,7 +512,7 @@ describe("update_handler_factory.cjs", () => { expect(capturedClient).toBe(mockGithub); }); - it("should pass the correct context.repo when no message.repo", async () => { + it("should route to target-repo when no message.repo is set", async () => { let capturedContext = null; const mockExecuteUpdate = vi.fn().mockImplementation(async (githubClient, context, num, data) => { capturedContext = context; @@ -530,11 +530,11 @@ describe("update_handler_factory.cjs", () => { }); const handler = await handlerFactory({ "target-repo": "owner/myrepo" }); - // Message without repo field — should use default context.repo + // Message without repo field — should route to the configured target-repo await handler({ title: "Test" }); - expect(capturedContext.repo.owner).toBe("testowner"); - expect(capturedContext.repo.repo).toBe("testrepo"); + expect(capturedContext.repo.owner).toBe("owner"); + expect(capturedContext.repo.repo).toBe("myrepo"); }); it("should route to message.repo when it matches the configured target-repo", async () => { @@ -616,6 +616,53 @@ describe("update_handler_factory.cjs", () => { expect(capturedUpdateData._workflowRepo.repo).toBe(mockContext.repo.repo); }); + it("should route to message.repo when target-repo is wildcard", async () => { + let capturedContext = null; + const mockExecuteUpdate = vi.fn().mockImplementation(async (githubClient, context, num, data) => { + capturedContext = context; + return { html_url: "https://example.com", title: "Updated" }; + }); + + const handlerFactory = factoryModule.createUpdateHandlerFactory({ + itemType: "update_test", + itemTypeName: "test item", + supportsPR: false, + resolveItemNumber: vi.fn().mockReturnValue({ success: true, number: 42 }), + buildUpdateData: vi.fn().mockReturnValue({ success: true, data: { title: "Test" } }), + executeUpdate: mockExecuteUpdate, + formatSuccessResult: vi.fn().mockReturnValue({ success: true }), + }); + + const handler = await handlerFactory({ "target-repo": "*" }); + // Message specifies a cross-repo target; wildcard allows any repo + await handler({ issue_number: 42, repo: "org/other-repo" }); + + expect(capturedContext.repo.owner).toBe("org"); + expect(capturedContext.repo.repo).toBe("other-repo"); + }); + + it("should fail when target-repo is wildcard and message has no repo", async () => { + const mockExecuteUpdate = vi.fn().mockResolvedValue({ html_url: "https://example.com", title: "Updated" }); + + const handlerFactory = factoryModule.createUpdateHandlerFactory({ + itemType: "update_test", + itemTypeName: "test item", + supportsPR: false, + resolveItemNumber: vi.fn().mockReturnValue({ success: true, number: 42 }), + buildUpdateData: vi.fn().mockReturnValue({ success: true, data: { title: "Test" } }), + executeUpdate: mockExecuteUpdate, + formatSuccessResult: vi.fn().mockReturnValue({ success: true }), + }); + + const handler = await handlerFactory({ "target-repo": "*" }); + // No repo field — must fail because "*" is not a valid repo slug + const result = await handler({ title: "Test" }); + + expect(result.success).toBe(false); + expect(result.error).toContain("Invalid repository format"); + expect(mockExecuteUpdate).not.toHaveBeenCalled(); + }); + it("should include body in log fields when _rawBody is present", async () => { const mockExecuteUpdate = vi.fn().mockResolvedValue({ html_url: "https://example.com", title: "Updated" }); diff --git a/pkg/workflow/safe_outputs_tools_generation_test.go b/pkg/workflow/safe_outputs_tools_generation_test.go index 263b46f8c7b..6cfc2813b7c 100644 --- a/pkg/workflow/safe_outputs_tools_generation_test.go +++ b/pkg/workflow/safe_outputs_tools_generation_test.go @@ -168,8 +168,40 @@ func TestAddRepoParameterIfNeededWildcardTargetRepo(t *testing.T) { assert.Contains(t, repoProp["description"].(string), "Any repository can be targeted", "description should indicate any repo allowed") } -// TestGenerateFilteredToolsJSONUpdateIssueWithWildcardTargetRepo tests that update_issue tool -// is generated in tools.json when target-repo is "*" (wildcard). +// TestAddRepoParameterIfNeededSpecificTargetRepoNoAllowedRepos tests that repo param is NOT added +// for update_issue when target-repo is a specific repo but allowed-repos is empty. +// The handler automatically routes to the configured target-repo, so the agent doesn't need to +// specify repo in the tool schema. +func TestAddRepoParameterIfNeededSpecificTargetRepoNoAllowedRepos(t *testing.T) { + tool := map[string]any{ + "name": "update_issue", + "inputSchema": map[string]any{ + "type": "object", + "properties": map[string]any{ + "title": map[string]any{"type": "string"}, + }, + }, + } + + safeOutputs := &SafeOutputsConfig{ + UpdateIssues: &UpdateIssuesConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{ + TargetRepoSlug: "org/target-repo", + }, + }, + }, + } + + addRepoParameterIfNeeded(tool, "update_issue", safeOutputs) + + inputSchema := tool["inputSchema"].(map[string]any) + properties := inputSchema["properties"].(map[string]any) + + _, hasRepo := properties["repo"] + assert.False(t, hasRepo, "repo parameter should NOT be added when target-repo is specific and no allowed-repos") +} + func TestGenerateFilteredToolsJSONUpdateIssueWithWildcardTargetRepo(t *testing.T) { data := &WorkflowData{ SafeOutputs: &SafeOutputsConfig{