Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: Add update_pull_request tool#122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

monotykamary
Copy link
Contributor

@monotykamarymonotykamary commentedApr 5, 2025
edited
Loading

This pull request introduces theupdate_pull_request tool to the GitHub MCP server.

Changes:

  • Added theUpdatePullRequest function inpkg/github/pullrequests.go to define the tool and its handler.
  • Added theOptionalParamOK helper function inpkg/github/server.go.
  • Registered the new tool inpkg/github/server.go.
  • Added unit tests for theUpdatePullRequest tool inpkg/github/pullrequests_test.go.
  • Fixed test failures related to request body expectations for thebase field during testing.

The new tool allows users to update the following fields of an existing pull request:

  • title
  • body
  • state (open/closed)
  • base branch
  • maintainer_can_modify flag

This was updated through theupdate_pull_request tool GitHub MCP server (^6).

@CopilotCopilotAI review requested due to automatic review settingsApril 5, 2025 08:00
Copy link
Contributor

@CopilotCopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

pkg/github/pullrequests.go:156

  • The error message returned for missing update parameters includes a trailing period. Consider unifying the error message text (e.g. 'No update parameters provided') to match test expectations and ensure consistency.
if !updateNeeded {

pkg/github/server.go:129

  • [nitpick] The error message in the optionalParamOk helper could be clarified by explicitly stating the expected and actual types, making debugging easier when a parameter has an unexpected type.
if !ok {

@monotykamary
Copy link
ContributorAuthor

image

Copy link
Collaborator

@juruenjuruen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thank you so much for your PR! ❤️

I added a few comments, mainly to the newoptionalParamOk function.

I'm not super hyped about the name, but naming is hard :)

In general, this tool has shown that the currentoptionalParam falls short when you actually want to know whether the parameter actually exists.

This is probably out of the scope for this PR, but I'm going to open an issue to discuss how we can handle optional parameters better. I have the feeling that it might make sense that theoptionalParam to always return whether the param was passed. But again, that might be a separate discussion.

Copy link
Collaborator

@juruenjuruen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@monotykamary thank you for your changes, just one more thing, would you mind updating the README to list the new tool?

monotykamary reacted with thumbs up emoji
@monotykamary
Copy link
ContributorAuthor

Oh right, forgot about that part. Should be up 👍

juruen
juruen previously approved these changesApr 6, 2025
Copy link
Collaborator

@juruenjuruen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM! 🚀

Thank you so much for your contribution!

monotykamary reacted with rocket emoji
@juruen
Copy link
Collaborator

@monotykamary can you please update your branch with latest main, and we we'll be good to merge!

monotykamary reacted with thumbs up emoji

@monotykamary
Copy link
ContributorAuthor

@juruen I've updated it to follow the newer named export convention, should be good 👍

@juruen
Copy link
Collaborator

@monotykamary I can't update your branch because you probably setmaintainer_can_modify tofalse, so you need to do it before we can merge it.

@monotykamary
Copy link
ContributorAuthor

Aha my bad.@juruen should be set. This PR was made by the MCP server, should probably note somewhere to addmcp.DefaultBool(true), to allow maintainer edits by default.

@juruen
Copy link
Collaborator

juruen commentedApr 9, 2025
edited
Loading

Aha my bad.@juruen should be set. This PR was made by the MCP server

I figured that as well 😄

should probably note somewhere to addmcp.DefaultBool(true), to allow maintainer edits by default.

Agreed!

@juruenjuruen merged commit56c1fce intogithub:mainApr 9, 2025
9 checks passed
manian0430 pushed a commit to ChrisLally/github-mcp-server that referenced this pull requestApr 12, 2025
* feat: add update_pull_request tool* refactor: address feedback on optionalParamOK helper* docs: add update_pull_request tool documentation* refactor: update optionalParamsOK as exported member* fix: rename to exported function
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

Copilot code reviewCopilotCopilot left review comments

@juruenjuruenjuruen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@monotykamary@juruen

[8]ページ先頭

©2009-2025 Movatter.jp