Skip to content

Commit 9c93026

Browse files
Copilotpelikhan
Copilot
and
pelikhan
committed
fix: address review comments on resolve-pull-request-review-thread target config
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
1 parent 528d6ab commit 9c93026

3 files changed

Lines changed: 120 additions & 27 deletions

File tree

‎actions/setup/js/resolve_pr_review_thread.cjs‎

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ const { getErrorMessage } = require("./error_helpers.cjs");
99
const { getPRNumber } = require("./update_context_helpers.cjs");
1010
const { logStagedPreviewInfo } = require("./staged_preview.cjs");
1111
const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
12-
const { resolveTargetRepoConfig } = require("./repo_helpers.cjs");
12+
const { resolveTargetRepoConfig, validateTargetRepo } = require("./repo_helpers.cjs");
1313

1414
/**
1515
* Type constant for handler identification
@@ -21,7 +21,7 @@ const HANDLER_TYPE = "resolve_pull_request_review_thread";
2121
* Used to validate the thread before resolving.
2222
* @param {any} github - GitHub GraphQL instance
2323
* @param {string} threadId - Review thread node ID (e.g., 'PRRT_kwDOABCD...')
24-
* @returns {Promise<{prNumber: number, repoNameWithOwner: string}|null>} The PR number and repo, or null if not found
24+
* @returns {Promise<{prNumber: number, repoNameWithOwner: string|null}|null>} The PR number and repo, or null if not found
2525
*/
2626
async function getThreadPullRequestInfo(github, threadId) {
2727
const query = /* GraphQL */ `
@@ -92,9 +92,10 @@ async function main(config = {}) {
9292
const resolveTarget = config.target || "triggering";
9393
const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config);
9494

95-
// Normalize repo names to lowercase once for case-insensitive comparison during validation
96-
const normalizedDefaultTargetRepo = defaultTargetRepo ? defaultTargetRepo.toLowerCase() : null;
97-
const normalizedAllowedRepos = new Set(Array.from(allowedRepos).map(r => r.toLowerCase()));
95+
// Whether the user explicitly configured cross-repo targeting.
96+
// defaultTargetRepo always has a value (falls back to context.repo), so we check
97+
// the raw config keys to distinguish user-configured from default.
98+
const hasExplicitTargetConfig = !!(config["target-repo"] || config.allowed_repos?.length > 0);
9899

99100
const authClient = await createAuthenticatedGitHubClient(config);
100101

@@ -156,23 +157,25 @@ async function main(config = {}) {
156157

157158
const { prNumber: threadPRNumber, repoNameWithOwner: threadRepo } = threadInfo;
158159

159-
// When a target-repo or allowed-repos is configured, validate the thread's repository.
160+
// When the user explicitly configured target-repo or allowed-repos, validate the thread's
161+
// repository using validateTargetRepo (supports wildcards like "*", "org/*").
160162
// Otherwise, fall back to the legacy behavior of scoping to the triggering PR only.
161-
const hasTargetRepoConfig = defaultTargetRepo || allowedRepos.size > 0;
162-
163-
if (hasTargetRepoConfig) {
164-
// Cross-repo mode: validate thread repo against configured repos
165-
if (threadRepo) {
166-
const normalizedThreadRepo = threadRepo.toLowerCase();
167-
const isDefaultRepo = normalizedDefaultTargetRepo && normalizedThreadRepo === normalizedDefaultTargetRepo;
168-
const isAllowedRepo = isDefaultRepo || normalizedAllowedRepos.has(normalizedThreadRepo);
169-
if (!isAllowedRepo) {
170-
core.warning(`Thread ${threadId} belongs to repo ${threadRepo}, which is not in the allowed repos`);
171-
return {
172-
success: false,
173-
error: `Thread belongs to repo '${threadRepo}', but only threads in allowed repositories can be resolved. Allowed: ${defaultTargetRepo}${allowedRepos.size > 0 ? ", " + Array.from(allowedRepos).join(", ") : ""}`,
174-
};
175-
}
163+
if (hasExplicitTargetConfig) {
164+
// Cross-repo mode: validate thread repo against configured repos (fail closed if missing)
165+
if (!threadRepo) {
166+
core.warning(`Could not determine repository for thread ${threadId}`);
167+
return {
168+
success: false,
169+
error: `Could not determine the repository for thread ${threadId}`,
170+
};
171+
}
172+
const repoValidation = validateTargetRepo(threadRepo, defaultTargetRepo, allowedRepos);
173+
if (!repoValidation.valid) {
174+
core.warning(`Thread ${threadId} belongs to repo ${threadRepo}, which is not in the allowed repos`);
175+
return {
176+
success: false,
177+
error: repoValidation.error,
178+
};
176179
}
177180

178181
// Determine target PR number based on target config

‎actions/setup/js/resolve_pr_review_thread.test.cjs‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,99 @@ describe("resolve_pr_review_thread - cross-repo support", () => {
464464
expect(result.success).toBe(false);
465465
expect(result.error).toContain("pull request context");
466466
});
467+
468+
it("should allow resolving when allowed_repos uses '*' wildcard", async () => {
469+
mockGraphqlForThread(10, "any-owner/any-repo");
470+
471+
const { main } = require("./resolve_pr_review_thread.cjs");
472+
const freshHandler = await main({
473+
max: 10,
474+
"target-repo": "default-owner/default-repo",
475+
allowed_repos: ["*"],
476+
target: "*",
477+
});
478+
479+
const message = {
480+
type: "resolve_pull_request_review_thread",
481+
thread_id: "PRRT_kwDOWildcard",
482+
};
483+
484+
const result = await freshHandler(message, {});
485+
486+
expect(result.success).toBe(true);
487+
});
488+
489+
it("should allow resolving when allowed_repos uses org/* pattern", async () => {
490+
mockGraphqlForThread(10, "other-owner/specific-repo");
491+
492+
const { main } = require("./resolve_pr_review_thread.cjs");
493+
const freshHandler = await main({
494+
max: 10,
495+
"target-repo": "default-owner/default-repo",
496+
allowed_repos: ["other-owner/*"],
497+
target: "*",
498+
});
499+
500+
const message = {
501+
type: "resolve_pull_request_review_thread",
502+
thread_id: "PRRT_kwDOOrgWildcard",
503+
};
504+
505+
const result = await freshHandler(message, {});
506+
507+
expect(result.success).toBe(true);
508+
});
509+
510+
it("should reject resolving when org/* pattern does not match", async () => {
511+
mockGraphqlForThread(10, "wrong-owner/specific-repo");
512+
513+
const { main } = require("./resolve_pr_review_thread.cjs");
514+
const freshHandler = await main({
515+
max: 10,
516+
"target-repo": "default-owner/default-repo",
517+
allowed_repos: ["other-owner/*"],
518+
target: "*",
519+
});
520+
521+
const message = {
522+
type: "resolve_pull_request_review_thread",
523+
thread_id: "PRRT_kwDOOrgWildcard",
524+
};
525+
526+
const result = await freshHandler(message, {});
527+
528+
expect(result.success).toBe(false);
529+
expect(result.error).toContain("not in the allowed-repos list");
530+
});
531+
532+
it("should fail closed when threadRepo is null in cross-repo mode", async () => {
533+
// Simulate GraphQL returning a thread with no repository info
534+
mockGraphql.mockImplementation(query => {
535+
if (query.includes("resolveReviewThread")) {
536+
return Promise.resolve({ resolveReviewThread: { thread: { id: "PRRT_x", isResolved: true } } });
537+
}
538+
return Promise.resolve({
539+
node: { pullRequest: { number: 10, repository: null } },
540+
});
541+
});
542+
543+
const { main } = require("./resolve_pr_review_thread.cjs");
544+
const freshHandler = await main({
545+
max: 10,
546+
"target-repo": "other-owner/other-repo",
547+
target: "*",
548+
});
549+
550+
const message = {
551+
type: "resolve_pull_request_review_thread",
552+
thread_id: "PRRT_kwDONoRepo",
553+
};
554+
555+
const result = await freshHandler(message, {});
556+
557+
expect(result.success).toBe(false);
558+
expect(result.error).toContain("Could not determine");
559+
});
467560
});
468561

469562
describe("getPRNumber (shared helper in update_context_helpers)", () => {

‎pkg/workflow/resolve_pr_review_thread.go‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,11 @@ func (c *Compiler) parseResolvePullRequestReviewThreadConfig(outputMap map[strin
2626
// Parse common base fields with default max of 10
2727
c.parseBaseSafeOutputConfig(configMap, &config.BaseSafeOutputConfig, 10)
2828

29-
// Parse target config (target, target-repo, allowed-repos) with validation
30-
targetConfig, isInvalid := ParseTargetConfig(configMap)
31-
if isInvalid {
32-
return nil // Invalid configuration (e.g., wildcard target-repo), return nil to cause validation error
33-
}
29+
// Parse target config (target, target-repo, allowed-repos)
30+
targetConfig, _ := ParseTargetConfig(configMap)
3431
config.SafeOutputTargetConfig = targetConfig
3532

36-
resolvePRReviewThreadLog.Printf("Parsed resolve-pull-request-review-thread config: max=%d, target_repo=%s", config.Max, config.TargetRepoSlug)
33+
resolvePRReviewThreadLog.Printf("Parsed resolve-pull-request-review-thread config: max=%d, target_repo=%s", templatableIntValue(config.Max), config.TargetRepoSlug)
3734
} else {
3835
// If configData is nil or not a map, still set the default max
3936
config.Max = defaultIntStr(10)

0 commit comments

Comments
 (0)