Skip to content

Conversation

JoannaaKL
Copy link
Contributor

@JoannaaKL JoannaaKL commented Oct 8, 2025

This pr:

@JoannaaKL JoannaaKL marked this pull request as ready for review October 9, 2025 08:00
@JoannaaKL JoannaaKL requested a review from a team as a code owner October 9, 2025 08:00
@Copilot Copilot AI review requested due to automatic review settings October 9, 2025 08:00
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new tool to update project items for users or organizations, addressing issue #44. The implementation follows the existing pattern of project item management tools by adding the UpdateProjectItem function with comprehensive validation and testing.

  • Add UpdateProjectItem tool function with field update capabilities
  • Implement comprehensive test coverage with error handling scenarios
  • Update project item data structures to support additional fields

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
pkg/github/tools.go Registers the new UpdateProjectItem tool in the write toolset
pkg/github/projects.go Implements UpdateProjectItem function with validation and API call logic
pkg/github/projects_test.go Adds comprehensive test suite covering success and error scenarios
pkg/github/minimal_types.go Extends MinimalProjectItem struct with Title and Description fields
pkg/github/toolsnaps/update_project_item.snap Tool schema snapshot for testing validation
README.md Documents the new update_project_item tool parameters

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@JoannaaKL JoannaaKL force-pushed the add-update_project_item-tool branch from c6415d1 to d835623 Compare October 9, 2025 09:09
@tonytrg
Copy link
Contributor

tonytrg commented Oct 9, 2025

I am having a hard time understanding new_field, not too familiar with the api of projects but it seems hard to understand to me only from the mcps side of description.

https://docs.github.com/en/rest/projects/items?apiVersion=2022-11-28#update-project-item-for-organization--parameters

fields array of objects Required
A list of field updates to apply.

id integer Required
The ID of the project field to update.

value null or string or number Required
The new value for the field:

For text, number, and date fields, provide the new value directly.
For single select and iteration fields, provide the ID of the option or iteration.
To clear the field, set this to null.

),
mcp.WithObject("new_field",
mcp.Required(),
mcp.Description("Object consisting of the ID of the project field to update and the new value for the field. To clear the field, set \"value\" to null. Example: {\"id\": 123456, \"value\": \"New Value\"}"),
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like we are looking for a map struct.

Should it look something like this

			mcp.Items(
				map[string]interface{}{
					"type": "string",
				},
			),

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if we can rely on the llm to always understand and format a correct parseable struct

Copy link
Contributor

@tonytrg tonytrg Oct 9, 2025

Choose a reason for hiding this comment

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

ah ok i see it needs to be more flexible

			mcp.WithObject("new_field",
				mcp.Required(),
				mcp.Description("Object consisting of the ID of the project field to update and the new value for the field. To clear the field, set \"value\" to null."),
				mcp.Properties(map[string]any{
					"id": map[string]any{
						"type":        "number",
						"description": "The ID of the project field to update (required)",
					},
					"value": map[string]any{
						"description": "The new value for the field. Can be a string for text fields, a number for number/single_select/iteration fields, or null to clear the field (required)",
					},
				}),

would something like that help?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd propose we split it into 2 fields which is field_id and field_value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How the types would look like then? We can use WithNumber for the field_id but field value needs to stay an object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can do it 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@almaleksia @JoannaaKL i updated the struct the one before was wrong

@JoannaaKL JoannaaKL force-pushed the add-update_project_item-tool branch from ab0ae59 to f48befc Compare October 9, 2025 14:26
@tonytrg
Copy link
Contributor

tonytrg commented Oct 9, 2025

💯 thanks for making this complicated one work

note for future as i dont want to block this merge:

  • correct parsing of the iteration ids is missing right now, but as that actually should be part of the sdk we can fix that later on

@JoannaaKL JoannaaKL merged commit 5c61abe into github:main Oct 9, 2025
10 checks passed
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 support for Projects

3 participants