Fix runtime description leak on list_issue_fields legacy variant

The pkg/translations TranslationHelper is first-write-wins on cache
hits: if two callers pass the same key with different fallback
strings, only the first fallback is remembered. Both ListIssueFields
(MS-aware) and ListIssueFieldsLegacy shared the translation key
TOOL_LIST_ISSUE_FIELDS_DESCRIPTION, so at runtime the second-registered
variant (legacy) inherited the first's description — leaking the
MS-flavoured 'single_select, multi_select' text into the FF-off surface.

Only list_issue_fields was affected: issue_write and list_issues use
identical top-level descriptions across variants (their MS-vs-legacy
differentiation is in InputSchema property descriptions, which are
hardcoded strings not routed through the translation helper).

Fix: give the legacy variant its own translation key
(TOOL_LIST_ISSUE_FIELDS_LEGACY_DESCRIPTION). Simplify buildListIssueFields
to accept the already-resolved description string, so the two variants
resolve their translation before calling the shared builder.

Live-verified against a local binary:

- FF off: description no longer contains 'multi_select'
- FF on:  description contains 'multi_select'

Regression test: Test_ListIssueFields_LegacyDoesNotLeakMSDescription
UnderRealTranslationHelper uses an in-test cache that mimics the real
helper's first-write-wins behaviour. Confirmed the test fails if both
variants share a key and passes with them split.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: owenniblock

pkg/github/issue_fields.go

@@ -129,29 +129,38 @@ type issueFieldsOrgQuery struct {
 // multi_select arm, so the OUTPUT will still surface multi-select fields if
 // the org has them (matching what the dotcom UI shows). Only the description
 // is gated.
+//
+// The two variants use DIFFERENT translation keys because the shared
+// TranslationHelper is first-write-wins on cache hits — if both variants used
+// the same key, the second one registered would inherit the first's cached
+// value at runtime, and the FF-off variant would leak the MS description.
 func ListIssueFields(t translations.TranslationHelperFunc) inventory.ServerTool {
-	st := buildListIssueFields(t, true)
+	description := t(
+		"TOOL_LIST_ISSUE_FIELDS_DESCRIPTION",
+		"List issue fields for a repository or organization. Returns field definitions including name, type (text, number, date, single_select, multi_select), and for single_select and multi_select fields the list of valid option names. When repo is omitted, returns org-level fields directly.",
+	)
+	st := buildListIssueFields(t, description)
 	st.FeatureFlagEnable = FeatureFlagIssueFieldsMultiSelect
 	return st
 }
 
 // ListIssueFieldsLegacy is the FF-off variant of list_issue_fields.
 func ListIssueFieldsLegacy(t translations.TranslationHelperFunc) inventory.ServerTool {
-	st := buildListIssueFields(t, false)
+	description := t(
+		"TOOL_LIST_ISSUE_FIELDS_LEGACY_DESCRIPTION",
+		"List issue fields for a repository or organization. Returns field definitions including name, type (text, number, date, single_select), and for single_select fields the list of valid option names. When repo is omitted, returns org-level fields directly.",
+	)
+	st := buildListIssueFields(t, description)
 	st.FeatureFlagDisable = []string{FeatureFlagIssueFieldsMultiSelect}
 	return st
 }
 
-func buildListIssueFields(t translations.TranslationHelperFunc, multiSelectEnabled bool) inventory.ServerTool {
-	description := "List issue fields for a repository or organization. Returns field definitions including name, type (text, number, date, single_select), and for single_select fields the list of valid option names. When repo is omitted, returns org-level fields directly."
-	if multiSelectEnabled {
-		description = "List issue fields for a repository or organization. Returns field definitions including name, type (text, number, date, single_select, multi_select), and for single_select and multi_select fields the list of valid option names. When repo is omitted, returns org-level fields directly."
-	}
+func buildListIssueFields(t translations.TranslationHelperFunc, description string) inventory.ServerTool {
 	return NewTool(
 		ToolsetMetadataIssues,
 		mcp.Tool{
 			Name:        "list_issue_fields",
-			Description: t("TOOL_LIST_ISSUE_FIELDS_DESCRIPTION", description),
+			Description: description,
 			Annotations: &mcp.ToolAnnotations{
 				Title:        t("TOOL_LIST_ISSUE_FIELDS_USER_TITLE", "List issue fields"),
 				ReadOnlyHint: true,

pkg/github/issue_fields_test.go

@@ -316,3 +316,38 @@ func Test_ListIssueFields(t *testing.T) {
 		})
 	}
 }
+
+// Test_ListIssueFields_LegacyDoesNotLeakMSDescriptionUnderRealTranslationHelper
+// pins the fix for the translation-key collision bug. The real translation
+// helper is first-write-wins on cache hits: if both the MS and legacy variants
+// use the same translation key, the second registration inherits the first
+// registration's cached description regardless of the fallback passed in.
+//
+// AllTools() calls the MS variant first, so under the bug the legacy variant
+// silently gets the MS description at runtime — leaking "multi_select" into
+// what should be a legacy-only surface. This test simulates the real helper's
+// cache behaviour and asserts the two descriptions actually differ.
+func Test_ListIssueFields_LegacyDoesNotLeakMSDescriptionUnderRealTranslationHelper(t *testing.T) {
+	// Mimic the caching behaviour of translations.TranslationHelper (see
+	// pkg/translations/translations.go): first-write-wins keyed by key name.
+	cache := map[string]string{}
+	firstWriteWins := func(key, defaultValue string) string {
+		if v, ok := cache[key]; ok {
+			return v
+		}
+		cache[key] = defaultValue
+		return defaultValue
+	}
+
+	// Call in registration order: MS variant first, legacy second. This is
+	// what AllTools() does — see tools.go.
+	msDesc := ListIssueFields(firstWriteWins).Tool.Description
+	legacyDesc := ListIssueFieldsLegacy(firstWriteWins).Tool.Description
+
+	require.NotEqual(t, msDesc, legacyDesc,
+		"the MS and legacy variants MUST use different translation keys so the real TranslationHelper's first-write-wins cache doesn't leak the MS description into the legacy variant")
+	assert.Contains(t, msDesc, "multi_select",
+		"MS variant description must advertise multi_select")
+	assert.NotContains(t, legacyDesc, "multi_select",
+		"legacy variant description must not advertise multi_select — that would leak the gated feature")
+}