Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds target, target-repo, and allowed-repos support to resolve-pull-request-review-thread so workflows can resolve review threads outside the triggering PR/repo (consistent with other safe-outputs).
Changes:
- Extend Go safe-output config parsing to include
SafeOutputTargetConfigforresolve-pull-request-review-thread. - Forward
target,target-repo, andallowed_reposinto the generated JS handler config. - Update JS handler + tests to fetch
repository.nameWithOwnervia GraphQL and validate cross-repo targeting.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pkg/workflow/resolve_pr_review_thread.go | Embed/parse target config for resolve thread safe-output config. |
| pkg/workflow/compiler_safe_outputs_config.go | Add target, target-repo, allowed_repos to handler config builder for this safe output. |
| actions/setup/js/resolve_pr_review_thread.cjs | Add repo-aware thread lookup and cross-repo validation logic. |
| actions/setup/js/resolve_pr_review_thread.test.cjs | Extend mocks + add cross-repo test cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Parse target config (target, target-repo, allowed-repos) with validation | ||
| targetConfig, isInvalid := ParseTargetConfig(configMap) | ||
| if isInvalid { | ||
| return nil // Invalid configuration (e.g., wildcard target-repo), return nil to cause validation error | ||
| } |
There was a problem hiding this comment.
ParseTargetConfig currently always returns isInvalid=false (and allows target-repo: "*" per its doc), so this isInvalid branch is dead code and the comment about wildcard being invalid is misleading. Either remove the check here or switch to a parser that can actually return a validation error for disallowed values.
| // Parse target config (target, target-repo, allowed-repos) with validation | |
| targetConfig, isInvalid := ParseTargetConfig(configMap) | |
| if isInvalid { | |
| return nil // Invalid configuration (e.g., wildcard target-repo), return nil to cause validation error | |
| } | |
| // Parse target config (target, target-repo, allowed-repos) | |
| targetConfig := ParseTargetConfig(configMap) |
There was a problem hiding this comment.
Fixed in 9c93026 — removed the isInvalid check and used _, _ pattern consistent with mark_pull_request_as_ready_for_review.go.
| // When a target-repo or allowed-repos is configured, validate the thread's repository. | ||
| // Otherwise, fall back to the legacy behavior of scoping to the triggering PR only. | ||
| const hasTargetRepoConfig = defaultTargetRepo || allowedRepos.size > 0; | ||
|
|
||
| if (hasTargetRepoConfig) { | ||
| // Cross-repo mode: validate thread repo against configured repos | ||
| if (threadRepo) { | ||
| const normalizedThreadRepo = threadRepo.toLowerCase(); | ||
| const isDefaultRepo = normalizedDefaultTargetRepo && normalizedThreadRepo === normalizedDefaultTargetRepo; | ||
| const isAllowedRepo = isDefaultRepo || normalizedAllowedRepos.has(normalizedThreadRepo); | ||
| if (!isAllowedRepo) { | ||
| core.warning(`Thread ${threadId} belongs to repo ${threadRepo}, which is not in the allowed repos`); | ||
| return { | ||
| success: false, | ||
| error: `Thread belongs to repo '${threadRepo}', but only threads in allowed repositories can be resolved. Allowed: ${defaultTargetRepo}${allowedRepos.size > 0 ? ", " + Array.from(allowedRepos).join(", ") : ""}`, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| // Determine target PR number based on target config | ||
| if (resolveTarget === "triggering") { | ||
| if (!triggeringPRNumber) { | ||
| core.warning("Cannot resolve review thread: not running in a pull request context"); | ||
| return { | ||
| success: false, | ||
| error: "Cannot resolve review threads outside of a pull request context", | ||
| }; | ||
| } | ||
| if (threadPRNumber !== triggeringPRNumber) { | ||
| core.warning(`Thread ${threadId} belongs to PR #${threadPRNumber}, not triggering PR #${triggeringPRNumber}`); | ||
| return { | ||
| success: false, | ||
| error: `Thread belongs to PR #${threadPRNumber}, but only threads on the triggering PR #${triggeringPRNumber} can be resolved`, | ||
| }; | ||
| } | ||
| } else if (resolveTarget !== "*") { | ||
| // Explicit PR number target | ||
| const targetPRNumber = parseInt(resolveTarget, 10); | ||
| if (Number.isNaN(targetPRNumber) || targetPRNumber <= 0) { | ||
| core.warning(`Invalid target PR number: '${resolveTarget}'`); | ||
| return { | ||
| success: false, | ||
| error: `Invalid target: '${resolveTarget}' - must be 'triggering', '*', or a positive integer`, | ||
| }; | ||
| } | ||
| if (threadPRNumber !== targetPRNumber) { | ||
| core.warning(`Thread ${threadId} belongs to PR #${threadPRNumber}, not target PR #${targetPRNumber}`); | ||
| return { | ||
| success: false, | ||
| error: `Thread belongs to PR #${threadPRNumber}, but target is PR #${targetPRNumber}`, | ||
| }; | ||
| } | ||
| } | ||
| // resolveTarget === "*": any PR in allowed repos — no further PR number check needed | ||
| } else { | ||
| // Default (legacy) mode: scope to triggering PR only |
There was a problem hiding this comment.
hasTargetRepoConfig is always truthy because defaultTargetRepo is always a non-empty string (it falls back to context.repo). That makes the else ("legacy mode") branch unreachable and the comment about only validating when configured inaccurate. Consider either removing the dead branch, or basing the condition on the presence of config keys (e.g. config["target-repo"] / config.allowed_repos) if you truly need a legacy path.
There was a problem hiding this comment.
Fixed in 9c93026 — replaced defaultTargetRepo || allowedRepos.size > 0 with hasExplicitTargetConfig, which checks config["target-repo"] and config.allowed_repos?.length > 0 directly on the raw config to detect user intent rather than the always-truthy resolved value.
| if (hasTargetRepoConfig) { | ||
| // Cross-repo mode: validate thread repo against configured repos | ||
| if (threadRepo) { | ||
| const normalizedThreadRepo = threadRepo.toLowerCase(); | ||
| const isDefaultRepo = normalizedDefaultTargetRepo && normalizedThreadRepo === normalizedDefaultTargetRepo; | ||
| const isAllowedRepo = isDefaultRepo || normalizedAllowedRepos.has(normalizedThreadRepo); | ||
| if (!isAllowedRepo) { | ||
| core.warning(`Thread ${threadId} belongs to repo ${threadRepo}, which is not in the allowed repos`); | ||
| return { | ||
| success: false, | ||
| error: `Thread belongs to repo '${threadRepo}', but only threads in allowed repositories can be resolved. Allowed: ${defaultTargetRepo}${allowedRepos.size > 0 ? ", " + Array.from(allowedRepos).join(", ") : ""}`, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The repo allowlist check here only supports exact matches (Set.has) and will not honor the wildcard patterns (*, org/*, */repo, etc.) supported elsewhere via repo_helpers.validateRepo/isRepoAllowed. This breaks existing allowed_repos semantics and makes configs like allowed_repos: ["*"] ineffective. Reuse validateTargetRepo/validateRepo from repo_helpers.cjs for the threadRepo validation (and fail closed if threadRepo is unexpectedly missing).
There was a problem hiding this comment.
Fixed in 9c93026 — replaced the custom exact-match logic with validateTargetRepo(threadRepo, defaultTargetRepo, allowedRepos) from repo_helpers.cjs, which supports wildcards (*, org/*, etc.). Also added fail-closed behavior when threadRepo is null.
| @@ -37,7 +41,15 @@ async function getThreadPullRequestNumber(github, threadId) { | |||
|
|
|||
| const result = await github.graphql(query, { threadId }); | |||
|
|
|||
| return result?.node?.pullRequest?.number ?? null; | |||
| const pullRequest = result?.node?.pullRequest; | |||
| if (!pullRequest) { | |||
| return null; | |||
| } | |||
|
|
|||
| return { | |||
| prNumber: pullRequest.number, | |||
| repoNameWithOwner: pullRequest.repository?.nameWithOwner ?? null, | |||
| }; | |||
There was a problem hiding this comment.
The JSDoc says repoNameWithOwner is a string, but the function can return null (pullRequest.repository?.nameWithOwner ?? null). Update the return type to string|null (and/or enforce non-null if you expect this field to always be present) to keep the docs consistent with runtime behavior.
There was a problem hiding this comment.
Fixed in 9c93026 — updated the JSDoc return type to {prNumber: number, repoNameWithOwner: string|null}|null.
| describe("resolve_pr_review_thread - cross-repo support", () => { | ||
| const originalPayload = mockContext.payload; | ||
|
|
||
| beforeEach(() => { | ||
| vi.resetModules(); | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| global.context.payload = originalPayload; | ||
| }); | ||
|
|
||
| it("should allow resolving a thread in target-repo when configured", async () => { | ||
| mockGraphqlForThread(10, "other-owner/other-repo"); | ||
|
|
||
| const { main } = require("./resolve_pr_review_thread.cjs"); | ||
| const freshHandler = await main({ | ||
| max: 10, | ||
| "target-repo": "other-owner/other-repo", | ||
| target: "*", | ||
| }); | ||
|
|
||
| const message = { | ||
| type: "resolve_pull_request_review_thread", | ||
| thread_id: "PRRT_kwDOCrossRepo", | ||
| }; | ||
|
|
||
| const result = await freshHandler(message, {}); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.thread_id).toBe("PRRT_kwDOCrossRepo"); | ||
| expect(result.is_resolved).toBe(true); | ||
| }); | ||
|
|
||
| it("should reject a thread whose repo is not in allowed-repos", async () => { | ||
| mockGraphqlForThread(10, "other-owner/other-repo"); | ||
|
|
||
| const { main } = require("./resolve_pr_review_thread.cjs"); | ||
| const freshHandler = await main({ | ||
| max: 10, | ||
| "target-repo": "allowed-owner/allowed-repo", | ||
| target: "*", | ||
| }); | ||
|
|
||
| const message = { | ||
| type: "resolve_pull_request_review_thread", | ||
| thread_id: "PRRT_kwDOCrossRepo", | ||
| }; | ||
|
|
||
| const result = await freshHandler(message, {}); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.error).toContain("other-owner/other-repo"); | ||
| expect(result.error).toContain("allowed"); | ||
| }); | ||
|
|
||
| it("should allow cross-repo thread in allowed_repos list", async () => { | ||
| mockGraphqlForThread(10, "extra-owner/extra-repo"); | ||
|
|
||
| const { main } = require("./resolve_pr_review_thread.cjs"); | ||
| const freshHandler = await main({ | ||
| max: 10, | ||
| "target-repo": "allowed-owner/allowed-repo", | ||
| allowed_repos: ["extra-owner/extra-repo"], | ||
| target: "*", | ||
| }); |
There was a problem hiding this comment.
Cross-repo tests cover exact repo matches, but there are no assertions for the wildcard/pattern allowlist behavior that other handlers support (e.g. allowed_repos: ["*"] or allowed_repos: ["other-owner/*"]). Adding at least one such test would prevent regressions and would have caught the current exact-match-only validation logic.
There was a problem hiding this comment.
Fixed in 9c93026 — added tests for allowed_repos: ["*"] wildcard, allowed_repos: ["org/*"] pattern (match and non-match), and the fail-closed case when threadRepo is null in cross-repo mode.
| } | ||
| config.SafeOutputTargetConfig = targetConfig | ||
|
|
||
| resolvePRReviewThreadLog.Printf("Parsed resolve-pull-request-review-thread config: max=%d, target_repo=%s", config.Max, config.TargetRepoSlug) |
There was a problem hiding this comment.
config.Max is a *string (templatable int), but this log uses %d, which will print %!d(*string=...) at runtime. Use %v/%s with a nil-safe dereference (or log templatableIntValue(config.Max) if you specifically want the numeric value).
| resolvePRReviewThreadLog.Printf("Parsed resolve-pull-request-review-thread config: max=%d, target_repo=%s", config.Max, config.TargetRepoSlug) | |
| maxVal := "" | |
| if config.Max != nil { | |
| maxVal = *config.Max | |
| } | |
| resolvePRReviewThreadLog.Printf("Parsed resolve-pull-request-review-thread config: max=%s, target_repo=%s", maxVal, config.TargetRepoSlug) |
There was a problem hiding this comment.
Fixed in 9c93026 — replaced %d with templatableIntValue(config.Max) which extracts the integer value from the *string field.
…rget config Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The CI failure in that run ( Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
SafeOutputTargetConfigtoResolvePullRequestReviewThreadConfiginpkg/workflow/resolve_pr_review_thread.goparseResolvePullRequestReviewThreadConfigto parsetarget,target-repo, andallowed-reposcompiler_safe_outputs_config.goto includetarget,target-repo,allowed_reposin config generationactions/setup/js/resolve_pr_review_thread.cjsto support cross-repo thread resolution withresolveTargetRepoConfigisInvalidbranch (ParseTargetConfig always returns false) — use_, _pattern%dformat verb used on*stringMax field — usetemplatableIntValue(config.Max)hasExplicitTargetConfigto check raw config keys (config["target-repo"],config.allowed_repos) instead of always-truthydefaultTargetRepovalidateTargetRepofrom repo_helpers.cjs for wildcard-aware repo validation (supports*,org/*, etc.)repoNameWithOwner: string|nullthreadRepois null in cross-repo mode*,org/*, non-matching pattern)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.