Uh oh!
There was an error while loading. Please reload this page.
feat: gate issue_write and get_issue behind remote_mcp_issue_fields flag - #2558
Conversation
Ports the gating from PR #2553 onto main (the original merge landed on a stack base that did not make it to main). Changes: - pkg/inventory: FeatureFlagDisable becomes []string (any-listed-on → hide). FeatureFlagEnable stays as a single string. This avoids the AND-of-enable semantics from the earlier proposal, which encoded dependencies rather than rollout knobs and had no real call site. Disable-OR is the case that does need the slice (LegacyIssueWrite below). - pkg/github/issues.go: split IssueWrite into IssueWrite (flag-enabled, exposes issue_fields) and LegacyIssueWrite (flag-disabled, omits it). Both register as 'issue_write'; mutually exclusive flag annotations pick exactly one at runtime. Refactored into a shared buildIssueWrite helper instead of duplicating the ~250-line tool definition. - pkg/github/issues.go: GetIssue field_values enrichment now requires the flag at runtime. The verbose REST IssueFieldValues is always cleared from the response. - Existing single-flag Disable call sites converted to slices. - New toolsnap variant issue_write_ff_remote_mcp_issue_fields.snap; the canonical issue_write.snap is owned by LegacyIssueWrite. - README + flag docs regenerated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aligns issue_write and get_issue with existing Issues 2.0 gating by moving issue-fields schema/output behind the remote_mcp_issue_fields feature flag, while also extending inventory support to allow disabling an item when any of several flags are enabled.
Changes:
- Split
issue_writeinto two mutually-exclusive inventory registrations (IssueWriteflag-enabled +LegacyIssueWriteflag-disabled) and added a new toolsnap for the flag-enabled schema. - Gated
get_issuefield-values enrichment behindremote_mcp_issue_fields, and always cleared the verbose RESTissue_field_valuesoutput. - Changed inventory
FeatureFlagDisableannotations fromstringto[]stringwith OR semantics, and updated call sites/tests/docs accordingly.
Show a summary per file
| File | Description |
|---|---|
| README.md | Removes issue_fields from the default (flag-off) issue_write parameter docs. |
| pkg/inventory/server_tool.go | Changes ServerTool.FeatureFlagDisable to []string to support multi-flag disable gating. |
| pkg/inventory/resources.go | Changes ServerResourceTemplate.FeatureFlagDisable to []string. |
| pkg/inventory/prompts.go | Changes ServerPrompt.FeatureFlagDisable to []string. |
| pkg/inventory/filters.go | Updates feature gating logic to exclude items when any disable-flag is enabled. |
| pkg/inventory/registry_test.go | Updates inventory tests/helpers to reflect []string disable flags. |
| pkg/http/handler_test.go | Updates test helper to populate FeatureFlagDisable as a slice. |
| pkg/github/tools.go | Registers LegacyIssueWrite alongside IssueWrite in the tool list. |
| pkg/github/tools_validation_test.go | Updates “feature-flagged tool” detection for []string disable flags. |
| pkg/github/pullrequests.go | Migrates FeatureFlagDisable assignments to slice literals. |
| pkg/github/issues.go | Implements IssueWrite/LegacyIssueWrite split, schema trimming, and get_issue enrichment gating. |
| pkg/github/issues_test.go | Updates toolsnap assertions and adds legacy issue_write schema test; adjusts get_issue expectations for flag-off. |
| pkg/github/csv_output_test.go | Updates test tool disable flag to []string. |
| pkg/github/toolsnaps/issue_write.snap | Updates canonical issue_write snapshot to the legacy (flag-off) schema. |
| pkg/github/toolsnaps/issue_write_ff_remote_mcp_issue_fields.snap | Adds snapshot for the flag-enabled issue_write schema variant. |
| docs/insiders-features.md | Regenerated docs to show issue_write.issue_fields only under remote_mcp_issue_fields. |
| docs/feature-flags.md | Regenerated docs to show issue_write.issue_fields only under remote_mcp_issue_fields. |
Copilot's findings
- Files reviewed: 17/17 changed files
- Comments generated: 2
| gqlClient, err := deps.GetGQLClient(ctx) | ||
| if err != nil { | ||
| return utils.NewToolResultErrorFromErr("failed to get GraphQL client", err), nil, nil | ||
| } |
| @@ -457,24 +459,9 @@ func Test_GetIssue_FieldValues(t *testing.T) { | |||
| err = json.Unmarshal([]byte(textContent.Text), &returnedIssue) | |||
| require.NoError(t, err) | |||
| require.Len(t, returnedIssue.IssueFieldValues, 2, "expected two issue field values") | |||
| first := returnedIssue.IssueFieldValues[0] | |||
| assert.Equal(t, int64(1001), first.IssueFieldID) | |||
| assert.Equal(t, "FV_node_1", first.NodeID) | |||
| assert.Equal(t, "single_select", first.DataType) | |||
| assert.Equal(t, "High", first.Value) | |||
| require.NotNil(t, first.SingleSelectOption) | |||
| assert.Equal(t, int64(42), first.SingleSelectOption.ID) | |||
| assert.Equal(t, "High", first.SingleSelectOption.Name) | |||
| assert.Equal(t, "red", first.SingleSelectOption.Color) | |||
| second := returnedIssue.IssueFieldValues[1] | |||
| assert.Equal(t, int64(1002), second.IssueFieldID) | |||
| assert.Equal(t, "FV_node_2", second.NodeID) | |||
| assert.Equal(t, "text", second.DataType) | |||
| assert.Equal(t, "some text value", second.Value) | |||
| assert.Nil(t, second.SingleSelectOption) | |||
| // Flag is off: raw REST IssueFieldValues must be cleared, enriched FieldValues absent. | |||
| assert.Empty(t, returnedIssue.IssueFieldValues, "raw REST issue_field_values should not be exposed when flag is off") | |||
| assert.Empty(t, returnedIssue.FieldValues, "enriched field_values should not be present when flag is off") | |||
| } | |||
Replace the shared buildIssueWrite(includeIssueFields) helper with two fully duplicated tool definitions. When the FeatureFlagIssueFields flag is retired, LegacyIssueWrite can be deleted as a single function with no merge thinking required. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Companion to Test_GetIssue_FieldValues: when remote_mcp_issue_fields is enabled, the GraphQL nodes() round-trip populates the enriched field_values while the raw REST issue_field_values stays cleared. Addresses the Copilot review suggestion on #2558. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mergeIssueFieldValues built the merged slice by iterating a Go map, which produces non-deterministic ordering and caused a flake in Test_UpdateIssue/partial_update_with_issue_fields_reconciled_by_names (introduced in #2551). Switch to an order-preserving merge: emit incoming entries first in their original order, then any existing entries (in their original order) whose field IDs weren't seen in incoming. Semantics (incoming wins, existing preserved) unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| } | ||
| }) | ||
| st.FeatureFlagDisable = FeatureFlagIssuesGranular | ||
| st.FeatureFlagEnable = FeatureFlagIssueFields |
There was a problem hiding this comment.
Do we need to add this flag to set_issue_fields as well? It currently is still available when issues_granular is enabled. Does that need to also have the remote_mcp_issue_fields flag enabled, so that issue field writes are consistently behind the same flag?
There was a problem hiding this comment.
Issues granular I'm ok with having it alreayd.
Uh oh!
There was an error while loading. Please reload this page.
Companion to Test_GetIssue_FieldValues: when remote_mcp_issue_fields is enabled, the GraphQL nodes() round-trip populates the enriched field_values while the raw REST issue_field_values stays cleared. Addresses the Copilot review suggestion on #2558. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Ports the gating work from #2553 onto
main. #2553 was merged into thekelsey/issue-fields-cleanstack base, but that stack base never landed on main — soissue_writeandget_issueare currently ungated on main even thoughlist_issues,search_issues, andlist_issue_fieldsare.Picks the simpler half of #2553's inventory change: keep
FeatureFlagEnableas a singlestring, only promoteFeatureFlagDisableto[]string(OR-of-any). The AND-on-enable shape in the original PR had no real use site and encoded dependencies rather than rollout, so I dropped it.What changed
pkg/inventory:FeatureFlagDisableonServerTool/ServerResourceTemplate/ServerPromptbecomes[]stringwith OR semantics (any listed flag enabled → tool hidden).FeatureFlagEnablestays a single string.pkg/github/issues.go:IssueWrite(FeatureFlagEnable = FeatureFlagIssueFields, exposesissue_fields) andLegacyIssueWrite(FeatureFlagDisable = {FeatureFlagIssuesGranular, FeatureFlagIssueFields}, omitsissue_fields).issue_write; mutually exclusive annotations pick exactly one at runtime.buildIssueWrite(t, includeIssueFields)helper instead of duplicating the ~250-line tool definition.GetIssuefield-values enrichment now wrapped in a runtimeIsFeatureEnabled(ctx, FeatureFlagIssueFields)check; the verbose RESTIssueFieldValuesis always cleared.FeatureFlagDisablecall sites converted to slice literals.issue_write_ff_remote_mcp_issue_fields.snap. The canonicalissue_write.snapis now owned byLegacyIssueWrite(noissue_fieldsproperty).docs/feature-flags.md,docs/insiders-features.mdregenerated.MCP impact
issue_writeschema has noissue_fieldsproperty;get_issueresponse has nofield_valuesand no rawissue_field_values. Behaviour matches pre-Issues-2.0.issue_writeacceptsissue_fieldswithvalue/field_option_name/delete: true(fetch+merge semantics from Add support for issue fields values toissues.readandissues.write#2551 preserved);get_issuereturns the enrichedfield_values.Verification
Smoke-tested live against
github/plan-track-agentic-toolkit#33:Update via
issue_fields: [{field_name: "Priority", field_option_name: "P2"}]against an issue withPriority=P1, Effort=LowproducedPriority=P2, Effort=Low— non-destructive merge from #2551 confirmed still working under the gated path.script/lint,script/test(with race), and live MCP stdio calls all pass.