Require explicit repo for wildcard push_to_pull_request_branch targets in multi-repo workflows - #49813
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
repo for wildcard push_to_pull_request_branch targets in multi-repo workflows
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49813 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Adds repository disambiguation for wildcard PR-branch pushes in multi-repository workflows.
Changes:
- Adds the
repotool input. - Requires
repofor wildcard targets. - Updates handler validation coverage.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/safe_outputs_tools.json |
Adds and requires the repository input. |
actions/setup/js/safe_outputs_handlers.test.cjs |
Tests missing-repository validation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Require explicit repo for wildcard push_to_pull_request_branch targets
The approach is correct — adding repo to the schema and making it the primary requirement for target: "*" properly addresses the multi-repo checkout ambiguity.
One blocking issue found:
The pull_request_number field description (line 1213 in safe_outputs_tools.json) still says "Required when the workflow target is '''*''' (any PR)." — but this is no longer true after this change. repo is now the required field for wildcard targets. This stale description contradicts the new x-safe-outputs-target-requirements and will mislead agents.
See inline comment for the suggested fix.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.5 AIC · ⌖ 7.67 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report
📊 Test Analysis
Test Overview:
Quality Assessment:
Violations: None Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a stale description and a missing success-path test.
📋 Key Themes & Highlights
Key Themes
- Stale
pull_request_numberdescription — the field still says it is "Required when the workflow target is*" which contradicts the newrepo-first requirement. Agents reading the schema receive conflicting guidance. - Missing success-path test — the updated test block covers only the error case. A complementary test asserting that providing
repolets a wildcard call pass the requirements check would close the coverage gap.
Positive Highlights
- ✅ Clean, targeted schema change — adding
repoas a distinct field with a clearowner/repoformat description. - ✅ Correct
x-safe-outputs-target-requirementspivot frompull_request_numbertorepo, preventing the original fallback-to-GITHUB_WORKSPACE ambiguity. - ✅ Tool description updated in lock-step with the schema change.
- ✅ Test updated to supply
pull_request_numberwhile omittingrepo, proving the guard fires even when the previously required field is present.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 60.4 AIC · ⌖ 9.04 AIC · ⊞ 7.1K
Comment /matt to run again
There was a problem hiding this comment.
Verdict: Request changes
The schema change is incomplete: relaxing the wildcard requirement from pull_request_number to repo alone reintroduces a downstream failure mode instead of fixing it.
💡 Themes
- Broken validation coherence:
push_to_pull_request_branch.cjs(apply-time handler) still hard-requiresmessage.pull_request_numberwhentarget === "*", independent of the MCP-layer schema. By droppingpull_request_numberfromanyOf, calls with onlyreponow pass early validation and fail later with a confusing, less actionable error, after wasted work (git pin, patch/bundle generation, allowed-files scan). - Test coverage gap: the updated test only swaps which single field is missing; the previously-covered "no repo, no PR number" case and the newly-relevant "repo present, PR number missing" case are both left unverified.
- The schema/description update to add
repoas an accepted field is itself reasonable and clearly documented — the issue is solely thex-safe-outputs-target-requirementschange dropping the pre-existingpull_request_numberrequirement instead of requiring both fields together for wildcard targets.
🔎 Code quality review by PR Code Quality Reviewer · auto · 150.2 AIC · ⌖ 4 AIC · ⊞ 7.9K
Comment /review to run again
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers (newest first):
Run details: https://github.com/github/gh-aw/actions/runs/30760589044
|
…ull_request_branch Extend validateWildcardTargetRequirement to support an allOf property (AND semantics) alongside the existing anyOf (OR semantics). Update push_to_pull_request_branch wildcard requirements to require both repo (via anyOf) and pull_request_number (via allOf), ensuring MCP-level validation is coherent with the apply-time handler's requirements. Also update the pull_request_number field description to document that both fields must be supplied together for wildcard targets, and add test coverage for all three paths: missing repo, missing pull_request_number (with repo present), and both fields present (success path). Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all blockers in commit
|
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers appear addressed in your latest response, but the PR still shows blocking review state. Please:
Run details: https://github.com/github/gh-aw/actions/runs/30761726325
|
Maintainer HandoffAll reviewer blockers have been addressed in commit What changed
Local validation
Remaining for human review
|
|
@copilot Please continue triage on this PR. Your latest maintainer handoff says the blocker is fixed, but the PR still shows unresolved thread / blocking-review state. Please:
Run details: https://github.com/github/gh-aw/actions/runs/30763117443
|
Maintainer HandoffAll code changes are complete in commit Wildcard validation coherence fix (
|
| Input | Expected outcome |
|---|---|
pull_request_number present, repo absent |
errors: "requires repo" |
repo present, pull_request_number absent |
errors: "requires pull_request_number" |
Both repo and pull_request_number present |
passes wildcard validation (any further error is downstream) |
Remaining manual actions for maintainers
- Resolve open review threads — Six inline threads are still open. Three are outdated (their concerns are fixed by e226d1d). Three are active but also addressed by e226d1d. A maintainer should resolve them to clear the blocking review state.
- Trigger a CI re-run — The agent's push did not trigger CI. Close/reopen the PR or use
workflow_dispatchto get a green CI signal before merging.
|
@copilot The reviewer-visible fixes appear landed, and the remaining blocker is cleanup of stale review state. Please run the
Newest unresolved review threads to address first:
Run details: https://github.com/github/gh-aw/actions/runs/30764198337
|
|
🎉 This pull request is included in a new release. Release: |
push_to_pull_request_branchcould not resolve sub-repo checkouts in multi-repo workflows whentarget: "*", because the tool schema had norepoinput and fallback execution happened fromGITHUB_WORKSPACE. As a result, branch resolution failed when the PR branch existed only in a side checkout.Schema: add repo disambiguation for push-to-PR-branch
repo(owner/repo) topush_to_pull_request_branch.inputSchema.properties.repoin accepted fields.Wildcard target requirements: make repo explicit
x-safe-outputs-target-requirements["*"]forpush_to_pull_request_branchto requirerepo(primary + anyOf), so wildcard calls must specify which repository checkout to target.Behavioral contract coverage
push_to_pull_request_branchnow errors on missingrepo(even whenpull_request_numberis present), matching the new ambiguity guard.{ "name": "push_to_pull_request_branch", "inputSchema": { "properties": { "repo": { "type": "string" }, "pull_request_number": { "type": ["number", "string"] } } }, "x-safe-outputs-target-requirements": { "*": { "primary": "repo", "anyOf": ["repo"] } } }Run: https://github.com/github/gh-aw/actions/runs/30761726325