From 6697c6bc283e543537a09608591967eaaa3fad64 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 27 Mar 2026 21:33:51 +0000 Subject: [PATCH 1/5] Initial plan From b34d84a19c172854a7a54ff5593ed202511d20a2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 27 Mar 2026 22:03:04 +0000 Subject: [PATCH 2/5] Fix safe discussion updates: add labels field to schema, filter schema fields by config, add runtime guards - Add 'labels' field to update_discussion tool schema in safe_outputs_tools.json - Add filterToolSchemaFields() in Go to remove title/body/labels from update_discussion schema when not enabled in config (e.g., labels-only config hides title/body) - Add enhanceToolDescription() case for update_discussion with label constraints - Fix executeDiscussionUpdate() in JS to only call updateDiscussion GraphQL mutation when title or body is actually being updated (not for label-only updates) - Add allow_title and allow_body runtime guards in buildDiscussionUpdateData() to reject updates to fields not enabled in the safe-outputs configuration - Add comprehensive JS tests in update_discussion.test.cjs covering all combinations - Add Go tests for schema field filtering (TestFilterUpdateDiscussionSchemaFields*) and description enhancement (update_discussion cases in TestEnhanceToolDescription) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/afab35c0-82a0-49da-a73a-526f8bea768c Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com> --- actions/setup/js/safe_outputs_tools.json | 9 +- actions/setup/js/update_discussion.cjs | 56 +- actions/setup/js/update_discussion.test.cjs | 556 +++++++++++++++++++ pkg/workflow/safe_outputs_tools_filtering.go | 48 ++ pkg/workflow/safe_outputs_tools_test.go | 299 ++++++++++ pkg/workflow/tool_description_enhancer.go | 23 + 6 files changed, 970 insertions(+), 21 deletions(-) create mode 100644 actions/setup/js/update_discussion.test.cjs diff --git a/actions/setup/js/safe_outputs_tools.json b/actions/setup/js/safe_outputs_tools.json index fdcd78318f4..7cf9666acce 100644 --- a/actions/setup/js/safe_outputs_tools.json +++ b/actions/setup/js/safe_outputs_tools.json @@ -98,7 +98,7 @@ }, { "name": "update_discussion", - "description": "Update an existing GitHub discussion's title or body. Use this to modify discussion properties after creation. Only the fields you specify will be updated; other fields remain unchanged.", + "description": "Update an existing GitHub discussion's title, body, or labels. Use this to modify discussion properties after creation. Only the fields you specify will be updated; other fields remain unchanged.", "inputSchema": { "type": "object", "properties": { @@ -110,6 +110,13 @@ "type": "string", "description": "New discussion body to replace the existing content. Use Markdown formatting." }, + "labels": { + "type": "array", + "items": { + "type": "string" + }, + "description": "Replace the discussion labels with this list (e.g., [\"bug\", \"tracking:foo\"]). Labels must exist in the repository." + }, "discussion_number": { "type": ["number", "string"], "description": "Discussion number to update. This is the numeric ID from the GitHub URL (e.g., 345 in github.com/owner/repo/discussions/345). Required when the workflow target is '*' (any discussion)." diff --git a/actions/setup/js/update_discussion.cjs b/actions/setup/js/update_discussion.cjs index a80c8479ce1..05b223f8e58 100644 --- a/actions/setup/js/update_discussion.cjs +++ b/actions/setup/js/update_discussion.cjs @@ -143,37 +143,47 @@ async function executeDiscussionUpdate(github, context, discussionNumber, update throw new Error(`${ERR_NOT_FOUND}: Discussion #${discussionNumber} not found`); } - // Build mutation for updating discussion - let mutation = ` - mutation($discussionId: ID!, $title: String, $body: String) { - updateDiscussion(input: { discussionId: $discussionId, title: $title, body: $body }) { - discussion { - id - title - body - url + const hasTitleUpdate = updateData.title !== undefined; + const hasBodyUpdate = updateData.body !== undefined; + const hasLabelsUpdate = updateData.labels !== undefined; + + let updatedDiscussion = discussion; + + // Only call the updateDiscussion mutation when title or body actually needs updating. + // Skipping this when only labels are being changed avoids accidentally modifying + // the discussion body with stale or unexpected content. + if (hasTitleUpdate || hasBodyUpdate) { + const mutation = ` + mutation($discussionId: ID!, $title: String, $body: String) { + updateDiscussion(input: { discussionId: $discussionId, title: $title, body: $body }) { + discussion { + id + title + body + url + } } } - } - `; + `; - const variables = { - discussionId: discussion.id, - title: updateData.title || discussion.title, - body: updateData.body || discussion.body, - }; + const variables = { + discussionId: discussion.id, + title: hasTitleUpdate ? updateData.title : discussion.title, + body: hasBodyUpdate ? updateData.body : discussion.body, + }; - const mutationResult = await github.graphql(mutation, variables); - const updatedDiscussion = mutationResult.updateDiscussion.discussion; + const mutationResult = await github.graphql(mutation, variables); + updatedDiscussion = mutationResult.updateDiscussion.discussion; + } // Handle label replacement if labels were provided - if (updateData.labels !== undefined) { + if (hasLabelsUpdate) { try { const labelIds = await fetchLabelNodeIds(github, context.repo.owner, context.repo.repo, updateData.labels); await replaceDiscussionLabels(github, discussion.id, labelIds); core.info(`Successfully replaced labels on discussion #${discussionNumber}`); } catch (error) { - core.warning(`Discussion #${discussionNumber} title/body updated successfully, but label update failed: ${getErrorMessage(error)}`); + core.warning(`Discussion #${discussionNumber} label update failed: ${getErrorMessage(error)}`); } } @@ -240,10 +250,16 @@ function buildDiscussionUpdateData(item, config) { const updateData = {}; if (item.title !== undefined) { + if (config.allow_title !== true) { + return { success: false, error: "Title updates are not allowed by the safe-outputs configuration" }; + } // Sanitize title for Unicode security (no prefix handling needed for updates) updateData.title = sanitizeTitle(item.title); } if (item.body !== undefined) { + if (config.allow_body !== true) { + return { success: false, error: "Body updates are not allowed by the safe-outputs configuration" }; + } updateData.body = item.body; } diff --git a/actions/setup/js/update_discussion.test.cjs b/actions/setup/js/update_discussion.test.cjs new file mode 100644 index 00000000000..948c05c5de5 --- /dev/null +++ b/actions/setup/js/update_discussion.test.cjs @@ -0,0 +1,556 @@ +// @ts-check +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +const { main } = require("./update_discussion.cjs"); + +/** + * Tests for update_discussion.cjs + * + * These tests verify the safe-outputs behavior of discussion updates, particularly: + * - Label-only updates do NOT modify title or body + * - Title/body-only updates do NOT modify labels + * - Config guards prevent unauthorized field updates + * - All combinations of allowed fields work correctly + */ +describe("update_discussion", () => { + let mockCore; + let mockGithub; + let mockContext; + let originalGlobals; + let originalEnv; + + // Track all graphql calls for assertion + let graphqlCalls = /** @type {Array<{query: string, variables: any}>} */ []; + + // Default discussion returned by the "fetch discussion" query + const defaultDiscussion = { + id: "D_kwDOTest123", + title: "Original Title", + body: "Original body content", + url: "https://github.com/testowner/testrepo/discussions/42", + number: 42, + }; + + // Default repo labels for fetchLabelNodeIds + const defaultLabels = [ + { id: "LA_kwDO1", name: "Label1" }, + { id: "LA_kwDO2", name: "Label2" }, + { id: "LA_kwDO3", name: "Label3" }, + { id: "LA_kwDO4", name: "Label4" }, + { id: "LA_kwDO_bug", name: "bug" }, + { id: "LA_kwDO_feature", name: "feature" }, + ]; + + beforeEach(() => { + originalGlobals = { + core: global.core, + github: global.github, + context: global.context, + }; + originalEnv = { + staged: process.env.GH_AW_SAFE_OUTPUTS_STAGED, + workflowName: process.env.GH_AW_WORKFLOW_NAME, + workflowId: process.env.GH_AW_WORKFLOW_ID, + }; + + graphqlCalls = []; + + mockCore = { + infos: /** @type {string[]} */ [], + warnings: /** @type {string[]} */ [], + errors: /** @type {string[]} */ [], + info: /** @param {string} msg */ msg => mockCore.infos.push(msg), + warning: /** @param {string} msg */ msg => mockCore.warnings.push(msg), + error: /** @param {string} msg */ msg => mockCore.errors.push(msg), + setOutput: vi.fn(), + setFailed: vi.fn(), + }; + + mockGithub = { + graphql: async (/** @type {string} */ query, /** @type {any} */ variables) => { + graphqlCalls.push({ query, variables }); + + // Fetch discussion by number (getDiscussionQuery) + if (query.includes("discussion(number:")) { + return { + repository: { + discussion: { ...defaultDiscussion }, + }, + }; + } + + // Fetch repo labels for fetchLabelNodeIds + if (query.includes("labels(first: 100)") && query.includes("repository(owner:")) { + return { + repository: { + labels: { + nodes: defaultLabels, + }, + }, + }; + } + + // Fetch discussion labels (for replaceDiscussionLabels removeQuery) + if (query.includes("node(id:") && query.includes("on Discussion")) { + return { + node: { + labels: { + nodes: [], + }, + }, + }; + } + + // updateDiscussion mutation + if (query.includes("updateDiscussion")) { + return { + updateDiscussion: { + discussion: { + ...defaultDiscussion, + title: variables.title || defaultDiscussion.title, + body: variables.body || defaultDiscussion.body, + }, + }, + }; + } + + // addLabelsToLabelable mutation + if (query.includes("addLabelsToLabelable")) { + return { addLabelsToLabelable: { clientMutationId: "test" } }; + } + + // removeLabelsFromLabelable mutation + if (query.includes("removeLabelsFromLabelable")) { + return { removeLabelsFromLabelable: { clientMutationId: "test" } }; + } + + throw new Error(`Unexpected GraphQL query: ${query.substring(0, 80)}`); + }, + }; + + mockContext = { + eventName: "discussion", + repo: { owner: "testowner", repo: "testrepo" }, + serverUrl: "https://github.com", + runId: 12345, + payload: { + discussion: { number: 42 }, + }, + }; + + global.core = mockCore; + global.github = mockGithub; + global.context = mockContext; + process.env.GH_AW_WORKFLOW_NAME = "Test Workflow"; + process.env.GH_AW_WORKFLOW_ID = "test-workflow"; + delete process.env.GH_AW_SAFE_OUTPUTS_STAGED; + }); + + afterEach(() => { + global.core = originalGlobals.core; + global.github = originalGlobals.github; + global.context = originalGlobals.context; + if (originalEnv.staged === undefined) { + delete process.env.GH_AW_SAFE_OUTPUTS_STAGED; + } else { + process.env.GH_AW_SAFE_OUTPUTS_STAGED = originalEnv.staged; + } + if (originalEnv.workflowName === undefined) { + delete process.env.GH_AW_WORKFLOW_NAME; + } else { + process.env.GH_AW_WORKFLOW_NAME = originalEnv.workflowName; + } + if (originalEnv.workflowId === undefined) { + delete process.env.GH_AW_WORKFLOW_ID; + } else { + process.env.GH_AW_WORKFLOW_ID = originalEnv.workflowId; + } + }); + + // Helper to collect updateDiscussion mutation calls + function getUpdateDiscussionMutations() { + return graphqlCalls.filter(c => c.query.includes("updateDiscussion(")); + } + + // Helper to collect label mutation calls + function getAddLabelsCalls() { + return graphqlCalls.filter(c => c.query.includes("addLabelsToLabelable")); + } + + function getRemoveLabelsCalls() { + return graphqlCalls.filter(c => c.query.includes("removeLabelsFromLabelable")); + } + + describe("labels-only update (the bug scenario)", () => { + it("should update labels without calling updateDiscussion mutation when only labels are configured", async () => { + // This is the exact scenario from the bug report: + // safe-outputs: update-discussion: target: "*" allowed-labels: [Label1, Label2] + const handler = await main({ + target: "*", + allow_labels: true, + allowed_labels: ["Label1", "Label2"], + }); + + const result = await handler({ type: "update_discussion", labels: ["Label1", "Label2"], discussion_number: 42 }, {}); + expect(result.success).toBe(true); + + // CRITICAL: The updateDiscussion mutation must NOT have been called + // (body/title must not be modified when only labels need updating) + const updateMutations = getUpdateDiscussionMutations(); + expect(updateMutations).toHaveLength(0); + + // Labels should have been updated + const addCalls = getAddLabelsCalls(); + expect(addCalls).toHaveLength(1); + expect(mockCore.infos.some(msg => msg.includes("Successfully replaced labels"))).toBe(true); + }); + + it("should reject body update when only labels are configured", async () => { + // Bug scenario: AI passes body instead of labels because schema only showed 'body' + // (before the fix, the schema didn't have a 'labels' field, so the AI used 'body') + const handler = await main({ + target: "*", + allow_labels: true, + allowed_labels: ["Label1", "Label2"], + }); + + const result = await handler({ type: "update_discussion", body: '{"add_labels": ["Label3", "Label4"]}', discussion_number: 42 }, {}); + // Body update must be rejected since allow_body is not set + expect(result.success).toBe(false); + expect(result.error).toContain("Body updates are not allowed"); + + // The discussion body must NOT have been changed + const updateMutations = getUpdateDiscussionMutations(); + expect(updateMutations).toHaveLength(0); + }); + + it("should reject title update when only labels are configured", async () => { + const handler = await main({ + target: "*", + allow_labels: true, + allowed_labels: ["Label1", "Label2"], + }); + + const result = await handler({ type: "update_discussion", title: "New Title", discussion_number: 42 }, {}); + expect(result.success).toBe(false); + expect(result.error).toContain("Title updates are not allowed"); + + const updateMutations = getUpdateDiscussionMutations(); + expect(updateMutations).toHaveLength(0); + }); + + it("should fail when all requested labels are not in the allowed list", async () => { + const handler = await main({ + target: "*", + allow_labels: true, + allowed_labels: ["Label1", "Label2"], + }); + + // Agent attempts to set Label3 and Label4 which are NOT in the allowed list + // When ALL labels are disallowed, validateLabels returns an error + const result = await handler({ type: "update_discussion", labels: ["Label3", "Label4"], discussion_number: 42 }, {}); + // All labels were filtered out → error is returned + expect(result.success).toBe(false); + expect(result.error).toContain("No valid labels found after sanitization"); + + // The updateDiscussion mutation must NOT have been called + const updateMutations = getUpdateDiscussionMutations(); + expect(updateMutations).toHaveLength(0); + + // No label mutations should have been called either + expect(getAddLabelsCalls()).toHaveLength(0); + }); + + it("should apply only the allowed subset of requested labels", async () => { + const handler = await main({ + target: "*", + allow_labels: true, + allowed_labels: ["Label1", "Label2"], + }); + + // Agent requests Label1 (allowed) and Label3 (not allowed) + const result = await handler({ type: "update_discussion", labels: ["Label1", "Label3"], discussion_number: 42 }, {}); + expect(result.success).toBe(true); + + // Must not call updateDiscussion mutation + expect(getUpdateDiscussionMutations()).toHaveLength(0); + + // Labels were replaced - only Label1 should be added + const addCalls = getAddLabelsCalls(); + expect(addCalls).toHaveLength(1); + // Label1 has id "LA_kwDO1" + expect(addCalls[0].variables.labelIds).toEqual(["LA_kwDO1"]); + }); + }); + + describe("title-only update", () => { + it("should update title without calling label mutations when only title is configured", async () => { + const handler = await main({ + target: "*", + allow_title: true, + }); + + const result = await handler({ type: "update_discussion", title: "New Title", discussion_number: 42 }, {}); + expect(result.success).toBe(true); + + // The updateDiscussion mutation MUST have been called with the new title + const updateMutations = getUpdateDiscussionMutations(); + expect(updateMutations).toHaveLength(1); + expect(updateMutations[0].variables.title).toBe("New Title"); + // Body should preserve the existing value (not change it) + expect(updateMutations[0].variables.body).toBe(defaultDiscussion.body); + + // No label mutations should have been called + expect(getAddLabelsCalls()).toHaveLength(0); + expect(getRemoveLabelsCalls()).toHaveLength(0); + }); + + it("should reject body update when only title is configured", async () => { + const handler = await main({ + target: "*", + allow_title: true, + }); + + const result = await handler({ type: "update_discussion", body: "New body", discussion_number: 42 }, {}); + expect(result.success).toBe(false); + expect(result.error).toContain("Body updates are not allowed"); + + expect(getUpdateDiscussionMutations()).toHaveLength(0); + }); + + it("should reject label update when only title is configured", async () => { + const handler = await main({ + target: "*", + allow_title: true, + }); + + const result = await handler({ type: "update_discussion", labels: ["bug"], discussion_number: 42 }, {}); + // labels update is silently skipped (allow_labels is not set, so the labels block is ignored) + // The result is a no-op (no update fields) + expect(result.success).toBe(true); + expect(getUpdateDiscussionMutations()).toHaveLength(0); + expect(getAddLabelsCalls()).toHaveLength(0); + }); + }); + + describe("body-only update", () => { + it("should update body without modifying title or labels", async () => { + const handler = await main({ + target: "*", + allow_body: true, + }); + + const result = await handler({ type: "update_discussion", body: "Updated body content", discussion_number: 42 }, {}); + expect(result.success).toBe(true); + + // The updateDiscussion mutation MUST have been called with the new body + const updateMutations = getUpdateDiscussionMutations(); + expect(updateMutations).toHaveLength(1); + expect(updateMutations[0].variables.body).toContain("Updated body content"); + // Title should preserve the existing value (not change it) + expect(updateMutations[0].variables.title).toBe(defaultDiscussion.title); + + // No label mutations should have been called + expect(getAddLabelsCalls()).toHaveLength(0); + expect(getRemoveLabelsCalls()).toHaveLength(0); + }); + + it("should reject title update when only body is configured", async () => { + const handler = await main({ + target: "*", + allow_body: true, + }); + + const result = await handler({ type: "update_discussion", title: "New Title", discussion_number: 42 }, {}); + expect(result.success).toBe(false); + expect(result.error).toContain("Title updates are not allowed"); + + expect(getUpdateDiscussionMutations()).toHaveLength(0); + }); + }); + + describe("title and body update (no labels)", () => { + it("should update title and body without touching labels", async () => { + const handler = await main({ + target: "*", + allow_title: true, + allow_body: true, + }); + + const result = await handler({ type: "update_discussion", title: "New Title", body: "New body content", discussion_number: 42 }, {}); + expect(result.success).toBe(true); + + const updateMutations = getUpdateDiscussionMutations(); + expect(updateMutations).toHaveLength(1); + expect(updateMutations[0].variables.title).toBe("New Title"); + expect(updateMutations[0].variables.body).toContain("New body content"); + + // No label mutations + expect(getAddLabelsCalls()).toHaveLength(0); + }); + + it("should use existing title when only body is provided even with allow_title", async () => { + const handler = await main({ + target: "*", + allow_title: true, + allow_body: true, + }); + + const result = await handler({ type: "update_discussion", body: "Body only update", discussion_number: 42 }, {}); + expect(result.success).toBe(true); + + const updateMutations = getUpdateDiscussionMutations(); + expect(updateMutations).toHaveLength(1); + // Title should be preserved from the fetched discussion + expect(updateMutations[0].variables.title).toBe(defaultDiscussion.title); + expect(updateMutations[0].variables.body).toContain("Body only update"); + }); + }); + + describe("all fields allowed", () => { + it("should update title, body, and labels when all are configured", async () => { + const handler = await main({ + target: "*", + allow_title: true, + allow_body: true, + allow_labels: true, + allowed_labels: ["Label1", "bug"], + }); + + const result = await handler({ type: "update_discussion", title: "New Title", body: "New body content", labels: ["Label1", "bug"], discussion_number: 42 }, {}); + expect(result.success).toBe(true); + + // updateDiscussion mutation should be called for title/body + const updateMutations = getUpdateDiscussionMutations(); + expect(updateMutations).toHaveLength(1); + expect(updateMutations[0].variables.title).toBe("New Title"); + expect(updateMutations[0].variables.body).toContain("New body content"); + + // Label mutations should be called too + const addCalls = getAddLabelsCalls(); + expect(addCalls).toHaveLength(1); + }); + + it("should update only labels when only labels field is provided even with all fields allowed", async () => { + const handler = await main({ + target: "*", + allow_title: true, + allow_body: true, + allow_labels: true, + }); + + const result = await handler({ type: "update_discussion", labels: ["bug"], discussion_number: 42 }, {}); + expect(result.success).toBe(true); + + // updateDiscussion mutation must NOT be called when only labels are provided + // This is the key behavioral fix: label-only updates don't touch title/body + const updateMutations = getUpdateDiscussionMutations(); + expect(updateMutations).toHaveLength(0); + + // Labels should be updated + const addCalls = getAddLabelsCalls(); + expect(addCalls).toHaveLength(1); + }); + + it("should update only title when only title field is provided even with all fields allowed", async () => { + const handler = await main({ + target: "*", + allow_title: true, + allow_body: true, + allow_labels: true, + }); + + const result = await handler({ type: "update_discussion", title: "Title Only Update", discussion_number: 42 }, {}); + expect(result.success).toBe(true); + + // Only updateDiscussion mutation should be called, no label mutations + const updateMutations = getUpdateDiscussionMutations(); + expect(updateMutations).toHaveLength(1); + expect(updateMutations[0].variables.title).toBe("Title Only Update"); + expect(updateMutations[0].variables.body).toBe(defaultDiscussion.body); + + expect(getAddLabelsCalls()).toHaveLength(0); + expect(getRemoveLabelsCalls()).toHaveLength(0); + }); + }); + + describe("no fields configured", () => { + it("should reject body update when no fields are configured", async () => { + const handler = await main({ + target: "*", + // No allow_title, allow_body, or allow_labels + }); + + const result = await handler({ type: "update_discussion", body: "New body", discussion_number: 42 }, {}); + expect(result.success).toBe(false); + expect(result.error).toContain("Body updates are not allowed"); + + expect(getUpdateDiscussionMutations()).toHaveLength(0); + }); + + it("should reject title update when no fields are configured", async () => { + const handler = await main({ + target: "*", + // No allow_title, allow_body, or allow_labels + }); + + const result = await handler({ type: "update_discussion", title: "New title", discussion_number: 42 }, {}); + expect(result.success).toBe(false); + expect(result.error).toContain("Title updates are not allowed"); + + expect(getUpdateDiscussionMutations()).toHaveLength(0); + }); + }); + + describe("triggering context", () => { + it("should use triggering discussion number when not explicitly provided", async () => { + const handler = await main({ + target: "triggering", + allow_labels: true, + }); + + const result = await handler({ type: "update_discussion", labels: ["bug"] }, {}); + expect(result.success).toBe(true); + + // Should have fetched discussion #42 from context + const fetchCalls = graphqlCalls.filter(c => c.query.includes("discussion(number:")); + expect(fetchCalls.length).toBeGreaterThan(0); + expect(fetchCalls[0].variables.number).toBe(42); + }); + + it("should use explicit discussion_number when provided with target *", async () => { + const handler = await main({ + target: "*", + allow_labels: true, + }); + + const result = await handler({ type: "update_discussion", labels: ["bug"], discussion_number: 99 }, {}); + expect(result.success).toBe(true); + + const fetchCalls = graphqlCalls.filter(c => c.query.includes("discussion(number:")); + expect(fetchCalls.length).toBeGreaterThan(0); + expect(fetchCalls[0].variables.number).toBe(99); + }); + }); + + describe("main factory", () => { + it("should return a handler function", async () => { + const handler = await main({ allow_labels: true }); + expect(typeof handler).toBe("function"); + }); + + it("should return a handler function with empty config", async () => { + const handler = await main(); + expect(typeof handler).toBe("function"); + }); + + it("should return a handler function with labels-only config (bug scenario config)", async () => { + // Corresponds to: safe-outputs: update-discussion: target: "*" allowed-labels: [Label1, Label2] + const handler = await main({ + target: "*", + allow_labels: true, + allowed_labels: ["Label1", "Label2"], + }); + expect(typeof handler).toBe("function"); + }); + }); +}); diff --git a/pkg/workflow/safe_outputs_tools_filtering.go b/pkg/workflow/safe_outputs_tools_filtering.go index efa27ffc1e3..96a483a744a 100644 --- a/pkg/workflow/safe_outputs_tools_filtering.go +++ b/pkg/workflow/safe_outputs_tools_filtering.go @@ -193,6 +193,9 @@ func generateFilteredToolsJSON(data *WorkflowData, markdownPath string) (string, enhancedTool["description"] = enhancedDescription } + // Filter schema fields based on what is configured as allowed + filterToolSchemaFields(enhancedTool, toolName, data.SafeOutputs) + // Add repo parameter to inputSchema if allowed-repos has entries addRepoParameterIfNeeded(enhancedTool, toolName, data.SafeOutputs) @@ -432,6 +435,51 @@ func generateFilteredToolsJSON(data *WorkflowData, markdownPath string) (string, return string(filteredJSON), nil } +// filterToolSchemaFields removes fields from a tool's inputSchema properties +// that are not enabled by the safe output configuration. This prevents the AI +// from attempting to use fields that would be rejected at runtime. +func filterToolSchemaFields(tool map[string]any, toolName string, safeOutputs *SafeOutputsConfig) { + if safeOutputs == nil { + return + } + + switch toolName { + case "update_discussion": + config := safeOutputs.UpdateDiscussions + if config == nil { + return + } + // Deep-copy the inputSchema so we don't modify the shared original + inputSchema, ok := tool["inputSchema"].(map[string]any) + if !ok { + return + } + properties, ok := inputSchema["properties"].(map[string]any) + if !ok { + return + } + // Clone properties and remove fields that are not configured + newProps := make(map[string]any, len(properties)) + maps.Copy(newProps, properties) + if config.Title == nil { + delete(newProps, "title") + } + if config.Body == nil { + delete(newProps, "body") + } + if config.Labels == nil { + delete(newProps, "labels") + } + // Clone inputSchema with new properties + newSchema := make(map[string]any, len(inputSchema)) + maps.Copy(newSchema, inputSchema) + newSchema["properties"] = newProps + tool["inputSchema"] = newSchema + safeOutputsConfigLog.Printf("Filtered update_discussion schema fields: title=%v, body=%v, labels=%v", + config.Title != nil, config.Body != nil, config.Labels != nil) + } +} + // addRepoParameterIfNeeded adds a "repo" parameter to the tool's inputSchema // if the safe output configuration has allowed-repos entries func addRepoParameterIfNeeded(tool map[string]any, toolName string, safeOutputs *SafeOutputsConfig) { diff --git a/pkg/workflow/safe_outputs_tools_test.go b/pkg/workflow/safe_outputs_tools_test.go index 70ae2011041..62c06075256 100644 --- a/pkg/workflow/safe_outputs_tools_test.go +++ b/pkg/workflow/safe_outputs_tools_test.go @@ -660,6 +660,67 @@ func TestEnhanceToolDescription(t *testing.T) { wantContains: []string{"Unknown tool."}, wantNotContains: []string{"CONSTRAINTS:"}, }, + { + name: "update_discussion with labels only", + toolName: "update_discussion", + baseDescription: "Update a discussion.", + safeOutputs: &SafeOutputsConfig{ + UpdateDiscussions: &UpdateDiscussionsConfig{ + AllowedLabels: []string{"Label1", "Label2"}, + Labels: testBoolPtr(true), + }, + }, + wantContains: []string{"CONSTRAINTS:", `Only these labels are allowed: ["Label1" "Label2"]`}, + wantNotContains: []string{"Title updates are allowed.", "Body updates are allowed."}, + }, + { + name: "update_discussion with title and body", + toolName: "update_discussion", + baseDescription: "Update a discussion.", + safeOutputs: &SafeOutputsConfig{ + UpdateDiscussions: &UpdateDiscussionsConfig{ + Title: testBoolPtr(true), + Body: testBoolPtr(true), + }, + }, + wantContains: []string{"CONSTRAINTS:", "Title updates are allowed.", "Body updates are allowed."}, + wantNotContains: []string{"Label updates are allowed."}, + }, + { + name: "update_discussion with all fields", + toolName: "update_discussion", + baseDescription: "Update a discussion.", + safeOutputs: &SafeOutputsConfig{ + UpdateDiscussions: &UpdateDiscussionsConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("3")}, + }, + Title: testBoolPtr(true), + Body: testBoolPtr(true), + Labels: testBoolPtr(true), + AllowedLabels: []string{"bug"}, + }, + }, + wantContains: []string{ + "CONSTRAINTS:", + "Maximum 3 discussion(s) can be updated.", + "Title updates are allowed.", + "Body updates are allowed.", + `Only these labels are allowed: ["bug"]`, + }, + }, + { + name: "update_discussion with labels (no allowed list)", + toolName: "update_discussion", + baseDescription: "Update a discussion.", + safeOutputs: &SafeOutputsConfig{ + UpdateDiscussions: &UpdateDiscussionsConfig{ + Labels: testBoolPtr(true), + }, + }, + wantContains: []string{"CONSTRAINTS:", "Label updates are allowed."}, + wantNotContains: []string{"Only these labels are allowed"}, + }, } for _, tt := range tests { @@ -854,3 +915,241 @@ func TestRepoParameterAddedOnlyWithAllowedRepos(t *testing.T) { }) } } + +// TestFilterUpdateDiscussionSchemaFields verifies that filterToolSchemaFields correctly +// removes fields from the update_discussion tool schema based on what is enabled in config. +func TestFilterUpdateDiscussionSchemaFields(t *testing.T) { + boolPtr := func(b bool) *bool { return &b } + + makeToolWithAllFields := func() map[string]any { + return map[string]any{ + "name": "update_discussion", + "inputSchema": map[string]any{ + "type": "object", + "properties": map[string]any{ + "title": map[string]any{"type": "string"}, + "body": map[string]any{"type": "string"}, + "labels": map[string]any{"type": "array"}, + "discussion_number": map[string]any{"type": "number"}, + "secrecy": map[string]any{"type": "string"}, + "integrity": map[string]any{"type": "string"}, + }, + }, + } + } + + tests := []struct { + name string + safeOutputs *SafeOutputsConfig + expectTitle bool + expectBody bool + expectLabels bool + expectDiscNum bool + expectSecrecy bool + expectIntegrity bool + }{ + { + name: "labels only - title and body removed", + safeOutputs: &SafeOutputsConfig{ + UpdateDiscussions: &UpdateDiscussionsConfig{ + Labels: boolPtr(true), + AllowedLabels: []string{"Label1", "Label2"}, + }, + }, + expectTitle: false, + expectBody: false, + expectLabels: true, + expectDiscNum: true, + expectSecrecy: true, + expectIntegrity: true, + }, + { + name: "title and body only - labels removed", + safeOutputs: &SafeOutputsConfig{ + UpdateDiscussions: &UpdateDiscussionsConfig{ + Title: boolPtr(true), + Body: boolPtr(true), + }, + }, + expectTitle: true, + expectBody: true, + expectLabels: false, + expectDiscNum: true, + expectSecrecy: true, + expectIntegrity: true, + }, + { + name: "all fields enabled", + safeOutputs: &SafeOutputsConfig{ + UpdateDiscussions: &UpdateDiscussionsConfig{ + Title: boolPtr(true), + Body: boolPtr(true), + Labels: boolPtr(true), + }, + }, + expectTitle: true, + expectBody: true, + expectLabels: true, + expectDiscNum: true, + expectSecrecy: true, + expectIntegrity: true, + }, + { + name: "no fields enabled - all content fields removed", + safeOutputs: &SafeOutputsConfig{ + UpdateDiscussions: &UpdateDiscussionsConfig{}, + }, + expectTitle: false, + expectBody: false, + expectLabels: false, + expectDiscNum: true, + expectSecrecy: true, + expectIntegrity: true, + }, + { + name: "nil safe outputs - no changes", + safeOutputs: nil, + expectTitle: true, + expectBody: true, + expectLabels: true, + expectDiscNum: true, + expectSecrecy: true, + expectIntegrity: true, + }, + { + name: "no update_discussion config - no changes", + safeOutputs: &SafeOutputsConfig{ + CreateIssues: &CreateIssuesConfig{}, + }, + expectTitle: true, + expectBody: true, + expectLabels: true, + expectDiscNum: true, + expectSecrecy: true, + expectIntegrity: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tool := makeToolWithAllFields() + filterToolSchemaFields(tool, "update_discussion", tt.safeOutputs) + + inputSchema, ok := tool["inputSchema"].(map[string]any) + require.True(t, ok, "inputSchema should be present") + + properties, ok := inputSchema["properties"].(map[string]any) + require.True(t, ok, "properties should be present") + + _, hasTitle := properties["title"] + _, hasBody := properties["body"] + _, hasLabels := properties["labels"] + _, hasDiscNum := properties["discussion_number"] + _, hasSecrecy := properties["secrecy"] + _, hasIntegrity := properties["integrity"] + + assert.Equal(t, tt.expectTitle, hasTitle, "title field presence mismatch") + assert.Equal(t, tt.expectBody, hasBody, "body field presence mismatch") + assert.Equal(t, tt.expectLabels, hasLabels, "labels field presence mismatch") + assert.Equal(t, tt.expectDiscNum, hasDiscNum, "discussion_number field should always be present") + assert.Equal(t, tt.expectSecrecy, hasSecrecy, "secrecy field should always be present") + assert.Equal(t, tt.expectIntegrity, hasIntegrity, "integrity field should always be present") + }) + } +} + +// TestFilterUpdateDiscussionSchemaFieldsViaGenerateFiltered verifies that the end-to-end +// generateFilteredToolsJSON correctly filters update_discussion fields based on config. +func TestFilterUpdateDiscussionSchemaFieldsViaGenerateFiltered(t *testing.T) { + boolPtr := func(b bool) *bool { return &b } + + tests := []struct { + name string + safeOutputs *SafeOutputsConfig + expectTitle bool + expectBody bool + expectLabels bool + }{ + { + name: "only allowed-labels configured removes title and body from schema", + safeOutputs: &SafeOutputsConfig{ + UpdateDiscussions: &UpdateDiscussionsConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + SafeOutputTargetConfig: SafeOutputTargetConfig{Target: "*"}, + }, + Labels: boolPtr(true), + AllowedLabels: []string{"Label1", "Label2"}, + }, + }, + expectTitle: false, + expectBody: false, + expectLabels: true, + }, + { + name: "title and body configured removes labels from schema", + safeOutputs: &SafeOutputsConfig{ + UpdateDiscussions: &UpdateDiscussionsConfig{ + Title: boolPtr(true), + Body: boolPtr(true), + }, + }, + expectTitle: true, + expectBody: true, + expectLabels: false, + }, + { + name: "all fields configured", + safeOutputs: &SafeOutputsConfig{ + UpdateDiscussions: &UpdateDiscussionsConfig{ + Title: boolPtr(true), + Body: boolPtr(true), + Labels: boolPtr(true), + }, + }, + expectTitle: true, + expectBody: true, + expectLabels: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + data := &WorkflowData{ + SafeOutputs: tt.safeOutputs, + } + + toolsJSON, err := generateFilteredToolsJSON(data, "") + require.NoError(t, err, "generateFilteredToolsJSON should not error") + + var tools []map[string]any + err = json.Unmarshal([]byte(toolsJSON), &tools) + require.NoError(t, err, "tools JSON should be valid") + + // Find update_discussion tool + var discTool map[string]any + for _, tool := range tools { + if tool["name"] == "update_discussion" { + discTool = tool + break + } + } + require.NotNil(t, discTool, "update_discussion tool should be present") + + inputSchema, ok := discTool["inputSchema"].(map[string]any) + require.True(t, ok, "inputSchema should be present") + + properties, ok := inputSchema["properties"].(map[string]any) + require.True(t, ok, "properties should be present") + + _, hasTitle := properties["title"] + _, hasBody := properties["body"] + _, hasLabels := properties["labels"] + _, hasDiscNum := properties["discussion_number"] + + assert.Equal(t, tt.expectTitle, hasTitle, "title field presence mismatch") + assert.Equal(t, tt.expectBody, hasBody, "body field presence mismatch") + assert.Equal(t, tt.expectLabels, hasLabels, "labels field presence mismatch") + assert.True(t, hasDiscNum, "discussion_number should always be present") + }) + } +} diff --git a/pkg/workflow/tool_description_enhancer.go b/pkg/workflow/tool_description_enhancer.go index d8a14a932e5..7a3404e04d0 100644 --- a/pkg/workflow/tool_description_enhancer.go +++ b/pkg/workflow/tool_description_enhancer.go @@ -105,6 +105,29 @@ func enhanceToolDescription(toolName, baseDescription string, safeOutputs *SafeO } } + case "update_discussion": + if config := safeOutputs.UpdateDiscussions; config != nil { + if templatableIntValue(config.Max) > 0 { + constraints = append(constraints, fmt.Sprintf("Maximum %d discussion(s) can be updated.", templatableIntValue(config.Max))) + } + if config.Target != "" { + constraints = append(constraints, fmt.Sprintf("Target: %s.", config.Target)) + } + if config.Title != nil && *config.Title { + constraints = append(constraints, "Title updates are allowed.") + } + if config.Body != nil && *config.Body { + constraints = append(constraints, "Body updates are allowed.") + } + if config.Labels != nil { + if len(config.AllowedLabels) > 0 { + constraints = append(constraints, fmt.Sprintf("Only these labels are allowed: %s.", formatLabelList(config.AllowedLabels))) + } else { + constraints = append(constraints, "Label updates are allowed.") + } + } + } + case "close_issue": if config := safeOutputs.CloseIssues; config != nil { if templatableIntValue(config.Max) > 0 { From 93d51dce69990691e486174fb44be93681c00a5e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 27 Mar 2026 22:05:18 +0000 Subject: [PATCH 3/5] Address code review: align schema filtering with runtime guards, improve warning message clarity Agent-Logs-Url: https://github.com/github/gh-aw/sessions/afab35c0-82a0-49da-a73a-526f8bea768c Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com> --- actions/setup/js/update_discussion.cjs | 3 ++- pkg/workflow/safe_outputs_tools_filtering.go | 12 +++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/actions/setup/js/update_discussion.cjs b/actions/setup/js/update_discussion.cjs index 05b223f8e58..c2e86ea1019 100644 --- a/actions/setup/js/update_discussion.cjs +++ b/actions/setup/js/update_discussion.cjs @@ -183,7 +183,8 @@ async function executeDiscussionUpdate(github, context, discussionNumber, update await replaceDiscussionLabels(github, discussion.id, labelIds); core.info(`Successfully replaced labels on discussion #${discussionNumber}`); } catch (error) { - core.warning(`Discussion #${discussionNumber} label update failed: ${getErrorMessage(error)}`); + const context = hasTitleUpdate || hasBodyUpdate ? "title/body updated successfully but " : ""; + core.warning(`Discussion #${discussionNumber} ${context}label update failed: ${getErrorMessage(error)}`); } } diff --git a/pkg/workflow/safe_outputs_tools_filtering.go b/pkg/workflow/safe_outputs_tools_filtering.go index 96a483a744a..5ef435210b7 100644 --- a/pkg/workflow/safe_outputs_tools_filtering.go +++ b/pkg/workflow/safe_outputs_tools_filtering.go @@ -458,16 +458,18 @@ func filterToolSchemaFields(tool map[string]any, toolName string, safeOutputs *S if !ok { return } - // Clone properties and remove fields that are not configured + // Clone properties and remove fields that are not configured as allowed (nil or false). + // This aligns with the runtime check in update_discussion.cjs which requires + // allow_title/allow_body/allow_labels to be explicitly true. newProps := make(map[string]any, len(properties)) maps.Copy(newProps, properties) - if config.Title == nil { + if config.Title == nil || !*config.Title { delete(newProps, "title") } - if config.Body == nil { + if config.Body == nil || !*config.Body { delete(newProps, "body") } - if config.Labels == nil { + if config.Labels == nil || !*config.Labels { delete(newProps, "labels") } // Clone inputSchema with new properties @@ -476,7 +478,7 @@ func filterToolSchemaFields(tool map[string]any, toolName string, safeOutputs *S newSchema["properties"] = newProps tool["inputSchema"] = newSchema safeOutputsConfigLog.Printf("Filtered update_discussion schema fields: title=%v, body=%v, labels=%v", - config.Title != nil, config.Body != nil, config.Labels != nil) + config.Title != nil && *config.Title, config.Body != nil && *config.Body, config.Labels != nil && *config.Labels) } } From 9b5b8e236bb64ed3711028e761c9f99e66279f2f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 27 Mar 2026 22:19:10 +0000 Subject: [PATCH 4/5] Add title/body field isolation tests: verify each field updates independently without mutating the other Agent-Logs-Url: https://github.com/github/gh-aw/sessions/948b9fb0-81fb-42b5-8bf3-a09a9a9d8e07 Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com> --- actions/setup/js/update_discussion.test.cjs | 74 +++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/actions/setup/js/update_discussion.test.cjs b/actions/setup/js/update_discussion.test.cjs index 948c05c5de5..35772d7ecdf 100644 --- a/actions/setup/js/update_discussion.test.cjs +++ b/actions/setup/js/update_discussion.test.cjs @@ -406,6 +406,80 @@ describe("update_discussion", () => { }); }); + describe("title and body field isolation (independent updates)", () => { + it("updating only title must not mutate the body", async () => { + // Both title and body are allowed, but only title is provided. + // The body sent to the API must equal the existing body exactly (not a new value). + const handler = await main({ + target: "*", + allow_title: true, + allow_body: true, + }); + + const result = await handler({ type: "update_discussion", title: "Updated Title Only", discussion_number: 42 }, {}); + expect(result.success).toBe(true); + + const updateMutations = getUpdateDiscussionMutations(); + expect(updateMutations).toHaveLength(1); + // Title was changed + expect(updateMutations[0].variables.title).toBe("Updated Title Only"); + // Body was NOT changed — must be the original value fetched from the discussion + expect(updateMutations[0].variables.body).toBe(defaultDiscussion.body); + }); + + it("updating only body must not mutate the title", async () => { + // Both title and body are allowed, but only body is provided. + // The title sent to the API must equal the existing title exactly (not a new value). + const handler = await main({ + target: "*", + allow_title: true, + allow_body: true, + }); + + const result = await handler({ type: "update_discussion", body: "Updated body only", discussion_number: 42 }, {}); + expect(result.success).toBe(true); + + const updateMutations = getUpdateDiscussionMutations(); + expect(updateMutations).toHaveLength(1); + // Title was NOT changed — must be the original value fetched from the discussion + expect(updateMutations[0].variables.title).toBe(defaultDiscussion.title); + // Body was changed + expect(updateMutations[0].variables.body).toContain("Updated body only"); + }); + + it("updating title independently does not affect a subsequent body-only update", async () => { + // Simulate two separate handler calls (separate safe output messages). + // First updates title; second updates body. Each must preserve the other. + const handler = await main({ + target: "*", + allow_title: true, + allow_body: true, + }); + + // First call: update title only + const result1 = await handler({ type: "update_discussion", title: "First Title Update", discussion_number: 42 }, {}); + expect(result1.success).toBe(true); + + let mutations = getUpdateDiscussionMutations(); + expect(mutations).toHaveLength(1); + expect(mutations[0].variables.title).toBe("First Title Update"); + expect(mutations[0].variables.body).toBe(defaultDiscussion.body); + + // Clear tracked calls for the second assertion + graphqlCalls.length = 0; + + // Second call: update body only (uses a fresh fetch of the discussion) + const result2 = await handler({ type: "update_discussion", body: "Second Body Update", discussion_number: 42 }, {}); + expect(result2.success).toBe(true); + + mutations = getUpdateDiscussionMutations(); + expect(mutations).toHaveLength(1); + // Title still equals the original (from fetch) — not the value written in call 1 + expect(mutations[0].variables.title).toBe(defaultDiscussion.title); + expect(mutations[0].variables.body).toContain("Second Body Update"); + }); + }); + describe("all fields allowed", () => { it("should update title, body, and labels when all are configured", async () => { const handler = await main({ From 56bbedeeb9f7ec16596e35e1cb3f16fd77df24bd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 27 Mar 2026 23:01:41 +0000 Subject: [PATCH 5/5] Add update-discussion operation spec to safe-outputs-specification.md (v1.1.0) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/cabcb33e-bd21-417c-967f-d8448d7c0a2d Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- scratchpad/safe-outputs-specification.md | 128 ++++++++++++++++++++++- 1 file changed, 126 insertions(+), 2 deletions(-) diff --git a/scratchpad/safe-outputs-specification.md b/scratchpad/safe-outputs-specification.md index f62ba26ca03..5d10a83bd25 100644 --- a/scratchpad/safe-outputs-specification.md +++ b/scratchpad/safe-outputs-specification.md @@ -7,7 +7,7 @@ sidebar: # Safe Outputs System Specification -**Version**: 1.0.0 +**Version**: 1.1.0 **Status**: Recommendation **Latest Version**: https://github.github.com/gh-aw/scratchpad/safe-outputs-specification/ **Editors**: GitHub Next Team @@ -32,6 +32,7 @@ This specification is governed by the GitHub Next team and follows semantic vers 4. [Security Model](#4-security-model) 5. [Builtin System Tools](#5-builtin-system-tools) 6. [GitHub Operations](#6-github-operations) + - [6.4 update-discussion Operation](#64-update-discussion-operation) 7. [Compliance Testing](#7-compliance-testing) 8. [Appendices](#appendices) 9. [References](#references) @@ -822,7 +823,119 @@ Each appendix includes: - Conformance test identifiers - Examples -### 6.4 GitHub Operations Summary Table +### 6.4 update-discussion Operation + +#### 6.4.1 Purpose + +Modifies an existing GitHub Discussion's title, body, or labels. Supports fine-grained field-level access control: each of `title`, `body`, and `labels` is independently gated by its own configuration flag. Only the fields enabled in the workflow configuration are exposed in the MCP tool schema and may be updated at runtime. + +#### 6.4.2 Configuration + +```yaml +safe-outputs: + update-discussion: + target: "triggering" # "triggering" | "*" | + allow-title: true # Permit title updates (default: false) + allow-body: true # Permit body updates (default: false) + allowed-labels: # Restricts agent-requested labels to this list + - Label1 + - Label2 +``` + +`allowed-labels` implicitly enables label updates. At least one of `allow-title`, `allow-body`, or `allowed-labels` MUST be configured. + +#### 6.4.3 Request Schema + +The effective schema is filtered at registration time: properties not enabled by the configuration are omitted from `inputSchema` before the tool is registered with the MCP server. + +**Full schema (all fields enabled):** + +```json +{ + "type": "object", + "properties": { + "discussion_number": { + "type": "integer", + "description": "Discussion number to update. Required when target is \"*\"" + }, + "title": { + "type": "string", + "description": "New discussion title. Only present when allow-title is true" + }, + "body": { + "type": "string", + "description": "New discussion body. Only present when allow-body is true" + }, + "labels": { + "type": "array", + "items": { "type": "string" }, + "description": "Labels to apply. Only present when allowed-labels is configured" + }, + "secrecy": { + "type": "string", + "description": "Confidentiality classification" + }, + "integrity": { + "type": "string", + "description": "Data integrity assertion" + } + }, + "required": [] +} +``` + +**Example — labels-only config:** + +When the configuration specifies only `allowed-labels` (no `allow-title` or `allow-body`), the registered schema exposes only `discussion_number`, `labels`, `secrecy`, and `integrity`. The `title` and `body` properties are absent, preventing the agent from attempting to set them. + +#### 6.4.4 Behavior + +The `update-discussion` handler MUST: + +1. Validate the incoming request against the filtered schema. +2. Resolve the target discussion number: + - `target: "triggering"` — use the discussion number from the workflow event context. + - `target: "*"` — use `discussion_number` from the agent request (required). + - `target: ` — use the configured number unconditionally. +3. Fetch the current discussion to obtain its `id`, `title`, and `body` (required for the `updateDiscussion` GraphQL mutation which is always a full-replace). +4. **If `title` or `body` is present in the request:** + a. Reject `title` with an error if `allow-title` is not `true` in the configuration. + b. Reject `body` with an error if `allow-body` is not `true` in the configuration. + c. Construct the update payload, preserving the fetched value for any field not supplied by the request: + - Omitted `title` → use the existing discussion title unchanged. + - Omitted `body` → use the existing discussion body unchanged. + d. Call the `updateDiscussion` GraphQL mutation with the constructed payload. +5. **If only `labels` is present (no `title` or `body` in the request):** + a. MUST NOT call the `updateDiscussion` GraphQL mutation. + b. MUST NOT modify the discussion title or body. +6. If `labels` is present: + a. Filter the requested labels against `allowed-labels` if configured. + b. Sanitize all labels according to Section 4.3.2. + c. Apply label changes via the GitHub GraphQL API. +7. Return a success result summarising which fields were updated. + +#### 6.4.5 Field Isolation Invariants + +Implementations MUST guarantee: + +- **Title isolation**: Updating only `title` MUST leave the discussion body byte-for-byte identical to the value fetched in step 3. +- **Body isolation**: Updating only `body` MUST leave the discussion title byte-for-byte identical to the value fetched in step 3. +- **Label isolation**: Updating only `labels` MUST NOT invoke any mutation that modifies `title` or `body`. + +#### 6.4.6 Conformance Requirements + +- **T-UPD-001**: MUST expose only schema properties corresponding to enabled configuration fields. +- **T-UPD-002**: MUST reject `title` in the request when `allow-title` is not `true`, returning an actionable error. +- **T-UPD-003**: MUST reject `body` in the request when `allow-body` is not `true`, returning an actionable error. +- **T-UPD-004**: MUST preserve the existing `title` when performing a body-only update. +- **T-UPD-005**: MUST preserve the existing `body` when performing a title-only update. +- **T-UPD-006**: MUST NOT call the `updateDiscussion` mutation when the request contains only `labels`. +- **T-UPD-007**: MUST filter agent-requested labels against `allowed-labels` when configured. +- **T-UPD-008**: MUST sanitize all label values according to Section 4.3.2. +- **T-UPD-009**: MUST resolve `target: "triggering"` to the discussion number from the workflow event context. +- **T-UPD-010**: MUST require `discussion_number` in the request when `target: "*"` is configured. + +### 6.5 GitHub Operations Summary Table | Operation | Max Default | Cross-Repo | Permissions Required | Staged Support | |-----------|-------------|------------|---------------------|----------------| @@ -1087,6 +1200,17 @@ The system does NOT protect against: ## Change Log +### Version 1.1.0 (Recommendation) + +**update-discussion Field Isolation** - 2026-03-27 + +- Added Section 6.4: detailed specification for the `update-discussion` operation +- Defined field-level access control: `allow-title`, `allow-body`, and `allowed-labels` each independently gate schema exposure and runtime execution +- Specified MCP tool schema filtering: fields not enabled in the workflow configuration are removed from `inputSchema` before tool registration, preventing the AI agent from attempting unsupported updates +- Specified runtime guards: requests that include `title` or `body` when the corresponding flag is not set MUST be rejected with a clear error +- Defined field isolation invariants (T-UPD-004 through T-UPD-006): label-only updates MUST NOT call the `updateDiscussion` mutation; title-only / body-only updates MUST preserve the other field exactly as fetched from the API +- Added 10 conformance requirements (T-UPD-001 through T-UPD-010) + ### Version 1.0.0 (Recommendation) **Initial Release** - 2026-01-20