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

2 participants