Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/patch-fix-cross-repo-update-issue.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions actions/setup/js/repo_helpers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wildcard check is placed correctly before the default repo check, which ensures "*" bypasses all other validation. Consider documenting the implication: when target-repo is "*", the agent must always supply a repo field — otherwise resolveAndValidateRepo will receive "*" as the repo string and emit an "Invalid repository format" error (verified by the new test in update_handler_factory.test.cjs). A short JSDoc comment here would make that contract explicit.

}

// Default repo is always allowed
if (qualifiedRepo === defaultRepo) {
return { valid: true, error: null, qualifiedRepo };
Expand Down
29 changes: 29 additions & 0 deletions actions/setup/js/repo_helpers.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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/*"]);
Expand Down
26 changes: 15 additions & 11 deletions actions/setup/js/update_handler_factory.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix — resolveAndValidateRepo is now always called regardless of whether item.repo is set, which ensures the configured target-repo is honoured unconditionally. The updated test "should route to target-repo when no message.repo is set" captures this behavior clearly.

// 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}`);
}

Expand Down
55 changes: 51 additions & 4 deletions actions/setup/js/update_handler_factory.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@
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;
Expand All @@ -530,11 +530,11 @@
});

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 () => {
Expand Down Expand Up @@ -616,6 +616,53 @@
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");

Check failure on line 662 in actions/setup/js/update_handler_factory.test.cjs

View workflow job for this annotation

GitHub Actions / js

update_handler_factory.test.cjs > update_handler_factory.cjs > authentication: github-token and cross-repo routing > should fail when target-repo is wildcard and message has no repo

AssertionError: expected 'Repository \'*\' is not a valid \'own…' to contain 'Invalid repository format' Expected: "Invalid repository format" Received: "Repository '*' is not a valid 'owner/repo' slug." ❯ update_handler_factory.test.cjs:662:28
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" });

Expand Down
36 changes: 34 additions & 2 deletions pkg/workflow/safe_outputs_tools_generation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
Loading