fix: allow wildcard target-repo: "*" in safe-output handlers#21877
fix: allow wildcard target-repo: "*" in safe-output handlers#21877
target-repo: "*" in safe-output handlers#21877Conversation
…sue) The parseCommentsConfig function incorrectly called validateTargetRepoSlug which rejected target-repo: \"*\" as invalid, returning nil. This caused the add_comment handler to be absent from GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG. The wildcard \"*\" is a valid target-repo for add-comment (means \"any repository\"), consistent with ParseTargetConfig used by update-issue and similar handlers. - Remove validateTargetRepoSlug call from parseCommentsConfig - Update tests to reflect that target-repo: \"*\" is now correctly accepted Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/35f1e5f0-7dc0-4972-b5aa-b63ed8fdbe6b
target-repo: "*" in add-comment safe-outputs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Note
🔒 Integrity filtering filtered 2 items
Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
- pr:#21877 (
pull_request_read: Resource 'pr:#21877' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.) - pr:#21876 (
pull_request_read: Resource 'pr:#21876' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)
💥 [THE END] — Illustrated by Smoke Claude
| shouldBeNil: false, // Wildcard "*" is a valid target-repo for add-comment | ||
| }, | ||
| { | ||
| name: "target-repo without target field", |
There was a problem hiding this comment.
Good change — the test name update from "should be rejected" to "is allowed" correctly reflects the new intended behavior that wildcard * is a valid target-repo value for add-comment safe-outputs.
| t.Errorf("expected TargetRepoSlug to be \"*\", got %q", result.TargetRepoSlug) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Nice improvement to the test assertions — checking TargetRepoSlug == "*" is much more precise than just checking for non-nil. This ensures the wildcard value is correctly preserved through config parsing.
|
@copilot look for other instances of this bug |
There was a problem hiding this comment.
Pull request overview
Fixes safe-outputs config compilation so safe-outputs.add-comment.target-repo: "*" is preserved (instead of dropping the entire add_comment handler), making the tool available to the agent at runtime.
Changes:
- Removed
add-comment’s wildcard rejection fortarget-repo: "*"so the handler is still registered. - Updated unit tests to treat wildcard
target-repoas valid foradd-comment. - Added an integration test to ensure the compiled
GH_AW_SAFE_OUTPUTS_HANDLER_CONFIGJSON includestarget-repo: "*".
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/workflow/add_comment.go | Stops rejecting target-repo: "*" during add-comment config parsing. |
| pkg/workflow/add_comment_target_repo_test.go | Updates unit coverage to assert wildcard target-repo is accepted. |
| pkg/workflow/add_comment_target_repo_integration_test.go | Adds integration coverage to ensure compiled handler config contains wildcard target-repo. |
| pkg/workflow/config_parsing_helpers_test.go | Updates helper-level parsing test to expect non-nil config for wildcard add-comment.target-repo. |
Comments suppressed due to low confidence (1)
pkg/workflow/add_comment.go:74
- Now that add-comment allows wildcard target-repo values (""), the field comment/docstring for TargetRepoSlug should be updated to mention that "" is also valid (meaning any repository). Otherwise the struct documentation is misleading for users and future maintainers.
// Validate discussion field - must be true if present
if config.Discussion != nil && !*config.Discussion {
addCommentLog.Print("Invalid discussion: must be true if present")
return nil // Invalid configuration, return nil to cause validation error
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result := compiler.parseCommentsConfig(outputMap) | ||
| if result != nil { | ||
| t.Errorf("expected nil for wildcard target-repo, got %+v", result) | ||
| if result == nil { | ||
| t.Errorf("expected non-nil config for wildcard target-repo, got nil") | ||
| } else if result.TargetRepoSlug != "*" { | ||
| t.Errorf("expected TargetRepoSlug to be \"*\", got %q", result.TargetRepoSlug) |
There was a problem hiding this comment.
The section header comment says these are wildcard validation tests that should return nil for invalid config, but add-comment wildcard is now expected to be valid. Please update or narrow that comment so it doesn’t contradict the updated add-comment test case.
… handlers Same bug as add-comment: validateTargetRepoSlug silently dropped the entire handler from GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG when target-repo: \"*\" was set. Affected handlers fixed: - create-issue - create-discussion - close-issue, close-discussion, close-pull-request (via shared helper) - add-reviewer - create-pull-request Remove now-dead validateTargetRepoSlug function from validation_helpers.go. PR-specific handlers (create-pr-review-comment, submit-pr-review, etc.) still use parseTargetRepoWithValidation which intentionally rejects wildcards. Update tests to expect non-nil configs for wildcard target-repo. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/11066158-a231-4c1b-ad26-84217b8864d8
Found 5 more instances of the same bug — all handlers using
Fixed in 6937e72 by removing all 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:
|
target-repo: "*" in add-comment safe-outputstarget-repo: "*" in safe-output handlers
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
Agent Container Tool Check
Result: 12/12 tools available ✅ — PASS
|
|
🎭 The smoke test agent was here! Run §23326705010 results:
Overall: PR Author: Note 🔒 Integrity filtering filtered 1 itemIntegrity filtering activated and filtered the following item during workflow execution.
|
There was a problem hiding this comment.
This PR correctly removes the validateTargetRepoSlug validation that was unnecessarily blocking wildcard * for target-repo. The change is well-tested with updated unit and integration tests. Smoke test approved! 🤖
Note
🔒 Integrity filtering filtered 1 item
Integrity filtering activated and filtered the following item during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
📰 BREAKING: Report filed by Smoke Copilot
| @@ -104,19 +104,6 @@ func formatList(items []string) string { | |||
| return fmt.Sprintf("%s, and %s", formatList(items[:len(items)-1]), items[len(items)-1]) | |||
There was a problem hiding this comment.
The removal of validateTargetRepoSlug is intentional here — the wildcard * is now a valid value for target-repo. The validation that was preventing this is correctly removed, and the wildcard handling is now deferred to the runtime handler config. Good change! 🎯
| return nil // Invalid configuration, return nil to cause validation error | ||
| } | ||
|
|
||
| // Validate discussion field - must be true if present |
There was a problem hiding this comment.
Now that the target-repo: "*" wildcard is allowed, the handler config correctly receives and passes the wildcard value. The existing discussion field validation below still works correctly. ✅
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Commit pushed:
|
Smoke Test ResultsMerged PRs: #21893 Fix MCP Gateway failure: default Warning
|
|
Smoke Test Run 23326704978 — PARTIAL ✅ Core (#1–11): ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ Note 🔒 Integrity filtering filtered 1 itemIntegrity filtering activated and filtered the following item during workflow execution.
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal! Reviewed wildcard target-repo changes in add_comment, add_reviewer, create_issue, create_pull_request, create_discussion, and close_entity_helpers. The removal of validateTargetRepoSlug is well-tested across both unit and integration levels.
Note
🔒 Integrity filtering filtered 1 item
Integrity filtering activated and filtered the following item during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
💥 [THE END] — Illustrated by Smoke Claude
| "target": "*", | ||
| "target-repo": "*", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Good addition of a test case for the wildcard target-repo: "*" scenario. This ensures the change is properly validated at the integration level. Consider also testing with an actual mock comment to verify the end-to-end flow works with the wildcard target.
| @@ -104,19 +104,6 @@ func formatList(items []string) string { | |||
| return fmt.Sprintf("%s, and %s", formatList(items[:len(items)-1]), items[len(items)-1]) | |||
There was a problem hiding this comment.
The removal of validateTargetRepoSlug is clean. Since wildcard "*" is now a valid value for target-repo, this validation was preventing a legitimate use case. The unit tests in config_parsing_helpers_test.go properly cover the new behavior.
|
Hey The PR is in excellent shape — focused on a single root cause, thoroughly documented with root-cause analysis and affected-handler enumeration, comprehensive test coverage (unit + integration), and a proper changeset entry. This looks ready for maintainer review. 🚀
|
|
@copilot review the JavaScript handlers and make sure they can handle * keyword, add tests |
Verify that JavaScript handlers correctly process target-repo: \"*\" at runtime: - repo_helpers.test.cjs: add resolveTargetRepoConfig wildcard passthrough test and resolveAndValidateRepo wildcard behavior tests - create_issue.test.cjs: add wildcard target-repo tests in repository targeting - close_issue.test.cjs: add wildcard target-repo close test - create_pull_request.test.cjs: add wildcard target-repo describe block with tests - create_discussion_labels.test.cjs: add wildcard target-repo cross-repo discussion test - add_comment.test.cjs: add wildcard target-repo cross-repo comment test The JS handlers already support target-repo: \"*\" via resolveTargetRepoConfig / resolveAndValidateRepo in repo_helpers.cjs (validateRepo already handles defaultRepo === \"*\" since an earlier change). These tests confirm end-to-end handler behavior with the wildcard config. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/495db36c-7128-407a-be46-fc3505faa9bf
Reviewed the JavaScript handlers. All handlers that accept Added tests in d645442 to confirm end-to-end behavior:
|
When
target-repo: "*"was set in asafe-outputshandler, the compiledGH_AW_SAFE_OUTPUTS_HANDLER_CONFIGsilently omitted that handler entirely, making the tool unavailable to the agent at runtime.Root cause
Multiple handler parsers called
validateTargetRepoSlugwhich unconditionally rejects"*", returningnil. This caused the affected handler entry to never appear in the handler registry output. Wildcard"*"is already valid fortarget-repoinupdate-issue,update-discussion, and other handlers viaParseTargetConfig, and the JavaScript runtime (repo_helpers.cjs) already supportstarget-repo: "*"for all handler types viaresolveTargetRepoConfig/resolveAndValidateRepo.Affected handlers
The same bug was present in all handlers using
validateTargetRepoSlug:add-commentcreate-issuecreate-discussionclose-issue,close-discussion,close-pull-request(via shared helper)add-reviewercreate-pull-requestPR-specific handlers (
create-pull-request-review-comment,submit-pull-request-review,reply-to-pull-request-review-comment) use a separateparseTargetRepoWithValidationthat intentionally rejects wildcards and were left unchanged.Changes
Go (compiler)
add_comment.go: RemovevalidateTargetRepoSlugcall fromparseCommentsConfigcreate_issue.go: RemovevalidateTargetRepoSlugcallcreate_discussion.go: RemovevalidateTargetRepoSlugcallclose_entity_helpers.go: RemovevalidateTargetRepoSlugcall from shared close entity parseradd_reviewer.go: RemovevalidateTargetRepoSlugcallcreate_pull_request.go: RemovevalidateTargetRepoSlugcallvalidation_helpers.go: Remove now-deadvalidateTargetRepoSlugfunctionadd_comment_target_repo_test.go: Update test case that expectednilon wildcard to assert correct non-nil configadd_comment_target_repo_integration_test.go: Add test case verifyingtarget-repo: "*"appears in compiled handler config JSONconfig_parsing_helpers_test.go: Update wildcard target-repo tests foradd-comment,create-issue,create-discussion, andcreate-pull-requestto expect valid configs rather thannilJavaScript (runtime)
The JS handlers already supported
target-repo: "*"viavalidateRepo'sdefaultRepo === "*"branch. New tests confirm end-to-end behavior:repo_helpers.test.cjs:resolveTargetRepoConfigwildcard passthrough +resolveAndValidateRepowildcard behaviorcreate_issue.test.cjs: wildcard target-repo issue creation and invalid slug rejectionclose_issue.test.cjs: wildcard target-repo issue closingcreate_pull_request.test.cjs: wildcard target-repo PR creation and invalid slug rejectioncreate_discussion_labels.test.cjs: wildcard target-repo discussion creationadd_comment.test.cjs: wildcard target-repo comment posting to cross-repoExample
Now correctly produces each handler entry in
GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG, and the JS handlers accept any validowner/reposlug in messages at runtime.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
✨ PR Review Safe Output Test - Run 23324229844
Note
🔒 Integrity filtering filtered 2 items
Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
target-repo: "*"in safe-output handlers #21877 (pull_request_read: Resource 'pr:fix: allow wildcardtarget-repo: "*"in safe-output handlers #21877' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)pull_request_read: Resource 'pr:[docs] docs: remove redundant content from templating.md #21876' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)✨ PR Review Safe Output Test - Run 23326704978
Note
🔒 Integrity filtering filtered 1 item
Integrity filtering activated and filtered the following item during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
target-repo: "*"in safe-output handlers #21877 (pull_request_read: Resource 'pr:fix: allow wildcardtarget-repo: "*"in safe-output handlers #21877' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)