Use IssueOrPRNumber for submit_pull_request_review.pull_request_number#39723
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Aligns submit_pull_request_review.pull_request_number validation with the repository-wide convention by switching from OptionalPositiveInteger to IssueOrPRNumber, eliminating inconsistent normalization behavior across safe-output handlers.
Changes:
- Updated the Go safe-outputs validation schema for
submit_pull_request_review.pull_request_numberto useIssueOrPRNumber. - Updated JS validator tests to reflect
IssueOrPRNumberbehavior (string values are preserved rather than normalized to integers). - Adjusted invalid-value test cases to match
IssueOrPRNumber’s type-based validation (reject non-string/non-number types).
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_validation_config.go | Switches submit_pull_request_review.pull_request_number schema from OptionalPositiveInteger to IssueOrPRNumber for consistency with other handlers. |
| actions/setup/js/safe_output_type_validator.test.cjs | Updates expectations and invalid cases to match IssueOrPRNumber validation/normalization behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #39723 does not have the 'implementation' label and has only 1 new line of code in business logic directories (≤100 threshold). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (2 tests analyzed)
Go: 0 ( Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — two observations worth addressing before merge, but the core change is directionally correct.
📋 Key Themes & Highlights
Key Themes
-
Incomplete consistency fix: Three other handlers (
create_pull_request_review_comment,reply_to_pull_request_review_comment,close_pull_request) still useOptionalPositiveIntegerforpull_request_number. Fixing justsubmit_pull_request_reviewleaves the codebase partially inconsistent — a follow-up issue or TODO would help. -
Validation regression for 0 and -1: Switching to
IssueOrPRNumberbroadens acceptance beyond what the downstream handler can safely process without error.parseInt(0)is not NaN, so PR #0 and PR #-1 would silently pass through to the GitHub API. The old validator caught these early.
Positive Highlights
- ✅ Correct diagnosis:
IssueOrPRNumberis the right convention for PR/issue number fields, enabling temporary ID references - ✅ Test updates are clear and the inline comment explains the normalisation change well
- ✅ Single-line Go change is surgical and matches the existing pattern used by all other PR handlers
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
| "body": {Type: "string", Sanitize: true, MaxLength: MaxBodyLength}, | ||
| "event": {Type: "string", Enum: []string{"APPROVE", "REQUEST_CHANGES", "COMMENT"}}, | ||
| "pull_request_number": {OptionalPositiveInteger: true}, | ||
| "pull_request_number": {IssueOrPRNumber: true}, |
There was a problem hiding this comment.
[/diagnose] This fix aligns submit_pull_request_review with the convention, but three other handlers still use OptionalPositiveInteger for their pull_request_number fields: create_pull_request_review_comment (line 224), reply_to_pull_request_review_comment (line 244), and close_pull_request (line 284). Consider a follow-up to fully resolve the inconsistency, or note in the PR body why those are intentionally different.
💡 Remaining instances
// create_pull_request_review_comment
"pull_request_number": {OptionalPositiveInteger: true}, // line 224
// reply_to_pull_request_review_comment
"pull_request_number": {OptionalPositiveInteger: true}, // line 244
// close_pull_request
"pull_request_number": {OptionalPositiveInteger: true}, // line 284If these also need temp-ID support, they should switch to IssueOrPRNumber too.
| const { validateItem } = await import("./safe_output_type_validator.cjs"); | ||
|
|
||
| const invalidValues = [0, -1, "abc", 3.14]; | ||
| // IssueOrPRNumber accepts any string or number - only reject invalid types |
There was a problem hiding this comment.
[/tdd] Narrowing the invalid-value set silently removes rejection of 0 and -1. With IssueOrPRNumber, these pass validation and reach pr_helpers.cjs where parseInt(String(0)) = 0 — which is not NaN, so the handler accepts it and fires a GitHub API call for PR #0 or #-1. The previous OptionalPositiveInteger caught these at validation time.
💡 Suggested addition to the test
// Values that pass IssueOrPRNumber validation but are still semantically
// invalid PR numbers — the handler (or validator) should reject them
const edgeCaseValues = [0, -1, 3.14];
for (const value of edgeCaseValues) {
const result = validateItem(
{ type: 'submit_pull_request_review', event: 'APPROVE', pull_request_number: value },
'submit_pull_request_review', 1
);
// Document whether these should be valid or invalid, and why
}If zero/negative numbers are intentionally allowed (relying on the GitHub API to reject them), a comment explaining that decision would help future readers.
There was a problem hiding this comment.
No blocking issues. The one-line Go change is correct and the test updates accurately reflect the new type contract.
### Non-blocking observations
Remaining inconsistency in sibling handlers
Three other pull_request_number fields in safe_outputs_validation_config.go still use OptionalPositiveInteger (lines 224, 244, 284: create_pull_request_review_comment, reply_to_pull_request_review_comment, and one more). The PR description frames this fix as a consistency alignment, yet the field type remains inconsistent across the PR-review handler family. Not a blocker for this PR, but worth a follow-up pass.
Weaker early validation is mitigated at runtime
IssueOrPRNumber now accepts semantically invalid PR numbers like 0, -1, "abc" at schema-validation time (only non-string/non-number types are rejected). These were caught at validation time before. The resolveTarget runtime guard (isNaN(itemNumber) || itemNumber <= 0 → shouldFail: true) provides a solid safety net, so this is an early-vs-late detection shift, not a silent failure path. Still, the validator gives less helpful diagnostics for pull_request_number: "not-a-number" than before.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer
submit_pull_request_reviewusedOptionalPositiveIntegerfor itspull_request_numberfield while other handlers (push_to_pull_request_branch,add_reviewer,update_pull_request) useIssueOrPRNumberfor the same field. This inconsistency caused subtle normalization differences.Changes
pull_request_numberfromOptionalPositiveIntegertoIssueOrPRNumberinsafe_outputs_validation_config.goIssueOrPRNumberpreserves string types (e.g.,"42"remains"42"instead of normalizing to42)Behavioral difference
Both types are handled correctly by the handler's
parseInt()call, butIssueOrPRNumberis the standard convention for PR/issue number fields across the codebase.