Add target field support to submit_pull_request_review safe-output#36546
Conversation
…eview Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… safe-output Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR extends the submit_pull_request_review safe-output type to support pull_request_number and repo, enabling workflows configured with target: "*" to submit reviews on arbitrary PRs/repositories (consistent with sibling PR review comment tools) and improving tool descriptions to surface target constraints.
Changes:
- Added
pull_request_numberandreposupport forsubmit_pull_request_reviewacross validation config, tool schemas (both copies), and agent output schema. - Enhanced
submit_pull_request_reviewtool descriptions to includetarget/target-repoconstraints, with accompanying tests. - Added safe-outputs target validation tests covering wildcard/numeric/invalid targets for
submit-pull-request-review.
Show a summary per file
| File | Description |
|---|---|
schemas/agent-output.json |
Adds pull_request_number and repo fields to SubmitPullRequestReviewOutput. |
pkg/workflow/safe_outputs_validation_config.go |
Allows pull_request_number and repo fields for submit_pull_request_review validation. |
pkg/workflow/js/safe_outputs_tools.json |
Updates tool description and input schema to accept pull_request_number and repo. |
actions/setup/js/safe_outputs_tools.json |
Mirrors the same tool description and schema updates for the runtime copy. |
pkg/workflow/tool_description_enhancer.go |
Appends Target and TargetRepoSlug constraints to enhanced tool descriptions for submit review. |
pkg/workflow/tool_description_enhancer_test.go |
Adds tests asserting target/target-repo constraints appear in enhanced descriptions. |
pkg/workflow/safe_outputs_target_validation_test.go |
Adds target validation test cases for submit-pull-request-review. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 2
| { | ||
| name: "valid wildcard target for submit-pull-request-review", | ||
| config: &SafeOutputsConfig{ | ||
| SubmitPullRequestReview: &SubmitPullRequestReviewConfig{ | ||
| BaseSafeOutputConfig: BaseSafeOutputConfig{}, | ||
| SafeOutputTargetConfig: SafeOutputTargetConfig{ | ||
| Target: "*", | ||
| }, | ||
| }, | ||
| }, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "valid numeric target for submit-pull-request-review", | ||
| config: &SafeOutputsConfig{ | ||
| SubmitPullRequestReview: &SubmitPullRequestReviewConfig{ | ||
| BaseSafeOutputConfig: BaseSafeOutputConfig{}, | ||
| SafeOutputTargetConfig: SafeOutputTargetConfig{ | ||
| Target: "99", | ||
| }, | ||
| }, | ||
| }, | ||
| wantErr: false, | ||
| }, |
| { | ||
| name: "invalid target for submit-pull-request-review", | ||
| config: &SafeOutputsConfig{ | ||
| SubmitPullRequestReview: &SubmitPullRequestReviewConfig{ | ||
| BaseSafeOutputConfig: BaseSafeOutputConfig{}, | ||
| SafeOutputTargetConfig: SafeOutputTargetConfig{ | ||
| Target: "invalid-value", | ||
| }, | ||
| }, | ||
| }, | ||
| wantErr: true, | ||
| errText: "invalid target value for submit-pull-request-review: \"invalid-value\"", | ||
| }, |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #36546 does not have the 'implementation' label and has 85 new lines of code in business logic directories (≤100 threshold). Neither enforcement condition is met. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnose — requesting changes on a formatting issue and a test coverage gap.
📋 Key Themes & Highlights
Issues
- Test formatting: New table entries are indented one extra level —
make fmt/ CI will catch this. - Missing runtime validation test: Config-level tests are good, but there's no test confirming that omitting
pull_request_numberwhentarget: "*"is active actually rejects the tool call at runtime. - Cryptic enhancer constraint:
"Target: *."in the enhanced description doesn't guide the agent; it should mirror the actionable wording already added to the tool schema.
Positive Highlights
- ✅ Clean parity fix —
submit_pull_request_reviewnow aligns with sibling PR tools (create_pull_request_review_comment,reply_to_pull_request_review_comment). - ✅ Both JSON copies updated in lockstep — easy to miss, good discipline.
- ✅ Validation config change is minimal and pattern-matched (
OptionalPositiveIntegerforpull_request_number, bounded string forrepo). - ✅ Schema updated to match the new fields — no drift between contract and implementation.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.2M
| @@ -246,6 +246,43 @@ func TestValidateSafeOutputsTarget(t *testing.T) { | |||
| }, | |||
| wantErr: false, | |||
| }, | |||
There was a problem hiding this comment.
[/tdd] The new test cases are indented one extra level (3 tabs) compared to the surrounding table entries (2 tabs), which will fail go fmt / CI formatting check.
💡 Fix
Align the new entries with the existing pattern:
{
name: "valid wildcard target for submit-pull-request-review",
...
},Run make fmt to auto-fix.
| @@ -246,6 +246,43 @@ func TestValidateSafeOutputsTarget(t *testing.T) { | |||
| }, | |||
| wantErr: false, | |||
| }, | |||
There was a problem hiding this comment.
[/tdd] The tests validate that the config accepts wildcard/numeric/invalid targets, but there's no test covering the runtime check: when target: "*" is active and the agent omits pull_request_number from the tool call, the call should be rejected. Without this regression test the enforcement path is untested.
💡 Suggested test shape
{
name: "wildcard target requires pull_request_number in tool call",
// Set up config with target: "*"
// Call validateToolOutput without pull_request_number
// Expect wantErr: true with errText matching "pull_request_number is required"
},This directly mirrors the existing coverage for create_pull_request_review_comment (if it exists), and makes the fix self-documenting.
| if templatableIntValue(config.Max) > 0 { | ||
| constraints = append(constraints, fmt.Sprintf("Maximum %d review(s) can be submitted.", templatableIntValue(config.Max))) | ||
| } | ||
| if config.Target != "" { |
There was a problem hiding this comment.
[/diagnose] "Target: *." is cryptic guidance for an agent. When target: "*" is set the agent must supply pull_request_number — the description should say so explicitly, consistent with the tool schema description that was just updated.
💡 Suggested wording
if config.Target == "*" {
constraints = append(constraints, "Target: any PR — you must supply pull_request_number in each call.")
} else if config.Target != "" {
constraints = append(constraints, fmt.Sprintf("Target: PR #%s.", config.Target))
}This matches the level of guidance already added to the tool schema description.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (5 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
There was a problem hiding this comment.
Two blocking issues must be fixed before merge.
### Findings
1. CI will fail — test indentation (safe_outputs_target_validation_test.go)
The three new test entries open at 3-tab indent; every existing entry in the slice opens at 2-tab. gofmt normalises this, so the CI formatting gate (make fmt + diff) will fail immediately. Run make fmt before pushing.
2. Missing addRepoParameterIfNeeded case for submit_pull_request_review (safe_outputs_tools_repo_params.go)
computeRepoParamForTool("submit_pull_request_review", ...) always returns nil because there is no case for this tool. Consequence: when a workflow configures submit-pull-request-review with allowed-repos or target-repo: "*", agents receive no context-aware repo description — they cannot determine which repositories are valid targets. Every other cross-repo tool (e.g. create_pull_request_review_comment) has the corresponding case.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 308.3K
| }, | ||
| wantErr: false, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Test cases indented one extra level — make fmt (and CI) will fail.
💡 Details
New test entries (lines 249–287) open their { at three tabs while all existing test entries in this slice open at two tabs. gofmt normalises composite-literal indentation, so the CI formatting gate (make fmt + diff) will detect a mismatch and fail.
Fix: remove the extra leading tab from the three new test cases, or run make fmt before committing.
|
|
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…eld-support # Conflicts: # .github/workflows/grumpy-reviewer.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot ensure implementation is complete all the way to JavaScript |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented through JavaScript coverage in commits 651b296 and fd310ee. I updated the JS validator test config for |
…rget - Add `license` field to the repository package manifest specification (table row + section), matching the schema field added in #36583. - Document the `pull_request_number`/`repo` requirement for submit-pull-request-review when target: "*", matching the support added in #36546. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
submit_pull_request_reviewlackedpull_request_numberandrepofields, making it impossible to use withtarget: "*"— unlikecreate_pull_request_review_commentandreply_to_pull_request_review_commentwhich already support them. The tool description actively told agents not to pass these fields.Changes
safe_outputs_validation_config.go— Addpull_request_number(OptionalPositiveInteger) andrepofields tosubmit_pull_request_review, matching the pattern of sibling PR tools.safe_outputs_tools.json(both copies) — Addpull_request_numberandrepoto the input schema; replace the "do NOT pass pull_request_number" guidance with correcttarget: "*"instructions.schemas/agent-output.json— Addpull_request_numberandrepotoSubmitPullRequestReviewOutput.tool_description_enhancer.go— Surfacetargetandtarget-repoconstraints in the enhanced description, consistent withadd_comment,close_pull_request, etc.submit_pull_request_review; add enhancer tests fortargetandtarget-repoconstraints.actions/setup/js/safe_output_type_validator.test.cjsto includepull_request_numberandrepoinsubmit_pull_request_reviewvalidation config and add explicit cases for both fields present, each field independently, neither field, and invalidpull_request_numbervalues.Example