Skip to content

Add support for issue fields values to issues.read and issues.write#2551

Open
kelsey-myers wants to merge 6 commits into
github:mainfrom
kelsey-myers:kelsey/issue-fields-clean
Open

Add support for issue fields values to issues.read and issues.write#2551
kelsey-myers wants to merge 6 commits into
github:mainfrom
kelsey-myers:kelsey/issue-fields-clean

Conversation

@kelsey-myers
Copy link
Copy Markdown
Contributor

Summary

This PR is passing issue field values to the issue write & read tools, which now (after #2452) support this.

Why

What changed

  • the go-gothub lib was updated to pass issue field value sto rest api
  • the repo upgraded to use v0.87 of go-github
  • we're now using new params for IssueFieldValues

MCP impact

  • No tool or API changes
  • Tool schema or behavior changed
  • New tool added

Prompts tested (tool changes only)

  • added tests to make sure that passing fields works, as well as retrieving them

Security / limits

  • No security or limits impact
  • Auth / permissions considered
  • Data exposure, filtering, or token/size limits considered

Tool renaming

  • I am renaming tools as part of this PR (e.g. a part of a consolidation effort)
    • I have added the new tool aliases in deprecated_tool_aliases.go
  • I am not renaming tools as part of this PR

Note: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.

Lint & tests

  • Linted locally with ./script/lint
  • Tested locally with ./script/test

Docs

  • Not needed
  • Updated (README / docs / examples)

@kelsey-myers
Copy link
Copy Markdown
Contributor Author

Tools tested on this branch

list_issue_fields

Inputs:

param type required notes
owner string yes repo owner or org login
repo string no if omitted, returns org-level fields

Output: array of issue field definitions

[
  {
    "id": "IFSS_1",
    "full_database_id": 99,
    "name": "Priority",
    "data_type": "SINGLE_SELECT",
    "visibility": "ALL",
    "options": [
      { "id": "OPT_1", "name": "High", "color": "red" },
      { "id": "OPT_2", "name": "Low", "color": "blue" }
    ]
  },
  {
    "id": "IFT_2",
    "full_database_id": 42,
    "name": "DRI",
    "data_type": "TEXT",
    "visibility": "ORG_ONLY"
  }
]

issue_write (create / update)

New input: issue_fields — array of field values to set on the issue. Each item requires field_name and either value or field_option_name (not both).

{
  "issue_fields": [
    { "field_name": "Priority", "field_option_name": "High" },
    { "field_name": "DRI", "value": "monalisa" },
    { "field_name": "Due Date", "value": "2026-06-01" }
  ]
}
  • Use field_option_name for SINGLE_SELECT fields — the server validates the option exists before calling the API
  • Use value for TEXT, NUMBER, and DATE fields

Output: unchanged — { "id": "...", "url": "..." }


issue_read (method: get)

Inputs: unchanged

Output: the response now includes a field_values array alongside the standard issue fields:

{
  "number": 123,
  "title": "...",
  "field_values": [
    { "field_name": "Priority", "value": "High" },
    { "field_name": "DRI", "value": "monalisa" }
  ]
}

The raw issue_field_values from the REST response is replaced with this normalised GraphQL-sourced field_values for consistency with list_issues and search_issues.

iulia-b and others added 3 commits May 26, 2026 09:44
…abaseId

- Expose fullDatabaseId (BigInt) in list_issue_fields
- Add issue_fields parameter to issue_write for setting field values
- Support single-select fields via field_option_name resolution
- Add REST API field value extraction in get_issue responses
- Update minimal types with IssueFieldValue for REST responses

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@kelsey-myers kelsey-myers force-pushed the kelsey/issue-fields-clean branch from 70e8866 to 7b1b06c Compare May 26, 2026 16:44
Copy link
Copy Markdown
Contributor

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 extends the issues tool surface to support reading and writing GitHub Issues 2.0 custom field values, aligning issue_read/issue_write behavior with the newer REST/GraphQL capabilities introduced alongside the go-github upgrade.

Changes:

  • Added issue_fields input to issue_write, including name-based resolution of field IDs and (for single-select) option validation.
  • Expanded issue_read output handling to include REST issue_field_values, and added best-effort GraphQL enrichment to normalize to field_values where possible.
  • Updated minimal/DTO types and unit tests to cover issue field value parsing and write request shaping.
Show a summary per file
File Description
README.md Documents the new issue_fields input for issue_write.
pkg/github/minimal_types.go Adds REST-backed minimal issue field value types and separates REST issue_field_values from GraphQL-normalized field_values.
pkg/github/issues.go Implements parsing + GraphQL metadata resolution for issue_fields, wires field values into create/update, and adjusts search/get enrichment paths.
pkg/github/issues_test.go Adds/updates tests for get/write behavior involving issue field values.
pkg/github/issue_fields.go Fetches/stores GraphQL fullDatabaseId for issue fields and adds parsing helper.
pkg/github/issue_fields_test.go Updates expectations to include fullDatabaseId/DatabaseID.
pkg/github/toolsnaps/issue_write.snap Updates tool schema snapshot to include issue_fields.
docs/insiders-features.md Reflects the issue_fields parameter in generated docs.
docs/feature-flags.md Reflects the issue_fields parameter in generated docs.

Copilot's findings

Comments suppressed due to low confidence (1)

pkg/github/issues.go:1599

  • searchIssuesHandler now always performs GraphQL field_values enrichment whenever there are results, and returns an error if GraphQL is unavailable or the query fails. This looks like a regression of the documented behavior of FeatureFlagIssueFields (which is meant to gate search_issues field_values enrichment); please restore the feature-flag check so search_issues works without GraphQL issue fields features enabled.
	var fieldValuesByID map[string][]MinimalFieldValue
	if len(result.Issues) > 0 {
		gqlClient, err := deps.GetGQLClient(ctx)
		if err != nil {
			return utils.NewToolResultErrorFromErr(errorPrefix+": failed to get GitHub GraphQL client", err), nil
		}
		fieldValuesByID, err = fetchIssueFieldValuesByNodeID(ctx, gqlClient, result.Issues)
		if err != nil {
			return ghErrors.NewGitHubGraphQLErrorResponse(ctx, errorPrefix+": failed to fetch issue field values", err), nil
		}
	}
  • Files reviewed: 9/9 changed files
  • Comments generated: 2

Comment thread pkg/github/issues.go Outdated
Comment thread pkg/github/issues.go
@kelsey-myers kelsey-myers marked this pull request as ready for review May 26, 2026 16:58
@kelsey-myers kelsey-myers requested a review from a team as a code owner May 26, 2026 16:58
@kelsey-myers
Copy link
Copy Markdown
Contributor Author

cc @SamMorrowDrums 👋🏾 gated these changes behind the fields flag in #2553 to keep the diffs manageable

Comment thread pkg/github/issues_test.go
Comment on lines +410 to +420
{
IssueFieldID: 1001,
NodeID: "FV_node_1",
DataType: "single_select",
Value: "High",
SingleSelectOption: &github.IssueFieldValueSingleSelectOption{
ID: 42,
Name: "High",
Color: "red",
},
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am thinking we should update the structure and only expose relevant fields

Suggested change
{
IssueFieldID: 1001,
NodeID: "FV_node_1",
DataType: "single_select",
Value: "High",
SingleSelectOption: &github.IssueFieldValueSingleSelectOption{
ID: 42,
Name: "High",
Color: "red",
},
},
{
DataType: "single_select",
Value: "High", # this should be name
SingleSelectOption: &github.IssueFieldValueSingleSelectOption{
Name: "High", # this should be also the name, not the value, which is the ID for single select options
Color: "red",
},
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah 👍🏼 you did this

	IssueFieldSingleSelect struct {
		FullDatabaseID githubv4.String `graphql:"fullDatabaseId"`
		Name           githubv4.String
		DataType       githubv4.String
		Options        []struct {
			FullDatabaseID githubv4.String `graphql:"fullDatabaseId"`
			Name           githubv4.String
		}

Copy link
Copy Markdown
Collaborator

@SamMorrowDrums SamMorrowDrums left a comment

Choose a reason for hiding this comment

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

Tried this end-to-end against github/plan-track-agentic-toolkit with a local build of this branch — two things I'd want to resolve before this ships, plus a smaller follow-up.

🚨 issue_write with issue_fields is destructive: it wipes all unspecified field values

The REST PATCH /repos/{o}/{r}/issues/{n} fields array is a full replacement of the issue's custom field values, not a merge. The PR forwards the caller's subset straight through, so setting one field silently clears every other field.

Reproduced live against issue #33 in that repo:

  1. Starting state via issue_read: field_values: [{Priority: P2}].
  2. Called issue_write with issue_fields: [{field_name:"Effort", field_option_name:"Low"}].
  3. Resulting state (confirmed via GraphQL): [{Effort: Low}]Priority is gone.

This is silent: the issue_write response is only {"id":..., "url":...} so it does not echo the resulting field set, and the model has no way to notice that other fields were destroyed. Omitting issue_fields entirely is safe (early return), and issue_fields: [] is also safe — the problem is specifically "supply a non-empty subset".

This feels like a release blocker on its own — agents calling issue_write to set one priority will routinely wipe DRI, Target Date, etc.

Suggested fixes, in order of preference:

  • Fetch current field values, merge with the caller's input, and send the full merged set to REST. Closest match to the implicit contract of an MCP "update issue" tool.
  • Or, at minimum, surface a very loud warning in the tool description that issue_fields is a full replacement, and have the response echo the final set of field values so destruction is at least visible.

⚠️ No mechanism to unset / clear a field value

There's no path to clear a value. I tried:

  • value: null → rejected client-side: value cannot be null for field "Effort".
  • value: "" → forwarded to REST, which 422s: Issue field option with name does not exist.
  • omitting the field from issue_fields → only "works" because of the destructive-overwrite bug above, which also wipes everything else. Not a real solution.

We need a documented delete: true (or equivalent) shape — the existing granular set_issue_fields tool already supports delete: true, so it would be good for the new issue_fields input to match. Without an unset path, agents told "remove the priority on this issue" have no way to do it.

⚠️ Server instructions don't teach the workflow (ideally gated by the same flag)

generateIssuesToolsetInstructions (the issues toolset's InstructionsFunc, rendered into the server's initialize instructions string) is static and still only mentions list_issue_types, search_issues, and state_reason. With this PR, when remote_mcp_issue_fields is on, the model suddenly sees field_values enriched into list_issues / issue_read output and a new issue_fields write input — but nothing in the server instructions tells it to call list_issue_fields (or read existing field_values first), prefer field_option_name over raw value for single-selects, or be aware of the replace-vs-merge semantics.

Other toolsets (e.g. projects) use this slot to teach the "list fields → use exact names → then write" pattern. Worth doing the same here. Ideally the additional issue-fields-specific guidance is appended only when the feature flag is enabled, so legacy clients aren't told about a surface they don't have. The InstructionsFunc(inv *Inventory) signature already gets the inventory, so flag-aware composition should be straightforward.


Happy to pair on any of the above if useful. The schema (oneOf of value xor field_option_name, additionalProperties: false), the field-name lookup with the nice "Known fields: …" error on list_issues, and the toolsnap coverage all look great — it's really just the write semantics I'd want pinned down before this lands.

@SamMorrowDrums
Copy link
Copy Markdown
Collaborator

Follow-up on blocker #1 — I checked whether this is REST behavior or wrapper behavior. It's the REST endpoint.

Direct curl against PATCH /repos/github/plan-track-agentic-toolkit/issues/33, bypassing this codebase entirely:

Before: [{Priority: P2}, {Effort: Low}]
PATCH body: {"issue_field_values":[{"field_id":5,"value":"Medium"}]}
After:  [{Effort: Medium}]   ← Priority gone

So the API contract for issue_field_values on PATCH /issues/{n} is full replacement, and any caller that supplies a partial array will silently destroy the unspecified values.

That doesn't change the recommendation, just shifts where the workaround has to live:

  • Ideal: get the REST endpoint changed to merge / accept partial updates (and add an explicit delete: true per-field for unset). That would also solve blocker me/my/I function #3 in one shot.
  • If we can't get the API to change in time, this PR needs to either (a) read existing field values and merge before PATCHing, or (b) make the replace semantics impossible to miss — strong warning in the schema description, and the response should echo the resulting field set so destruction is visible to the model.

Worth checking with whoever owns the issues REST endpoint whether merge + per-field delete is on the table before we decide which workaround to ship.

@SamMorrowDrums
Copy link
Copy Markdown
Collaborator

Dug into the monolith to figure out where the "Add" vs "Set" surprise comes from. tl;dr: the upstream already has the API shape we want, this PR is just calling the wrong endpoint.

What's actually on the server (app/api/issue_field_values.rb)

The helper get_issue_field_update_attributes takes a mode argument. The same code path is wired up from four different REST endpoints with different modes:

Endpoint Mode Behavior
POST /repos/{o}/{r}/issues/{n}/issue-field-values add Additive merge — never deletes a value not in the payload (only clears all when body is [])
PUT /repos/{o}/{r}/issues/{n}/issue-field-values set Full replace — anything not in the payload is deleted
DELETE /repos/{o}/{r}/issues/{n}/issue-field-values/{field_id} n/a Clears a single value
PATCH /repos/{o}/{r}/issues/{n} with issue_field_values:[…] set Same destructive replace as PUT (gated on IssueFieldsFeature for the org, otherwise silently ignored)

So the destructive overwrite I reported is intentional behavior of PATCH /issues/{n}, but it's only one of four documented shapes — and the "additive" shape we want for issue_write already exists as a first-class endpoint.

Why the surprise: PATCH vs POST share a helper, opposite modes

The Copilot review on the monolith PR that introduced these (#395944) summarises it as "adds POST and PUT endpoints for managing issue field values" and explicitly contrasts "POST for adding field values and PUT for setting/replacing". So:

  • If you tested by hitting the dedicated POST endpoint → additive, no surprises.
  • If you tested by hitting PATCH on the issue → destructive replace.

Both paths route into the same get_issue_field_update_attributes helper but with mode: "add" vs mode: "set" respectively. Easy to miss.

Simple recommendation

Stop putting issue_field_values in the PATCH /issues/{n} body, and instead call the dedicated endpoints from the issue_write handler:

issue_write …, issue_fields: [ {field_name, value|field_option_name, delete?: bool} ]
        │
        ├── any items with delete: true → for each: DELETE …/issue-field-values/{field_id}
        └── any items with values        → POST …/issue-field-values  (additive merge)

That gives us:

  • Non-destructive merge for the common case ("set Priority" doesn't wipe DRI).
  • A real unset path via the DELETE sub-resource — addresses the open question about clearing a value.
  • The existing granular set_issue_fields tool already exposes delete: true per field via the GraphQL setIssueFieldValue mutation, so the input shape stays consistent across both tools.
  • One extra HTTP call only when the caller actually wants to delete; the additive POST handles add+update in a single round-trip just like the current PATCH.

Trade-off: the monolith endpoints aren't in go-github v0.87 yet, but they're small enough to hit via the http client directly. Happy to sketch that out if useful.

PATCH /issues/{n} with issue_field_values would stay available for callers who explicitly want full replace, but issue_write shouldn't be that caller.

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.

5 participants