Fix missing cross-repo and auth properties in safe output schemas#18754
Open
Fix missing cross-repo and auth properties in safe output schemas#18754
Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
… schema properties - Add github-token to add-comment, create-issue, create-discussion schemas - Add target-repo and allowed-repos to create-code-scanning-alert and push-to-pull-request-branch schemas - Add allowed-repos and github-token to update-issue schema - Add runner and agent-output to custom safe-outputs/jobs schema - Handle runner (alias for runs-on) and agent-output (alias for output) in safe_jobs.go Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix safe outputs to support target-repo and allowed-repos
Fix missing cross-repo and auth properties in safe output schemas
Feb 27, 2026
dsyme
reviewed
Feb 27, 2026
…ep auth, add comprehensive tests Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
…nce, fix assert.Emptyf Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. 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:
|
dsyme
reviewed
Feb 28, 2026
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. 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:
|
dsyme
force-pushed
the
copilot/fix-safe-outputs-target-repo-support
branch
from
93c725f to
bbc11a3
Compare
dsyme
reviewed
Feb 28, 2026
…oss handlers - Create handler_auth.cjs with createAuthenticatedGitHubClient(config) helper - Update add_comment.cjs, create_issue.cjs, create_discussion.cjs to use authClient instead of global github (no global mutation for cross-repo auth) - Update update_handler_factory.cjs with authClient and cross-repo routing via message.repo - Add handler_auth.test.cjs with 6 tests for the shared auth helper - Add 4 tests to update_handler_factory.test.cjs covering auth and cross-repo routing - Update TestHandlerManagerStepPerOutputTokenInHandlerConfig to be precise about where per-output tokens appear (handler config JSON, NOT step-level github-token) - Fix create_issue_group.test.cjs to pass github client to searchForExistingParent Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. 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:
|
…tput handlers - Add github-token passthrough to Go handler registry for 22 handlers: add_labels, remove_labels, add_reviewer, assign_milestone, mark_pull_request_as_ready_for_review, update_discussion, link_sub_issue, update_release, create_pull_request_review_comment, submit_pull_request_review, reply_to_pull_request_review_comment, resolve_pull_request_review_thread, create_pull_request, push_to_pull_request_branch, update_pull_request, close_pull_request, hide_comment, dispatch_workflow, missing_tool, missing_data, assign_to_user, unassign_from_user - Update all corresponding .cjs handler files to use createAuthenticatedGitHubClient: replaces direct global github.rest.* and github.graphql() calls with authClient equivalents, falling back to global github when no token set - Also update close_issue.cjs and close_discussion.cjs with the same pattern (no registry change needed yet as their schemas lack github-token) - Fix submit_pr_review.test.cjs to set global.github mock in beforeEach so tests don't fail after other tests delete global.github Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. 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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes safe-output schema + compiler/handler mismatches that prevented cross-repository workflows from compiling and running correctly, primarily around target-repo, allowed-repos, and github-token, and adds consistent per-handler authentication support in the JavaScript handlers.
Changes:
- Extends workflow JSON schemas and Go safe-output parsing/structs to accept and populate cross-repo and auth fields (incl.
runner/agent-outputaliases for custom safe jobs). - Wires
github-token,target-repo, andallowed_reposinto the handler-manager config JSON so JS handlers receive the expected runtime config. - Introduces a shared JS auth helper (
handler_auth.cjs) and updates GitHub-calling handlers (plus tests) to use authenticated clients consistently.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_cross_repo_config_test.go | Adds Go tests covering cross-repo parsing and handler-manager config wiring. |
| pkg/workflow/safe_output_parser.go | Ensures target config parsing also captures allowed-repos. |
| pkg/workflow/safe_jobs.go | Adds aliases runner→runs-on and agent-output→output for safe jobs. |
| pkg/workflow/push_to_pull_request_branch.go | Adds struct fields + parsing for target-repo / allowed-repos. |
| pkg/workflow/create_code_scanning_alert.go | Adds struct fields + parsing for target-repo / allowed-repos. |
| pkg/workflow/config_helpers.go | Introduces parseAllowedReposFromConfig helper. |
| pkg/workflow/compiler_safe_outputs_steps.go | Refactors project token resolution and clarifies handler-manager step token precedence. |
| pkg/workflow/compiler_safe_outputs_job.go | Removes/updates stale comments in consolidated job output wiring. |
| pkg/workflow/compiler_safe_outputs_config.go | Adds github-token and cross-repo fields into handler config registry output. |
| pkg/parser/schemas/main_workflow_schema.json | Updates safe-output schemas to accept missing properties and safe-jobs aliases. |
| actions/setup/js/update_release.cjs | Uses shared auth helper for GitHub API calls. |
| actions/setup/js/update_handler_factory.test.cjs | Adds tests for auth client selection and cross-repo routing via message.repo. |
| actions/setup/js/update_handler_factory.cjs | Adds shared auth + cross-repo routing support (effective context + validated repo). |
| actions/setup/js/unassign_from_user.cjs | Uses shared auth helper for API calls. |
| actions/setup/js/submit_pr_review.test.cjs | Ensures global.github mock is reset per test to support auth changes. |
| actions/setup/js/submit_pr_review.cjs | Uses shared auth helper for PR fetching. |
| actions/setup/js/resolve_pr_review_thread.cjs | Uses shared auth helper for API/GraphQL calls. |
| actions/setup/js/reply_to_pr_review_comment.cjs | Uses shared auth helper for API calls. |
| actions/setup/js/remove_labels.cjs | Uses shared auth helper for API calls. |
| actions/setup/js/push_to_pull_request_branch.cjs | Uses shared auth helper for API calls. |
| actions/setup/js/mark_pull_request_as_ready_for_review.cjs | Uses shared auth helper for API calls. |
| actions/setup/js/link_sub_issue.cjs | Uses shared auth helper for REST/GraphQL calls. |
| actions/setup/js/hide_comment.cjs | Uses shared auth helper for API calls. |
| actions/setup/js/handler_auth.test.cjs | Adds unit tests for shared auth helper behavior. |
| actions/setup/js/handler_auth.cjs | New shared helper to create per-handler authenticated Octokit clients. |
| actions/setup/js/dispatch_workflow.cjs | Uses shared auth helper for API calls. |
| actions/setup/js/create_pull_request.cjs | Uses shared auth helper for REST/GraphQL calls. |
| actions/setup/js/create_pr_review_comment.cjs | Uses shared auth helper for PR fetching. |
| actions/setup/js/create_issue_group.test.cjs | Updates tests to pass GitHub client explicitly to helper. |
| actions/setup/js/create_issue.cjs | Uses shared auth helper; threads client into group-parent helpers and API/GraphQL calls. |
| actions/setup/js/create_discussion.cjs | Uses shared auth helper; threads client into helper functions and GraphQL calls. |
| actions/setup/js/close_pull_request.cjs | Uses shared auth helper for API calls. |
| actions/setup/js/close_issue.cjs | Uses shared auth helper for API calls. |
| actions/setup/js/close_discussion.cjs | Uses shared auth helper for API/GraphQL calls. |
| actions/setup/js/assign_to_user.cjs | Uses shared auth helper for API calls. |
| actions/setup/js/assign_milestone.cjs | Uses shared auth helper for API calls. |
| actions/setup/js/add_reviewer.cjs | Uses shared auth helper for API calls. |
| actions/setup/js/add_labels.cjs | Uses shared auth helper for API calls. |
| actions/setup/js/add_comment.cjs | Uses shared auth helper for REST/GraphQL calls and comment-hiding helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
378
to
379
| // If no project token, fall back to per-output tokens from handler types that support github-token. | ||
| steps = append(steps, " with:\n") |
There was a problem hiding this comment.
The comment // If no project token, fall back to per-output tokens from handler types that support github-token. is inaccurate: the code below explicitly does not fall back to per-output tokens and instead prefers the safe-outputs-level token, then magic secrets. Please update/remove this line so the documentation matches the implemented precedence and avoids confusion when debugging auth behavior.
Contributor
Author
There was a problem hiding this comment.
Already addressed in commit aae9308 — the inaccurate comment was removed.
Comment on lines
167
to
171
| // parseAllowedReposFromConfig extracts and validates allowed-repos from a config map. | ||
| // Returns a slice of repository slugs in "owner/repo" format, or nil if not present or invalid. | ||
| func parseAllowedReposFromConfig(configMap map[string]any) []string { | ||
| return ParseStringArrayFromConfig(configMap, "allowed-repos", configHelpersLog) | ||
| } |
There was a problem hiding this comment.
The doc comment for parseAllowedReposFromConfig says it returns nil when the key is invalid, but it delegates to ParseStringArrayFromConfig, which returns an empty slice ([]string{}) when the key exists but no valid strings are found (see config_helpers.go:62-68). Please adjust the comment to reflect the actual return contract (e.g., nil when not present, empty slice when present-but-empty/invalid), or change the implementation if nil is required by callers.
Contributor
Author
There was a problem hiding this comment.
Fixed in commit aae9308. The doc comment now accurately describes the return contract:
nilwhen the key is not present or the value is not a valid array type- empty slice when the key exists but contains no valid strings
Comment on lines
423
to
429
| // TestHandlerManagerStepUsesPerOutputToken verifies that the handler manager step | ||
| // uses the per-output github-token when no global safe-outputs token is set. | ||
| // TestHandlerManagerStepPerOutputTokenInHandlerConfig verifies that per-output tokens | ||
| // (e.g., add-comment.github-token) are wired into the handler config JSON (GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG) | ||
| // but NOT used as the step-level with.github-token. The step-level token follows the same | ||
| // precedence as github_token.go: project token > global safe-outputs token > magic secrets. | ||
| func TestHandlerManagerStepPerOutputTokenInHandlerConfig(t *testing.T) { |
There was a problem hiding this comment.
The leading comments above TestHandlerManagerStepPerOutputTokenInHandlerConfig are contradictory/outdated: they state the handler manager step "uses the per-output github-token" when no global token is set, but the test assertions below verify the opposite (per-output token must NOT be used as the step-level with.github-token). Please update/remove the stale comment so it matches the intended precedence documented in the PR description and in the test itself.
Contributor
Author
There was a problem hiding this comment.
Already addressed in commit aae9308 — the stale contradictory comment was removed.
Contributor
|
project token > global safe-outputs.github-token > magic secrets (GH_AW_GITHUB_TOKEN || GITHUB_TOKEN) project token is only for token, and should not be the default of other safe outputs Make sure to run smoke on this (after merge the fix from main) |
pelikhan
reviewed
Feb 28, 2026
Contributor
|
@Copilotfix CI failures and make sure actions/setup/js/handler_auth.cjs is always installed since more or less every safe output handler needs it |
Contributor
|
We should have a dedicated GH_AW_SAFE_OUTPUTS_GITHUB_TOKEN magic secret |
…h.cjs installation - Remove inaccurate 'fall back to per-output tokens' comment from compiler_safe_outputs_steps.go (the code explicitly does NOT do this) - Fix parseAllowedReposFromConfig doc comment to accurately describe nil (key not present) vs empty slice (key present but no valid strings) return contract - Remove stale 'TestHandlerManagerStepUsesPerOutputToken' text from test comment that contradicted the actual test assertions - Add handler_auth.cjs to SAFE_OUTPUTS_FILES in setup.sh so it is explicitly validated and installed - nearly every safe output handler requires it via createAuthenticatedGitHubClient Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
Contributor
Author
Fixed in commit 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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TargetRepoSlug/AllowedReposfields added)github-token) for all 31 handler typesrunner/agent-outputalias support insafe_jobs.goparseAllowedReposFromConfighelper +ParseTargetConfigfixbuildHandlerManagerStephandler_auth.cjs— sharedcreateAuthenticatedGitHubClient(config)helpercreateAuthenticatedGitHubClienthandler_auth.cjsadded to SAFE_OUTPUTS_FILES insetup.shfor explicit validation/installationbuildHandlerManagerStepparseAllowedReposFromConfigdoc comment to accurately describe nil vs empty slice return contractsafe_outputs_cross_repo_config_test.goOriginal prompt
This section details on the original issue you should resolve
<issue_title>Many safe outputs don't support either
target-repoorallowed-repos</issue_title><issue_description>
Many safe outputs don't support either
target-repoorallowed-reposThe safe outputs involved can be inferred from the name of the workflow test (e.g. add-comment)
We should fix all these systematically