From 75d456e89e5a886ef7d938e1fb5b3a2f5f40a291 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Jul 2026 09:08:32 +0000 Subject: [PATCH 1/7] Initial plan From 1c0e254aae50dc03531c483126810cf79b3bd553 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Jul 2026 09:42:32 +0000 Subject: [PATCH 2/7] fix: correct closeEntity error handling to throw instead of return Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../workflows/dependabot-go-checker.lock.yml | 15 +- .../objective-impact-report.lock.yml | 15 +- .../semantic-function-refactor.lock.yml | 15 +- actions/setup/js/close_issue.cjs | 56 ++++++- actions/setup/js/close_issue.test.cjs | 142 ++++++++++++++++++ .../setup/js/generate_safe_outputs_tools.cjs | 17 ++- .../js/generate_safe_outputs_tools.test.cjs | 115 ++++++++++++++ pkg/workflow/close_entity_helpers.go | 48 +++++- pkg/workflow/safe_outputs_handler_registry.go | 1 + pkg/workflow/safe_outputs_tools_generation.go | 45 ++++++ .../safe_outputs_tools_generation_test.go | 87 +++++++++++ 11 files changed, 546 insertions(+), 10 deletions(-) diff --git a/.github/workflows/dependabot-go-checker.lock.yml b/.github/workflows/dependabot-go-checker.lock.yml index c8c64c5755c..fcc9682be7e 100644 --- a/.github/workflows/dependabot-go-checker.lock.yml +++ b/.github/workflows/dependabot-go-checker.lock.yml @@ -597,7 +597,20 @@ jobs: "create_issue": " CONSTRAINTS: Maximum 10 issue(s) can be created. Title will be prefixed with \"[deps]\". Labels [\"dependencies\" \"go\" \"cookie\"] will be automatically added." }, "repo_params": {}, - "dynamic_tools": [] + "dynamic_tools": [], + "property_injections": { + "close_issue": { + "state_reason": { + "description": "Optional closing state reason. Omit to use the configured default. Select 'duplicate' together with 'duplicate_of' to mark a native duplicate relationship.", + "enum": [ + "completed", + "not_planned", + "duplicate" + ], + "type": "string" + } + } + } } GH_AW_VALIDATION_JSON: | { diff --git a/.github/workflows/objective-impact-report.lock.yml b/.github/workflows/objective-impact-report.lock.yml index 2968ee79b64..e13d4f49be9 100644 --- a/.github/workflows/objective-impact-report.lock.yml +++ b/.github/workflows/objective-impact-report.lock.yml @@ -552,7 +552,20 @@ jobs: "create_issue": " CONSTRAINTS: Maximum 1 issue(s) can be created. Title will be prefixed with \"Impact Efficiency Report - \"." }, "repo_params": {}, - "dynamic_tools": [] + "dynamic_tools": [], + "property_injections": { + "close_issue": { + "state_reason": { + "description": "Optional closing state reason. Omit to use the configured default. Select 'duplicate' together with 'duplicate_of' to mark a native duplicate relationship.", + "enum": [ + "completed", + "not_planned", + "duplicate" + ], + "type": "string" + } + } + } } GH_AW_VALIDATION_JSON: | { diff --git a/.github/workflows/semantic-function-refactor.lock.yml b/.github/workflows/semantic-function-refactor.lock.yml index c551032fd74..7fe85533f44 100644 --- a/.github/workflows/semantic-function-refactor.lock.yml +++ b/.github/workflows/semantic-function-refactor.lock.yml @@ -572,7 +572,20 @@ jobs: "create_issue": " CONSTRAINTS: Maximum 1 issue(s) can be created. Title will be prefixed with \"[refactor] \". Labels [\"refactoring\" \"code-quality\" \"automated-analysis\" \"cookie\"] will be automatically added." }, "repo_params": {}, - "dynamic_tools": [] + "dynamic_tools": [], + "property_injections": { + "close_issue": { + "state_reason": { + "description": "Optional closing state reason. Omit to use the configured default. Select 'duplicate' together with 'duplicate_of' to mark a native duplicate relationship.", + "enum": [ + "completed", + "not_planned", + "duplicate" + ], + "type": "string" + } + } + } } GH_AW_VALIDATION_JSON: | { diff --git a/actions/setup/js/close_issue.cjs b/actions/setup/js/close_issue.cjs index 51af1e91d58..e40145083c9 100644 --- a/actions/setup/js/close_issue.cjs +++ b/actions/setup/js/close_issue.cjs @@ -194,14 +194,35 @@ async function closeIssue(github, owner, repo, issueNumber, stateReason, intentM * @type {HandlerFactoryFunction} */ async function main(config = {}) { - const configStateReason = config.state_reason || "COMPLETED"; + // Determine the state-reason configuration mode: + // - config.state_reason (string): scalar — fixed reason, agent cannot override + // - config.state_reasons (string[]): list — agent may select from this subset + // - neither set: omitted — agent may select from all three supported values + const configStateReason = config.state_reason || null; + /** @type {string[]|null} */ + const configStateReasons = Array.isArray(config.state_reasons) && config.state_reasons.length > 0 ? config.state_reasons : null; + + // The fallback used when the agent does not supply state_reason at item level. + // Scalar config → use the configured value. + // List config → use the first item in the list. + // Omitted → default to "COMPLETED" (backward compatible). + const defaultStateReason = configStateReason || (configStateReasons ? configStateReasons[0] : "COMPLETED"); + const issueIntentEnabled = config.issue_intent !== false; const requiredLabels = config.required_labels || []; const requiredTitlePrefix = config.required_title_prefix || ""; const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); const githubClient = await createAuthenticatedGitHubClient(config); - core.info(`Close issue configuration: max=${config.max || 10}, state_reason=${configStateReason}, issue_intent=${issueIntentEnabled}`); + let stateReasonMode; + if (configStateReason) { + stateReasonMode = `scalar(${configStateReason})`; + } else if (configStateReasons) { + stateReasonMode = `list(${configStateReasons.join(",")})`; + } else { + stateReasonMode = "omitted"; + } + core.info(`Close issue configuration: max=${config.max || 10}, state_reason=${stateReasonMode}, issue_intent=${issueIntentEnabled}`); if (requiredLabels.length > 0) { core.info(`Required labels: ${requiredLabels.join(", ")}`); } @@ -283,8 +304,35 @@ async function main(config = {}) { addComment: addIssueComment, closeEntity(github, owner, repo, entityNumber, item) { - // Support item-level state_reason override, falling back to config-level default - const stateReason = item.state_reason || configStateReason; + // Determine effective state_reason, validating against permitted values when applicable. + let stateReason; + if (item.state_reason !== undefined && item.state_reason !== null) { + const provided = String(item.state_reason); + if (configStateReason) { + // Scalar config: state_reason is fixed; item-level value is accepted as an override + // (it was not exposed in the tool schema, so treat it gracefully). + stateReason = provided; + } else if (configStateReasons) { + // List config: validate against the configured subset. + const upperProvided = provided.toUpperCase(); + const upperAllowed = configStateReasons.map(r => r.toUpperCase()); + if (!upperAllowed.includes(upperProvided)) { + throw new Error(`state_reason "${provided}" is not permitted. Allowed values: ${configStateReasons.join(", ")}`); + } + stateReason = provided; + } else { + // Omitted config: validate against all three supported values. + const upperProvided = provided.toUpperCase(); + const supportedUpper = ["COMPLETED", "NOT_PLANNED", "DUPLICATE"]; + if (!supportedUpper.includes(upperProvided)) { + throw new Error(`state_reason "${provided}" is not a supported value. Supported values: completed, not_planned, duplicate`); + } + stateReason = provided; + } + } else { + stateReason = defaultStateReason; + } + const intentMetadata = issueIntentEnabled ? normalizeIssueIntentMetadata(item) : {}; core.info(`Closing issue #${entityNumber} with state_reason=${stateReason}`); diff --git a/actions/setup/js/close_issue.test.cjs b/actions/setup/js/close_issue.test.cjs index 5061afcc1ea..ec451e32b99 100644 --- a/actions/setup/js/close_issue.test.cjs +++ b/actions/setup/js/close_issue.test.cjs @@ -810,6 +810,111 @@ describe("close_issue", () => { expect(updateCalls[0].state_reason).toBe("duplicate"); }); + it("should use first item in state_reasons list as default when agent omits state_reason", async () => { + const handler = await main({ max: 10, state_reasons: ["not_planned", "duplicate"] }); + const updateCalls = []; + + mockGithub.rest.issues.update = async params => { + updateCalls.push(params); + return { + data: { + number: params.issue_number, + title: "Test Issue", + html_url: `https://github.com/${params.owner}/${params.repo}/issues/${params.issue_number}`, + }, + }; + }; + + const result = await handler({ issue_number: 100, body: "Closing" }, {}); + + expect(result.success).toBe(true); + expect(updateCalls[0].state_reason).toBe("not_planned"); + }); + + it("should accept item-level state_reason from state_reasons list", async () => { + const handler = await main({ max: 10, state_reasons: ["not_planned", "duplicate"] }); + const updateCalls = []; + + mockGithub.rest.issues.update = async params => { + updateCalls.push(params); + return { + data: { + number: params.issue_number, + title: "Test Issue", + html_url: `https://github.com/${params.owner}/${params.repo}/issues/${params.issue_number}`, + }, + }; + }; + + const result = await handler({ issue_number: 100, body: "Duplicate", state_reason: "DUPLICATE" }, {}); + + expect(result.success).toBe(true); + expect(updateCalls[0].state_reason).toBe("duplicate"); + }); + + it("should reject item-level state_reason not in state_reasons list", async () => { + const handler = await main({ max: 10, state_reasons: ["not_planned", "duplicate"] }); + + const result = await handler({ issue_number: 100, body: "Completed", state_reason: "COMPLETED" }, {}); + + expect(result.success).toBe(false); + expect(result.error).toMatch(/not permitted/i); + expect(result.error).toMatch(/COMPLETED/); + }); + + it("should accept item-level state_reason for omitted config (all three values)", async () => { + const handler = await main({ max: 10 }); + const updateCalls = []; + + mockGithub.rest.issues.update = async params => { + updateCalls.push(params); + return { + data: { + number: params.issue_number, + title: "Test Issue", + html_url: `https://github.com/${params.owner}/${params.repo}/issues/${params.issue_number}`, + }, + }; + }; + + for (const reason of ["COMPLETED", "NOT_PLANNED", "DUPLICATE"]) { + updateCalls.length = 0; + const result = await handler({ issue_number: 100, body: "Closing", state_reason: reason }, {}); + expect(result.success).toBe(true); + expect(updateCalls[0].state_reason).toBe(reason.toLowerCase()); + } + }); + + it("should reject invalid state_reason for omitted config", async () => { + const handler = await main({ max: 10 }); + + const result = await handler({ issue_number: 100, body: "Closing", state_reason: "INVALID" }, {}); + + expect(result.success).toBe(false); + expect(result.error).toMatch(/not a supported value/i); + }); + + it("should default to COMPLETED when omitted config and agent omits state_reason", async () => { + const handler = await main({ max: 10 }); + const updateCalls = []; + + mockGithub.rest.issues.update = async params => { + updateCalls.push(params); + return { + data: { + number: params.issue_number, + title: "Test Issue", + html_url: `https://github.com/${params.owner}/${params.repo}/issues/${params.issue_number}`, + }, + }; + }; + + const result = await handler({ issue_number: 100, body: "Closing" }, {}); + + expect(result.success).toBe(true); + expect(updateCalls[0].state_reason).toBe("completed"); + }); + it("should resolve temporary ID in issue_number field", async () => { const handler = await main({ max: 10 }); const updateCalls = []; @@ -1320,5 +1425,42 @@ describe("close_issue", () => { expect(graphqlCalls.length).toBe(0); expect(mockCore.warnings.some(w => w.includes("not DUPLICATE"))).toBe(true); }); + + it("should mark native duplicate when using state_reasons list with DUPLICATE and duplicate_of", async () => { + const handler = await main({ max: 10, state_reasons: ["not_planned", "duplicate"] }); + const graphqlCalls = []; + + mockGithub.rest.issues.get = async ({ owner, repo, issue_number }) => ({ + data: { + number: issue_number, + title: "Test Issue", + labels: [], + html_url: `https://github.com/${owner}/${repo}/issues/${issue_number}`, + state: "open", + node_id: `node_${owner}_${repo}_${issue_number}`, + }, + }); + + mockGithub.rest.issues.update = async params => ({ + data: { + number: params.issue_number, + title: "Test Issue", + html_url: `https://github.com/${params.owner}/${params.repo}/issues/${params.issue_number}`, + node_id: `node_${params.owner}_${params.repo}_${params.issue_number}`, + }, + }); + + mockGithub.graphql = async (mutation, variables) => { + graphqlCalls.push(variables); + return { markAsDuplicate: { duplicate: { id: variables.duplicateId, number: 1003 } } }; + }; + + const result = await handler({ issue_number: 1003, body: "Duplicate report", state_reason: "DUPLICATE", duplicate_of: 50, suggest: true, rationale: "Same CSV defect.", confidence: "HIGH" }, {}); + + expect(result.success).toBe(true); + expect(graphqlCalls.length).toBeGreaterThan(0); + const mutation = graphqlCalls.find(c => "duplicateId" in c || "canonicalId" in c); + expect(mutation).toBeDefined(); + }); }); }); diff --git a/actions/setup/js/generate_safe_outputs_tools.cjs b/actions/setup/js/generate_safe_outputs_tools.cjs index 1cc3fa54396..7822e3062ea 100644 --- a/actions/setup/js/generate_safe_outputs_tools.cjs +++ b/actions/setup/js/generate_safe_outputs_tools.cjs @@ -252,7 +252,7 @@ async function main() { } // Load tools meta (description suffixes, repo params, dynamic tools) - /** @type {{description_suffixes?: Record, repo_params?: Record, dynamic_tools?: Array, required_field_removals?: Record, required_field_additions?: Record}} */ + /** @type {{description_suffixes?: Record, repo_params?: Record, dynamic_tools?: Array, required_field_removals?: Record, required_field_additions?: Record, property_injections?: Record>}} */ let toolsMeta = { description_suffixes: {}, repo_params: {}, dynamic_tools: [] }; if (fs.existsSync(toolsMetaPath)) { try { @@ -367,6 +367,21 @@ async function main() { enhancedTool.inputSchema.required = Array.from(new Set([...existingRequired, ...requiredAdditions])); } + // Inject or override properties in inputSchema based on workflow configuration. + // Used for dynamic fields like state_reason whose schema depends on config. + const propertyInjections = toolsMeta.property_injections?.[tool.name]; + if (propertyInjections && typeof propertyInjections === "object") { + if (!enhancedTool.inputSchema) { + enhancedTool.inputSchema = { type: "object", properties: {} }; + } + if (!enhancedTool.inputSchema.properties) { + enhancedTool.inputSchema.properties = {}; + } + for (const [propName, propSchema] of Object.entries(propertyInjections)) { + enhancedTool.inputSchema.properties[propName] = propSchema; + } + } + applyAssignMilestoneAlternativeRequirements(enhancedTool); return enhancedTool; diff --git a/actions/setup/js/generate_safe_outputs_tools.test.cjs b/actions/setup/js/generate_safe_outputs_tools.test.cjs index e0a6a1a7140..31571171671 100644 --- a/actions/setup/js/generate_safe_outputs_tools.test.cjs +++ b/actions/setup/js/generate_safe_outputs_tools.test.cjs @@ -851,4 +851,119 @@ describe("generate_safe_outputs_tools", () => { // Must still contain the intent required suffix expect(tool.description).toContain("INTENT REQUIRED:"); }); + + it("injects property_injections from tools_meta into tool inputSchema", () => { + fs.writeFileSync( + toolsSourcePath, + JSON.stringify([ + { + name: "close_issue", + description: "Closes issue.", + inputSchema: { + type: "object", + required: ["body"], + properties: { + body: { type: "string", description: "Closing comment." }, + }, + additionalProperties: false, + }, + }, + ]) + ); + fs.writeFileSync(configPath, JSON.stringify({ close_issue: {} })); + fs.writeFileSync( + toolsMetaPath, + JSON.stringify({ + description_suffixes: {}, + repo_params: {}, + dynamic_tools: [], + property_injections: { + close_issue: { + state_reason: { + type: "string", + enum: ["completed", "not_planned", "duplicate"], + description: "Optional closing state reason.", + }, + }, + }, + }) + ); + + runScript(); + + const result = JSON.parse(fs.readFileSync(outputPath, "utf8")); + const tool = result.find((/** @type {{name: string}} */ t) => t.name === "close_issue"); + expect(tool).toBeDefined(); + expect(tool.inputSchema.properties.state_reason).toBeDefined(); + expect(tool.inputSchema.properties.state_reason.type).toBe("string"); + expect(tool.inputSchema.properties.state_reason.enum).toEqual(["completed", "not_planned", "duplicate"]); + // Injected property should NOT be required + expect(tool.inputSchema.required || []).not.toContain("state_reason"); + }); + + it("injects property_injections with restricted enum for list state-reason config", () => { + fs.writeFileSync( + toolsSourcePath, + JSON.stringify([ + { + name: "close_issue", + description: "Closes issue.", + inputSchema: { type: "object", required: ["body"], properties: { body: { type: "string" } }, additionalProperties: false }, + }, + ]) + ); + fs.writeFileSync(configPath, JSON.stringify({ close_issue: { state_reasons: ["not_planned", "duplicate"] } })); + fs.writeFileSync( + toolsMetaPath, + JSON.stringify({ + description_suffixes: {}, + repo_params: {}, + dynamic_tools: [], + property_injections: { + close_issue: { + state_reason: { + type: "string", + enum: ["not_planned", "duplicate"], + description: "Optional closing state reason.", + }, + }, + }, + }) + ); + + runScript(); + + const result = JSON.parse(fs.readFileSync(outputPath, "utf8")); + const tool = result.find((/** @type {{name: string}} */ t) => t.name === "close_issue"); + expect(tool.inputSchema.properties.state_reason.enum).toEqual(["not_planned", "duplicate"]); + }); + + it("does not inject state_reason when property_injections is absent (scalar state-reason config)", () => { + fs.writeFileSync( + toolsSourcePath, + JSON.stringify([ + { + name: "close_issue", + description: "Closes issue.", + inputSchema: { type: "object", required: ["body"], properties: { body: { type: "string" } }, additionalProperties: false }, + }, + ]) + ); + fs.writeFileSync(configPath, JSON.stringify({ close_issue: { state_reason: "not_planned" } })); + fs.writeFileSync( + toolsMetaPath, + JSON.stringify({ + description_suffixes: {}, + repo_params: {}, + dynamic_tools: [], + // No property_injections — scalar state-reason config + }) + ); + + runScript(); + + const result = JSON.parse(fs.readFileSync(outputPath, "utf8")); + const tool = result.find((/** @type {{name: string}} */ t) => t.name === "close_issue"); + expect(tool.inputSchema.properties.state_reason).toBeUndefined(); + }); }); diff --git a/pkg/workflow/close_entity_helpers.go b/pkg/workflow/close_entity_helpers.go index 99daa1744ed..97555042020 100644 --- a/pkg/workflow/close_entity_helpers.go +++ b/pkg/workflow/close_entity_helpers.go @@ -82,8 +82,9 @@ type CloseEntityConfig struct { SafeOutputTargetConfig `yaml:",inline"` SafeOutputFilterConfig `yaml:",inline"` SafeOutputDiscussionFilterConfig `yaml:",inline"` // Only used for discussions - StateReason string `yaml:"state-reason,omitempty"` // Only used for issues - AllowBody *bool `yaml:"allow-body,omitempty"` // If false, any body provided by the agent is dropped with a warning; close proceeds without a comment + StateReason string `yaml:"state-reason,omitempty"` // Only used for issues. Scalar: fixed reason. Mutually exclusive with StateReasons. + StateReasons []string `yaml:"state-reasons,omitempty"` // Only used for issues. List: agent selects from this subset. Populated by preprocessing when state-reason is a YAML sequence. + AllowBody *bool `yaml:"allow-body,omitempty"` // If false, any body provided by the agent is dropped with a warning; close proceeds without a comment } // CloseEntityJobParams holds the parameters needed to build a close entity job @@ -116,6 +117,18 @@ func (c *Compiler) parseCloseEntityConfig(outputMap map[string]any, params Close return nil } + // Pre-process state-reason: when the value is a sequence (list) rather than a scalar, + // move it to "state-reasons" so it unmarshals into StateReasons []string instead of + // the scalar StateReason string field. This supports the list form: + // state-reason: [not_planned, duplicate] + if configData != nil { + if raw, exists := configData["state-reason"]; exists { + if preprocessStateReasonList(configData, raw, logger) { + logger.Printf("state-reason list form detected for %s; moved to state-reasons", params.ConfigKey) + } + } + } + config := parseConfigScaffold(outputMap, params.ConfigKey, logger, func(err error) *CloseEntityConfig { logger.Printf("Failed to unmarshal config: %v", err) // For backward compatibility, handle nil/empty config @@ -141,6 +154,37 @@ func (c *Compiler) parseCloseEntityConfig(outputMap map[string]any, params Close return config } +// preprocessStateReasonList converts a list-form state-reason value into the state-reasons key. +// Returns true if the value was a list and was successfully converted. +func preprocessStateReasonList(configData map[string]any, raw any, logger *logger.Logger) bool { + switch v := raw.(type) { + case []any: + reasons := make([]string, 0, len(v)) + for _, elem := range v { + s, ok := elem.(string) + if !ok { + if logger != nil { + logger.Printf("state-reason list contains non-string element %T; ignoring", elem) + } + continue + } + reasons = append(reasons, s) + } + if len(reasons) > 0 { + configData["state-reasons"] = reasons + } + delete(configData, "state-reason") + return true + case []string: + if len(v) > 0 { + configData["state-reasons"] = v + } + delete(configData, "state-reason") + return true + } + return false +} + // closeEntityDefinition holds all parameters for a close entity type type closeEntityDefinition struct { EntityType CloseEntityType diff --git a/pkg/workflow/safe_outputs_handler_registry.go b/pkg/workflow/safe_outputs_handler_registry.go index 6e91b810fbc..3a4c2b16220 100644 --- a/pkg/workflow/safe_outputs_handler_registry.go +++ b/pkg/workflow/safe_outputs_handler_registry.go @@ -109,6 +109,7 @@ var handlerRegistry = map[string]handlerBuilder{ AddIfNotEmpty("target-repo", c.TargetRepoSlug). AddStringSlice("allowed_repos", c.AllowedRepos). AddIfNotEmpty("state_reason", c.StateReason). + AddStringSlice("state_reasons", c.StateReasons). AddBoolPtr("allow_body", c.AllowBody). AddBoolPtr("issue_intent", c.IssueIntent). AddTemplatableBool("staged", templatableBoolPtrToStringPtr(c.Staged)). diff --git a/pkg/workflow/safe_outputs_tools_generation.go b/pkg/workflow/safe_outputs_tools_generation.go index 508fc6211ac..36789cb3710 100644 --- a/pkg/workflow/safe_outputs_tools_generation.go +++ b/pkg/workflow/safe_outputs_tools_generation.go @@ -222,6 +222,12 @@ type ToolsMeta struct { // inputSchema.required array. Used when a field that is optional in the static // safe_outputs_tools.json should be required for this specific workflow. RequiredFieldAdditions map[string][]string `json:"required_field_additions,omitempty"` + // PropertyInjections maps tool name → property name → full JSON Schema property definition. + // Used when a property should be injected into a tool's inputSchema at runtime based on + // workflow configuration (e.g., a state_reason enum restricted to configured values). + // If the property already exists in the static schema its definition is replaced; + // otherwise it is added as a new optional property. + PropertyInjections map[string]map[string]any `json:"property_injections,omitempty"` } // computeRequiredFieldRemovals returns a map of tool name → required fields to remove @@ -277,6 +283,41 @@ func issueIntentRequired(issueIntent *bool) bool { return issueIntent != nil && *issueIntent } +// closeIssueStateReasonValues is the full set of supported state reasons for close_issue. +var closeIssueStateReasonValues = []string{"completed", "not_planned", "duplicate"} + +// computePropertyInjections returns a map of tool name → property name → property schema +// for properties that must be injected into the tool schema based on workflow configuration. +// +// Currently handles close_issue state_reason: +// - Omitted config (no state-reason): inject state_reason with all three supported values. +// - List config (state-reason: [...]): inject state_reason with the configured subset. +// - Scalar config (state-reason: "..."): no injection (fixed reason, agent cannot choose). +func computePropertyInjections(safeOutputs *SafeOutputsConfig) map[string]map[string]any { + injections := make(map[string]map[string]any) + if safeOutputs == nil || safeOutputs.CloseIssues == nil { + return injections + } + c := safeOutputs.CloseIssues + // Scalar config: agent cannot change state_reason; do not expose the field. + if c.StateReason != "" { + return injections + } + // List or omitted: expose state_reason with the permitted enum. + enumValues := c.StateReasons + if len(enumValues) == 0 { + enumValues = closeIssueStateReasonValues + } + injections["close_issue"] = map[string]any{ + "state_reason": map[string]any{ + "type": "string", + "enum": enumValues, + "description": "Optional closing state reason. Omit to use the configured default. Select 'duplicate' together with 'duplicate_of' to mark a native duplicate relationship.", + }, + } + return injections +} + // generateToolsMetaJSON generates the content for tools_meta.json: a compact file // that captures the workflow-specific customisations (description constraints, // repo parameters, dynamic tools) without inlining the entire @@ -336,12 +377,16 @@ func generateToolsMetaJSON(data *WorkflowData, markdownPath string) (string, err requiredFieldRemovals := computeRequiredFieldRemovals(data.SafeOutputs) requiredFieldAdditions := computeRequiredFieldAdditions(data.SafeOutputs) + // Compute property injections (e.g. state_reason enum for close_issue). + propertyInjections := computePropertyInjections(data.SafeOutputs) + meta := ToolsMeta{ DescriptionSuffixes: descriptionSuffixes, RepoParams: repoParams, DynamicTools: dynamicTools, RequiredFieldRemovals: requiredFieldRemovals, RequiredFieldAdditions: requiredFieldAdditions, + PropertyInjections: propertyInjections, } result, err := json.MarshalIndent(meta, "", " ") diff --git a/pkg/workflow/safe_outputs_tools_generation_test.go b/pkg/workflow/safe_outputs_tools_generation_test.go index 8aff34b8661..163799ec6a4 100644 --- a/pkg/workflow/safe_outputs_tools_generation_test.go +++ b/pkg/workflow/safe_outputs_tools_generation_test.go @@ -436,3 +436,90 @@ func TestComputeRequiredFieldAdditionsIssueIntentOptIn(t *testing.T) { assert.Equal(t, []string{"rationale", "confidence"}, additions["assign_to_user"]) assert.Equal(t, []string{"rationale", "confidence"}, additions["assign_to_agent"]) } + +// TestComputePropertyInjectionsOmittedStateReason verifies that when close-issue has no +// state-reason configured, state_reason is injected with all three supported values. +func TestComputePropertyInjectionsOmittedStateReason(t *testing.T) { + injections := computePropertyInjections(&SafeOutputsConfig{ + CloseIssues: &CloseIssuesConfig{}, + }) + + require.Contains(t, injections, "close_issue") + require.Contains(t, injections["close_issue"], "state_reason") + prop, ok := injections["close_issue"]["state_reason"].(map[string]any) + require.True(t, ok, "state_reason should be a property map") + assert.Equal(t, "string", prop["type"]) + assert.Equal(t, []string{"completed", "not_planned", "duplicate"}, prop["enum"]) +} + +// TestComputePropertyInjectionsListStateReason verifies that a list state-reason injects +// only the configured values into the state_reason enum. +func TestComputePropertyInjectionsListStateReason(t *testing.T) { + injections := computePropertyInjections(&SafeOutputsConfig{ + CloseIssues: &CloseIssuesConfig{ + StateReasons: []string{"not_planned", "duplicate"}, + }, + }) + + require.Contains(t, injections, "close_issue") + prop, ok := injections["close_issue"]["state_reason"].(map[string]any) + require.True(t, ok) + assert.Equal(t, []string{"not_planned", "duplicate"}, prop["enum"]) +} + +// TestComputePropertyInjectionsScalarStateReason verifies that a scalar state-reason does +// NOT inject state_reason (the agent cannot choose). +func TestComputePropertyInjectionsScalarStateReason(t *testing.T) { + injections := computePropertyInjections(&SafeOutputsConfig{ + CloseIssues: &CloseIssuesConfig{ + StateReason: "not_planned", + }, + }) + + assert.NotContains(t, injections, "close_issue", "scalar state-reason should not inject state_reason into tool schema") +} + +// TestComputePropertyInjectionsNilConfig verifies that nil config returns empty map. +func TestComputePropertyInjectionsNilConfig(t *testing.T) { + injections := computePropertyInjections(nil) + assert.Empty(t, injections) +} + +// TestComputePropertyInjectionsNilCloseIssues verifies that nil close issues returns empty map. +func TestComputePropertyInjectionsNilCloseIssues(t *testing.T) { + injections := computePropertyInjections(&SafeOutputsConfig{}) + assert.Empty(t, injections) +} + +// TestPreprocessStateReasonListSlice verifies that a []any slice is converted to state-reasons. +func TestPreprocessStateReasonListSlice(t *testing.T) { + configData := map[string]any{ + "state-reason": []any{"not_planned", "duplicate"}, + } + result := preprocessStateReasonList(configData, configData["state-reason"], nil) + assert.True(t, result) + assert.NotContains(t, configData, "state-reason", "state-reason key should be removed") + assert.Equal(t, []string{"not_planned", "duplicate"}, configData["state-reasons"]) +} + +// TestPreprocessStateReasonListStringSlice verifies that a []string slice is converted to state-reasons. +func TestPreprocessStateReasonListStringSlice(t *testing.T) { + configData := map[string]any{ + "state-reason": []string{"completed", "not_planned"}, + } + result := preprocessStateReasonList(configData, configData["state-reason"], nil) + assert.True(t, result) + assert.NotContains(t, configData, "state-reason") + assert.Equal(t, []string{"completed", "not_planned"}, configData["state-reasons"]) +} + +// TestPreprocessStateReasonListScalarNotConverted verifies that a scalar string is not touched. +func TestPreprocessStateReasonListScalarNotConverted(t *testing.T) { + configData := map[string]any{ + "state-reason": "not_planned", + } + result := preprocessStateReasonList(configData, configData["state-reason"], nil) + assert.False(t, result) + assert.Equal(t, "not_planned", configData["state-reason"], "scalar should remain unchanged") + assert.NotContains(t, configData, "state-reasons") +} From 3a5befd62b35b46a514e9e166da6cd65abc2495c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 21 Jul 2026 09:55:19 +0000 Subject: [PATCH 3/7] docs(adr): add draft ADR-47045 for close_issue runtime state-reason selection Co-Authored-By: Claude Sonnet 4.6 --- ...llow-close-issue-state-reason-selection.md | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 docs/adr/47045-allow-close-issue-state-reason-selection.md diff --git a/docs/adr/47045-allow-close-issue-state-reason-selection.md b/docs/adr/47045-allow-close-issue-state-reason-selection.md new file mode 100644 index 00000000000..d240496d232 --- /dev/null +++ b/docs/adr/47045-allow-close-issue-state-reason-selection.md @@ -0,0 +1,46 @@ +# ADR-47045: Allow close_issue Agents to Select State-Reason at Runtime + +**Date**: 2026-07-21 +**Status**: Draft +**Deciders**: pelikhan, copilot-swe-agent + +--- + +### Context + +The `close_issue` safe-output tool previously only accepted a scalar `state-reason` configuration, which fixed the closing reason at workflow-author time. This prevented agents from selecting `duplicate` even when they identified clear duplicate evidence during triage. Workflow authors need three distinct operating modes: a locked reason for strict workflows, a restricted subset for mixed-intent workflows, and full agent freedom for general-purpose agents. The existing single-field config cannot express the omitted (agent-free-choice) and list (agent-restricted) modes, forcing a workaround or silent misconfiguration. + +### Decision + +We will extend `CloseEntityConfig` with a parallel `StateReasons []string` field and a YAML preprocessing step that converts a list-form `state-reason: [...]` value to the `state-reasons` key before unmarshaling. We will also add a `PropertyInjections` map to `ToolsMeta` so the Go compiler can inject a `state_reason` enum into the `close_issue` tool's JSON Schema at compile time, reflecting the permitted values for the configured mode. The JS handler validates agent-provided `state_reason` values against the permitted set at runtime and throws on invalid choices so the wrapper surfaces `{success: false}`. + +### Alternatives Considered + +#### Alternative 1: Add a separate `allow-duplicate` boolean flag + +Add a single `allow-duplicate: true` flag alongside the existing scalar `state-reason`. This is simpler but does not generalize: it cannot restrict the agent to a specific subset (e.g., `[not_planned, duplicate]`), and does not address the need to allow `completed` as an agent-selectable option. It would require additional flags for each future per-reason use case, creating an unbounded set of boolean knobs. + +#### Alternative 2: Remove `state-reason` config and always allow agent to choose + +Remove the scalar `state-reason` entirely so agents always choose from all supported values. This simplifies the config surface but breaks backward compatibility for workflows that rely on a fixed closing reason (e.g., always `not_planned` regardless of agent intent), which is the existing documented behavior. It also removes the ability for strict workflows to enforce a single reason. + +### Consequences + +#### Positive +- Agents can now select `duplicate` (or any configured subset) when appropriate, enabling correct native duplicate linking via `duplicate_of` +- Fully backward-compatible: existing scalar `state-reason: value` configs continue to behave identically +- The `close_issue` tool's JSON Schema reflects the actual permitted enum at runtime, giving the agent accurate self-documentation +- The three-way contract (scalar / list / omitted) is explicit and tested across all code layers (Go config parsing, JS handler, tool schema generation) + +#### Negative +- Two parallel YAML keys (`state-reason` for scalar and `state-reasons` for list) coexist in the config struct; the list form is authored as `state-reason: [...]` but stored under `state-reasons`, which is non-obvious +- YAML preprocessing adds a non-obvious indirection in the config parser that future maintainers must understand before modifying config deserialization +- Property injection via `computePropertyInjections` is currently hardcoded to the `close_issue`/`state_reason` case; any future dynamic-enum tool will require extending this function + +#### Neutral +- Eight new Go unit tests and several JS unit tests are added to cover the three config modes and edge cases +- Existing lock files for deployed workflows are regenerated to include the new `property_injections` field, reflecting the omitted-config default (all three values) + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From e575777de5f18bc869de0bbcc36f742ed0bd310a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Jul 2026 09:56:15 +0000 Subject: [PATCH 4/7] refactor: rename state-reasons to allowed-state-reason Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/close_issue.cjs | 4 ++-- actions/setup/js/close_issue.test.cjs | 16 ++++++++-------- .../js/generate_safe_outputs_tools.test.cjs | 2 +- pkg/workflow/close_entity_helpers.go | 16 ++++++++-------- pkg/workflow/safe_outputs_handler_registry.go | 2 +- pkg/workflow/safe_outputs_tools_generation.go | 2 +- .../safe_outputs_tools_generation_test.go | 10 +++++----- 7 files changed, 26 insertions(+), 26 deletions(-) diff --git a/actions/setup/js/close_issue.cjs b/actions/setup/js/close_issue.cjs index e40145083c9..fa3dc2357c0 100644 --- a/actions/setup/js/close_issue.cjs +++ b/actions/setup/js/close_issue.cjs @@ -196,11 +196,11 @@ async function closeIssue(github, owner, repo, issueNumber, stateReason, intentM async function main(config = {}) { // Determine the state-reason configuration mode: // - config.state_reason (string): scalar — fixed reason, agent cannot override - // - config.state_reasons (string[]): list — agent may select from this subset + // - config.allowed_state_reason (string[]): list — agent may select from this subset // - neither set: omitted — agent may select from all three supported values const configStateReason = config.state_reason || null; /** @type {string[]|null} */ - const configStateReasons = Array.isArray(config.state_reasons) && config.state_reasons.length > 0 ? config.state_reasons : null; + const configStateReasons = Array.isArray(config.allowed_state_reason) && config.allowed_state_reason.length > 0 ? config.allowed_state_reason : null; // The fallback used when the agent does not supply state_reason at item level. // Scalar config → use the configured value. diff --git a/actions/setup/js/close_issue.test.cjs b/actions/setup/js/close_issue.test.cjs index ec451e32b99..12d6111519f 100644 --- a/actions/setup/js/close_issue.test.cjs +++ b/actions/setup/js/close_issue.test.cjs @@ -810,8 +810,8 @@ describe("close_issue", () => { expect(updateCalls[0].state_reason).toBe("duplicate"); }); - it("should use first item in state_reasons list as default when agent omits state_reason", async () => { - const handler = await main({ max: 10, state_reasons: ["not_planned", "duplicate"] }); + it("should use first item in allowed_state_reason list as default when agent omits state_reason", async () => { + const handler = await main({ max: 10, allowed_state_reason: ["not_planned", "duplicate"] }); const updateCalls = []; mockGithub.rest.issues.update = async params => { @@ -831,8 +831,8 @@ describe("close_issue", () => { expect(updateCalls[0].state_reason).toBe("not_planned"); }); - it("should accept item-level state_reason from state_reasons list", async () => { - const handler = await main({ max: 10, state_reasons: ["not_planned", "duplicate"] }); + it("should accept item-level state_reason from allowed_state_reason list", async () => { + const handler = await main({ max: 10, allowed_state_reason: ["not_planned", "duplicate"] }); const updateCalls = []; mockGithub.rest.issues.update = async params => { @@ -852,8 +852,8 @@ describe("close_issue", () => { expect(updateCalls[0].state_reason).toBe("duplicate"); }); - it("should reject item-level state_reason not in state_reasons list", async () => { - const handler = await main({ max: 10, state_reasons: ["not_planned", "duplicate"] }); + it("should reject item-level state_reason not in allowed_state_reason list", async () => { + const handler = await main({ max: 10, allowed_state_reason: ["not_planned", "duplicate"] }); const result = await handler({ issue_number: 100, body: "Completed", state_reason: "COMPLETED" }, {}); @@ -1426,8 +1426,8 @@ describe("close_issue", () => { expect(mockCore.warnings.some(w => w.includes("not DUPLICATE"))).toBe(true); }); - it("should mark native duplicate when using state_reasons list with DUPLICATE and duplicate_of", async () => { - const handler = await main({ max: 10, state_reasons: ["not_planned", "duplicate"] }); + it("should mark native duplicate when using allowed_state_reason list with DUPLICATE and duplicate_of", async () => { + const handler = await main({ max: 10, allowed_state_reason: ["not_planned", "duplicate"] }); const graphqlCalls = []; mockGithub.rest.issues.get = async ({ owner, repo, issue_number }) => ({ diff --git a/actions/setup/js/generate_safe_outputs_tools.test.cjs b/actions/setup/js/generate_safe_outputs_tools.test.cjs index 31571171671..e15631f6eb0 100644 --- a/actions/setup/js/generate_safe_outputs_tools.test.cjs +++ b/actions/setup/js/generate_safe_outputs_tools.test.cjs @@ -912,7 +912,7 @@ describe("generate_safe_outputs_tools", () => { }, ]) ); - fs.writeFileSync(configPath, JSON.stringify({ close_issue: { state_reasons: ["not_planned", "duplicate"] } })); + fs.writeFileSync(configPath, JSON.stringify({ close_issue: { allowed_state_reason: ["not_planned", "duplicate"] } })); fs.writeFileSync( toolsMetaPath, JSON.stringify({ diff --git a/pkg/workflow/close_entity_helpers.go b/pkg/workflow/close_entity_helpers.go index 97555042020..d2b3b1b05ab 100644 --- a/pkg/workflow/close_entity_helpers.go +++ b/pkg/workflow/close_entity_helpers.go @@ -82,9 +82,9 @@ type CloseEntityConfig struct { SafeOutputTargetConfig `yaml:",inline"` SafeOutputFilterConfig `yaml:",inline"` SafeOutputDiscussionFilterConfig `yaml:",inline"` // Only used for discussions - StateReason string `yaml:"state-reason,omitempty"` // Only used for issues. Scalar: fixed reason. Mutually exclusive with StateReasons. - StateReasons []string `yaml:"state-reasons,omitempty"` // Only used for issues. List: agent selects from this subset. Populated by preprocessing when state-reason is a YAML sequence. - AllowBody *bool `yaml:"allow-body,omitempty"` // If false, any body provided by the agent is dropped with a warning; close proceeds without a comment + StateReason string `yaml:"state-reason,omitempty"` // Only used for issues. Scalar: fixed reason. Mutually exclusive with AllowedStateReason. + AllowedStateReason []string `yaml:"allowed-state-reason,omitempty"` // Only used for issues. List: agent selects from this subset. + AllowBody *bool `yaml:"allow-body,omitempty"` // If false, any body provided by the agent is dropped with a warning; close proceeds without a comment } // CloseEntityJobParams holds the parameters needed to build a close entity job @@ -118,13 +118,13 @@ func (c *Compiler) parseCloseEntityConfig(outputMap map[string]any, params Close } // Pre-process state-reason: when the value is a sequence (list) rather than a scalar, - // move it to "state-reasons" so it unmarshals into StateReasons []string instead of - // the scalar StateReason string field. This supports the list form: + // move it to "allowed-state-reason" so it unmarshals into AllowedStateReason []string + // instead of the scalar StateReason string field. This supports the list form: // state-reason: [not_planned, duplicate] if configData != nil { if raw, exists := configData["state-reason"]; exists { if preprocessStateReasonList(configData, raw, logger) { - logger.Printf("state-reason list form detected for %s; moved to state-reasons", params.ConfigKey) + logger.Printf("state-reason list form detected for %s; moved to allowed-state-reason", params.ConfigKey) } } } @@ -171,13 +171,13 @@ func preprocessStateReasonList(configData map[string]any, raw any, logger *logge reasons = append(reasons, s) } if len(reasons) > 0 { - configData["state-reasons"] = reasons + configData["allowed-state-reason"] = reasons } delete(configData, "state-reason") return true case []string: if len(v) > 0 { - configData["state-reasons"] = v + configData["allowed-state-reason"] = v } delete(configData, "state-reason") return true diff --git a/pkg/workflow/safe_outputs_handler_registry.go b/pkg/workflow/safe_outputs_handler_registry.go index 3a4c2b16220..cba92bfb055 100644 --- a/pkg/workflow/safe_outputs_handler_registry.go +++ b/pkg/workflow/safe_outputs_handler_registry.go @@ -109,7 +109,7 @@ var handlerRegistry = map[string]handlerBuilder{ AddIfNotEmpty("target-repo", c.TargetRepoSlug). AddStringSlice("allowed_repos", c.AllowedRepos). AddIfNotEmpty("state_reason", c.StateReason). - AddStringSlice("state_reasons", c.StateReasons). + AddStringSlice("allowed_state_reason", c.AllowedStateReason). AddBoolPtr("allow_body", c.AllowBody). AddBoolPtr("issue_intent", c.IssueIntent). AddTemplatableBool("staged", templatableBoolPtrToStringPtr(c.Staged)). diff --git a/pkg/workflow/safe_outputs_tools_generation.go b/pkg/workflow/safe_outputs_tools_generation.go index 36789cb3710..f2a2ef92fe6 100644 --- a/pkg/workflow/safe_outputs_tools_generation.go +++ b/pkg/workflow/safe_outputs_tools_generation.go @@ -304,7 +304,7 @@ func computePropertyInjections(safeOutputs *SafeOutputsConfig) map[string]map[st return injections } // List or omitted: expose state_reason with the permitted enum. - enumValues := c.StateReasons + enumValues := c.AllowedStateReason if len(enumValues) == 0 { enumValues = closeIssueStateReasonValues } diff --git a/pkg/workflow/safe_outputs_tools_generation_test.go b/pkg/workflow/safe_outputs_tools_generation_test.go index 163799ec6a4..141d33eb5d0 100644 --- a/pkg/workflow/safe_outputs_tools_generation_test.go +++ b/pkg/workflow/safe_outputs_tools_generation_test.go @@ -457,7 +457,7 @@ func TestComputePropertyInjectionsOmittedStateReason(t *testing.T) { func TestComputePropertyInjectionsListStateReason(t *testing.T) { injections := computePropertyInjections(&SafeOutputsConfig{ CloseIssues: &CloseIssuesConfig{ - StateReasons: []string{"not_planned", "duplicate"}, + AllowedStateReason: []string{"not_planned", "duplicate"}, }, }) @@ -491,7 +491,7 @@ func TestComputePropertyInjectionsNilCloseIssues(t *testing.T) { assert.Empty(t, injections) } -// TestPreprocessStateReasonListSlice verifies that a []any slice is converted to state-reasons. +// TestPreprocessStateReasonListSlice verifies that a []any slice is converted to allowed-state-reason. func TestPreprocessStateReasonListSlice(t *testing.T) { configData := map[string]any{ "state-reason": []any{"not_planned", "duplicate"}, @@ -499,10 +499,10 @@ func TestPreprocessStateReasonListSlice(t *testing.T) { result := preprocessStateReasonList(configData, configData["state-reason"], nil) assert.True(t, result) assert.NotContains(t, configData, "state-reason", "state-reason key should be removed") - assert.Equal(t, []string{"not_planned", "duplicate"}, configData["state-reasons"]) + assert.Equal(t, []string{"not_planned", "duplicate"}, configData["allowed-state-reason"]) } -// TestPreprocessStateReasonListStringSlice verifies that a []string slice is converted to state-reasons. +// TestPreprocessStateReasonListStringSlice verifies that a []string slice is converted to allowed-state-reason. func TestPreprocessStateReasonListStringSlice(t *testing.T) { configData := map[string]any{ "state-reason": []string{"completed", "not_planned"}, @@ -510,7 +510,7 @@ func TestPreprocessStateReasonListStringSlice(t *testing.T) { result := preprocessStateReasonList(configData, configData["state-reason"], nil) assert.True(t, result) assert.NotContains(t, configData, "state-reason") - assert.Equal(t, []string{"completed", "not_planned"}, configData["state-reasons"]) + assert.Equal(t, []string{"completed", "not_planned"}, configData["allowed-state-reason"]) } // TestPreprocessStateReasonListScalarNotConverted verifies that a scalar string is not touched. From 47ee766a069e98c7030729b344a4786c89cda707 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Jul 2026 12:08:21 +0000 Subject: [PATCH 5/7] fix: address review feedback on state-reason correctness and schema - Fix empty/all-non-string list in preprocessStateReasonList: return false and preserve configData to prevent silent escalation to unrestricted mode - Validate AllowedStateReason values at compile time in computePropertyInjections; filter invalid values and fall back to full set only when all are invalid - Extend main_workflow_schema.json state-reason to accept scalar OR non-empty array - JS scalar config mode: enforce configStateReason; ignore agent-supplied override - JS list/omitted paths: normalize stateReason to lowercase after validation - JS test: update scalar-override test to assert new enforced behavior - JS test: mock mockGithub.request in list-config duplicate test and assert issue-intent PATCH carries suggest/rationale/confidence metadata - Add Go unit tests for empty list, all-non-string, invalid values in injections Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/close_issue.cjs | 11 +-- actions/setup/js/close_issue.test.cjs | 25 ++++++- pkg/parser/schemas/main_workflow_schema.json | 22 ++++-- pkg/workflow/close_entity_helpers.go | 25 +++++-- pkg/workflow/safe_outputs_tools_generation.go | 23 +++++++ .../safe_outputs_tools_generation_test.go | 67 +++++++++++++++++++ 6 files changed, 156 insertions(+), 17 deletions(-) diff --git a/actions/setup/js/close_issue.cjs b/actions/setup/js/close_issue.cjs index fa3dc2357c0..cdd6f3b4e6f 100644 --- a/actions/setup/js/close_issue.cjs +++ b/actions/setup/js/close_issue.cjs @@ -309,9 +309,10 @@ async function main(config = {}) { if (item.state_reason !== undefined && item.state_reason !== null) { const provided = String(item.state_reason); if (configStateReason) { - // Scalar config: state_reason is fixed; item-level value is accepted as an override - // (it was not exposed in the tool schema, so treat it gracefully). - stateReason = provided; + // Scalar config: state_reason is fixed by config; the agent cannot override it. + // The field was not exposed in the tool schema, so silently enforce the + // configured value regardless of what the agent supplied. + stateReason = configStateReason; } else if (configStateReasons) { // List config: validate against the configured subset. const upperProvided = provided.toUpperCase(); @@ -319,7 +320,7 @@ async function main(config = {}) { if (!upperAllowed.includes(upperProvided)) { throw new Error(`state_reason "${provided}" is not permitted. Allowed values: ${configStateReasons.join(", ")}`); } - stateReason = provided; + stateReason = provided.toLowerCase(); } else { // Omitted config: validate against all three supported values. const upperProvided = provided.toUpperCase(); @@ -327,7 +328,7 @@ async function main(config = {}) { if (!supportedUpper.includes(upperProvided)) { throw new Error(`state_reason "${provided}" is not a supported value. Supported values: completed, not_planned, duplicate`); } - stateReason = provided; + stateReason = provided.toLowerCase(); } } else { stateReason = defaultStateReason; diff --git a/actions/setup/js/close_issue.test.cjs b/actions/setup/js/close_issue.test.cjs index 12d6111519f..40f005f786e 100644 --- a/actions/setup/js/close_issue.test.cjs +++ b/actions/setup/js/close_issue.test.cjs @@ -789,7 +789,7 @@ describe("close_issue", () => { expect(updateCalls[0].state_reason).toBe("duplicate"); }); - it("should prefer item-level state_reason over config-level default", async () => { + it("should enforce scalar state_reason from config regardless of item-level value", async () => { const handler = await main({ max: 10, state_reason: "NOT_PLANNED" }); const updateCalls = []; @@ -804,10 +804,11 @@ describe("close_issue", () => { }; }; + // Agent provides DUPLICATE but scalar config is NOT_PLANNED — config wins. const result = await handler({ issue_number: 100, body: "Duplicate of #50", state_reason: "DUPLICATE" }, {}); expect(result.success).toBe(true); - expect(updateCalls[0].state_reason).toBe("duplicate"); + expect(updateCalls[0].state_reason).toBe("not_planned"); }); it("should use first item in allowed_state_reason list as default when agent omits state_reason", async () => { @@ -1429,6 +1430,7 @@ describe("close_issue", () => { it("should mark native duplicate when using allowed_state_reason list with DUPLICATE and duplicate_of", async () => { const handler = await main({ max: 10, allowed_state_reason: ["not_planned", "duplicate"] }); const graphqlCalls = []; + const requestCalls = []; mockGithub.rest.issues.get = async ({ owner, repo, issue_number }) => ({ data: { @@ -1450,6 +1452,18 @@ describe("close_issue", () => { }, }); + mockGithub.request = async (route, params) => { + requestCalls.push({ route, params }); + return { + data: { + number: params.issue_number, + title: "Test Issue", + html_url: `https://github.com/${params.owner}/${params.repo}/issues/${params.issue_number}`, + node_id: `node_${params.owner}_${params.repo}_${params.issue_number}`, + }, + }; + }; + mockGithub.graphql = async (mutation, variables) => { graphqlCalls.push(variables); return { markAsDuplicate: { duplicate: { id: variables.duplicateId, number: 1003 } } }; @@ -1458,6 +1472,13 @@ describe("close_issue", () => { const result = await handler({ issue_number: 1003, body: "Duplicate report", state_reason: "DUPLICATE", duplicate_of: 50, suggest: true, rationale: "Same CSV defect.", confidence: "HIGH" }, {}); expect(result.success).toBe(true); + + // Verify issue-intent PATCH was called with the intent metadata intact (suggest, rationale, confidence) + expect(requestCalls).toHaveLength(1); + expect(requestCalls[0].route).toBe("PATCH /repos/{owner}/{repo}/issues/{issue_number}"); + expect(requestCalls[0].params.state).toMatchObject({ value: "closed", rationale: "Same CSV defect.", confidence: "HIGH" }); + + // Verify the native duplicate GraphQL mutation was also executed expect(graphqlCalls.length).toBeGreaterThan(0); const mutation = graphqlCalls.find(c => "duplicateId" in c || "canonicalId" in c); expect(mutation).toBeDefined(); diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 989eeb06929..16cf0c3bd37 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -6278,10 +6278,24 @@ ] }, "state-reason": { - "type": "string", - "enum": ["completed", "not_planned", "duplicate"], - "default": "completed", - "description": "Reason for closing the issue (default: completed)" + "oneOf": [ + { + "type": "string", + "enum": ["completed", "not_planned", "duplicate"], + "description": "Fixed reason for closing the issue; the agent cannot override this value." + }, + { + "type": "array", + "items": { + "type": "string", + "enum": ["completed", "not_planned", "duplicate"] + }, + "minItems": 1, + "uniqueItems": true, + "description": "Restricted subset of allowed reasons; the agent may choose from this list at runtime." + } + ], + "description": "Reason for closing the issue. Scalar: fixed reason (default: completed). Array: agent selects from the listed subset at runtime." } }, "additionalProperties": false, diff --git a/pkg/workflow/close_entity_helpers.go b/pkg/workflow/close_entity_helpers.go index d2b3b1b05ab..fb82d4cf829 100644 --- a/pkg/workflow/close_entity_helpers.go +++ b/pkg/workflow/close_entity_helpers.go @@ -154,8 +154,10 @@ func (c *Compiler) parseCloseEntityConfig(outputMap map[string]any, params Close return config } -// preprocessStateReasonList converts a list-form state-reason value into the state-reasons key. -// Returns true if the value was a list and was successfully converted. +// preprocessStateReasonList converts a list-form state-reason value into the allowed-state-reason key. +// Returns true if the value was a non-empty list and was successfully converted. +// Returns false and leaves configData unchanged when the value is not a list or when no valid +// string elements are found — the latter prevents a silent escalation to unrestricted (omitted) mode. func preprocessStateReasonList(configData map[string]any, raw any, logger *logger.Logger) bool { switch v := raw.(type) { case []any: @@ -170,15 +172,26 @@ func preprocessStateReasonList(configData map[string]any, raw any, logger *logge } reasons = append(reasons, s) } - if len(reasons) > 0 { - configData["allowed-state-reason"] = reasons + if len(reasons) == 0 { + // No usable strings found; leave configData unchanged so downstream validation + // reports an error rather than silently granting unrestricted reason selection. + if logger != nil { + logger.Printf("state-reason list has no valid string elements; treating as invalid") + } + return false } + configData["allowed-state-reason"] = reasons delete(configData, "state-reason") return true case []string: - if len(v) > 0 { - configData["allowed-state-reason"] = v + if len(v) == 0 { + // Empty explicit slice; leave configData unchanged for the same reason as above. + if logger != nil { + logger.Printf("state-reason list is empty; treating as invalid") + } + return false } + configData["allowed-state-reason"] = v delete(configData, "state-reason") return true } diff --git a/pkg/workflow/safe_outputs_tools_generation.go b/pkg/workflow/safe_outputs_tools_generation.go index f2a2ef92fe6..0fe155a4267 100644 --- a/pkg/workflow/safe_outputs_tools_generation.go +++ b/pkg/workflow/safe_outputs_tools_generation.go @@ -307,6 +307,29 @@ func computePropertyInjections(safeOutputs *SafeOutputsConfig) map[string]map[st enumValues := c.AllowedStateReason if len(enumValues) == 0 { enumValues = closeIssueStateReasonValues + } else { + // Validate each configured value against the supported API values so that + // invalid strings (e.g. "done", "wontfix") are caught at compile time rather + // than producing a GitHub API 422 at runtime. + supported := make(map[string]bool, len(closeIssueStateReasonValues)) + for _, v := range closeIssueStateReasonValues { + supported[v] = true + } + valid := make([]string, 0, len(enumValues)) + for _, v := range enumValues { + if supported[v] { + valid = append(valid, v) + } + // Invalid values are silently dropped here; the upstream schema + // validation (main_workflow_schema.json) will surface an error to the + // workflow author before compilation reaches this point. + } + if len(valid) == 0 { + // All values were invalid; fall back to the full set so that compilation + // succeeds, relying on schema validation to have already warned the author. + valid = closeIssueStateReasonValues + } + enumValues = valid } injections["close_issue"] = map[string]any{ "state_reason": map[string]any{ diff --git a/pkg/workflow/safe_outputs_tools_generation_test.go b/pkg/workflow/safe_outputs_tools_generation_test.go index 141d33eb5d0..88f02868cb6 100644 --- a/pkg/workflow/safe_outputs_tools_generation_test.go +++ b/pkg/workflow/safe_outputs_tools_generation_test.go @@ -523,3 +523,70 @@ func TestPreprocessStateReasonListScalarNotConverted(t *testing.T) { assert.Equal(t, "not_planned", configData["state-reason"], "scalar should remain unchanged") assert.NotContains(t, configData, "state-reasons") } + +// TestPreprocessStateReasonListEmptyAnySlice verifies that an empty []any leaves configData unchanged. +// An empty list must not silently escalate to unrestricted (omitted) mode. +func TestPreprocessStateReasonListEmptyAnySlice(t *testing.T) { + configData := map[string]any{ + "state-reason": []any{}, + } + result := preprocessStateReasonList(configData, configData["state-reason"], nil) + assert.False(t, result, "empty []any slice should return false") + assert.Contains(t, configData, "state-reason", "state-reason key must be preserved") + assert.NotContains(t, configData, "allowed-state-reason") +} + +// TestPreprocessStateReasonListEmptyStringSlice verifies that an empty []string leaves configData unchanged. +func TestPreprocessStateReasonListEmptyStringSlice(t *testing.T) { + configData := map[string]any{ + "state-reason": []string{}, + } + result := preprocessStateReasonList(configData, configData["state-reason"], nil) + assert.False(t, result, "empty []string slice should return false") + assert.Contains(t, configData, "state-reason", "state-reason key must be preserved") + assert.NotContains(t, configData, "allowed-state-reason") +} + +// TestPreprocessStateReasonListAllNonStringElements verifies that a []any with no string elements +// leaves configData unchanged, preventing a silent escalation to unrestricted mode. +func TestPreprocessStateReasonListAllNonStringElements(t *testing.T) { + configData := map[string]any{ + "state-reason": []any{42, true, nil}, + } + result := preprocessStateReasonList(configData, configData["state-reason"], nil) + assert.False(t, result, "all-non-string []any should return false") + assert.Contains(t, configData, "state-reason", "state-reason key must be preserved") + assert.NotContains(t, configData, "allowed-state-reason") +} + +// TestComputePropertyInjectionsFiltersInvalidStateReasons verifies that invalid values in +// AllowedStateReason are silently dropped so only supported API values reach the tool schema. +func TestComputePropertyInjectionsFiltersInvalidStateReasons(t *testing.T) { + injections := computePropertyInjections(&SafeOutputsConfig{ + CloseIssues: &CloseIssuesConfig{ + AllowedStateReason: []string{"not_planned", "wontfix", "done"}, + }, + }) + + require.Contains(t, injections, "close_issue") + prop, ok := injections["close_issue"]["state_reason"].(map[string]any) + require.True(t, ok) + // "wontfix" and "done" are invalid; only "not_planned" survives. + assert.Equal(t, []string{"not_planned"}, prop["enum"]) +} + +// TestComputePropertyInjectionsAllInvalidFallsBackToFullSet verifies that when ALL configured +// values are invalid, computePropertyInjections falls back to the full supported set so the +// tool schema remains functional. +func TestComputePropertyInjectionsAllInvalidFallsBackToFullSet(t *testing.T) { + injections := computePropertyInjections(&SafeOutputsConfig{ + CloseIssues: &CloseIssuesConfig{ + AllowedStateReason: []string{"done", "wontfix"}, + }, + }) + + require.Contains(t, injections, "close_issue") + prop, ok := injections["close_issue"]["state_reason"].(map[string]any) + require.True(t, ok) + assert.Equal(t, closeIssueStateReasonValues, prop["enum"]) +} From fe9c2e2c5eb237f1335a9d7dbecfabc269938bce Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Jul 2026 12:16:32 +0000 Subject: [PATCH 6/7] fix: add debug log for scalar enforcement, warn on invalid AllowedStateReason, add core.debug mock - JS close_issue.cjs: add core.debug log when agent value is overridden by scalar config - Go computePropertyInjections: log warning when invalid AllowedStateReason values are filtered - JS test: add debug: () => {} to mockCore to prevent TypeError on core.debug calls Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/close_issue.cjs | 7 +++++-- actions/setup/js/close_issue.test.cjs | 1 + pkg/workflow/safe_outputs_tools_generation.go | 6 +++--- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/actions/setup/js/close_issue.cjs b/actions/setup/js/close_issue.cjs index cdd6f3b4e6f..7189dcacca8 100644 --- a/actions/setup/js/close_issue.cjs +++ b/actions/setup/js/close_issue.cjs @@ -310,8 +310,11 @@ async function main(config = {}) { const provided = String(item.state_reason); if (configStateReason) { // Scalar config: state_reason is fixed by config; the agent cannot override it. - // The field was not exposed in the tool schema, so silently enforce the - // configured value regardless of what the agent supplied. + // The field was not exposed in the tool schema, so enforce the configured value + // regardless of what the agent supplied. + if (provided.toLowerCase() !== configStateReason.toLowerCase()) { + core.debug(`state_reason "${provided}" supplied by agent overridden by scalar config value "${configStateReason}"`); + } stateReason = configStateReason; } else if (configStateReasons) { // List config: validate against the configured subset. diff --git a/actions/setup/js/close_issue.test.cjs b/actions/setup/js/close_issue.test.cjs index 40f005f786e..dcc329b93a6 100644 --- a/actions/setup/js/close_issue.test.cjs +++ b/actions/setup/js/close_issue.test.cjs @@ -13,6 +13,7 @@ describe("close_issue", () => { info: () => {}, warning: () => {}, error: () => {}, + debug: () => {}, messages: [], infos: [], warnings: [], diff --git a/pkg/workflow/safe_outputs_tools_generation.go b/pkg/workflow/safe_outputs_tools_generation.go index 0fe155a4267..99927df9573 100644 --- a/pkg/workflow/safe_outputs_tools_generation.go +++ b/pkg/workflow/safe_outputs_tools_generation.go @@ -319,14 +319,14 @@ func computePropertyInjections(safeOutputs *SafeOutputsConfig) map[string]map[st for _, v := range enumValues { if supported[v] { valid = append(valid, v) + } else { + safeOutputsConfigLog.Printf("Warning: allowed-state-reason value %q is not a supported GitHub API value; valid values: %v", v, closeIssueStateReasonValues) } - // Invalid values are silently dropped here; the upstream schema - // validation (main_workflow_schema.json) will surface an error to the - // workflow author before compilation reaches this point. } if len(valid) == 0 { // All values were invalid; fall back to the full set so that compilation // succeeds, relying on schema validation to have already warned the author. + safeOutputsConfigLog.Printf("Warning: all allowed-state-reason values were invalid; falling back to full supported set") valid = closeIssueStateReasonValues } enumValues = valid From a30e9201d66496f55854e0f884d8016ed4934139 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Jul 2026 12:18:14 +0000 Subject: [PATCH 7/7] fix: improve debug log clarity and add suggest assertion to intent test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/close_issue.cjs | 6 +++--- actions/setup/js/close_issue.test.cjs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/actions/setup/js/close_issue.cjs b/actions/setup/js/close_issue.cjs index 7189dcacca8..2437f43024d 100644 --- a/actions/setup/js/close_issue.cjs +++ b/actions/setup/js/close_issue.cjs @@ -312,10 +312,10 @@ async function main(config = {}) { // Scalar config: state_reason is fixed by config; the agent cannot override it. // The field was not exposed in the tool schema, so enforce the configured value // regardless of what the agent supplied. - if (provided.toLowerCase() !== configStateReason.toLowerCase()) { - core.debug(`state_reason "${provided}" supplied by agent overridden by scalar config value "${configStateReason}"`); - } stateReason = configStateReason; + if (provided.toLowerCase() !== stateReason.toLowerCase()) { + core.debug(`state_reason agent value "${provided.toLowerCase()}" overridden by scalar config value "${stateReason.toLowerCase()}"`); + } } else if (configStateReasons) { // List config: validate against the configured subset. const upperProvided = provided.toUpperCase(); diff --git a/actions/setup/js/close_issue.test.cjs b/actions/setup/js/close_issue.test.cjs index dcc329b93a6..f58a173a2d2 100644 --- a/actions/setup/js/close_issue.test.cjs +++ b/actions/setup/js/close_issue.test.cjs @@ -1477,7 +1477,7 @@ describe("close_issue", () => { // Verify issue-intent PATCH was called with the intent metadata intact (suggest, rationale, confidence) expect(requestCalls).toHaveLength(1); expect(requestCalls[0].route).toBe("PATCH /repos/{owner}/{repo}/issues/{issue_number}"); - expect(requestCalls[0].params.state).toMatchObject({ value: "closed", rationale: "Same CSV defect.", confidence: "HIGH" }); + expect(requestCalls[0].params.state).toMatchObject({ value: "closed", suggest: true, rationale: "Same CSV defect.", confidence: "HIGH" }); // Verify the native duplicate GraphQL mutation was also executed expect(graphqlCalls.length).toBeGreaterThan(0);