Skip to content

Validate add_comment_to_pending_review required params before dispatch#2719

Open
gustavo-sec wants to merge 1 commit into
github:mainfrom
gustavo-sec:enforce-add-comment-required-params
Open

Validate add_comment_to_pending_review required params before dispatch#2719
gustavo-sec wants to merge 1 commit into
github:mainfrom
gustavo-sec:enforce-add-comment-required-params

Conversation

@gustavo-sec

Copy link
Copy Markdown

Closes #2718.

add_comment_to_pending_review decoded its arguments with mapstructure.WeakDecode and never checked the required ones, so an omitted required argument (e.g. owner) was sent to the GraphQL API and surfaced as a confusing downstream error such as Could not resolve to a Repository with the name '/<repo>' instead of a clear missing-parameter message.

What changed

Validate owner, repo, pullNumber, path, body, and subjectType up front (using RequiredParam/RequiredInt, matching the other pull request tools) so a missing required argument is reported as missing required parameter: <name> before any API call.

Testing

  • go test ./... — green.
  • golangci-lint run (v2.9.0) — 0 issues.
  • No tool-snapshot or doc changes (input schema unchanged).
  • Added a test for the missing-owner path.
  • Verified live over stdio: calling add_comment_to_pending_review with owner omitted now returns missing required parameter: owner instead of the downstream repository-resolution error.

The same WeakDecode-without-validation pattern exists in copilot.go and discussions.go; happy to address those in a follow-up.

add_comment_to_pending_review decoded its arguments with mapstructure.WeakDecode
and never checked the required ones, so an omitted required argument (e.g. owner)
was sent to the GraphQL API and surfaced as a confusing downstream error such as
"Could not resolve to a Repository with the name '/<repo>'" instead of a clear
missing-parameter message.

Validate owner, repo, pullNumber, path, body, and subjectType up front (matching
how the other pull request tools use RequiredParam/RequiredInt) so a missing
required argument is reported as "missing required parameter: <name>".
@gustavo-sec gustavo-sec requested a review from a team as a code owner June 17, 2026 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add_comment_to_pending_review runs the GraphQL query on a missing required argument instead of validating it

1 participant