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..2437f43024d 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.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.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. + // 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,39 @@ 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 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. + 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(); + 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.toLowerCase(); + } 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.toLowerCase(); + } + } 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..f58a173a2d2 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: [], @@ -789,7 +790,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,12 +805,118 @@ 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("not_planned"); + }); + + 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 => { + 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 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 => { + 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 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" }, {}); + + 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 +1427,62 @@ 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 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: { + 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.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 } } }; + }; + + 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", suggest: true, 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/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..e15631f6eb0 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: { allowed_state_reason: ["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/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.* 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 99daa1744ed..fb82d4cf829 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 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 @@ -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 "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 allowed-state-reason", 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,50 @@ func (c *Compiler) parseCloseEntityConfig(outputMap map[string]any, params Close return config } +// 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: + 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 { + // 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 { + // 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 + } + 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..cba92bfb055 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("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 508fc6211ac..99927df9573 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,64 @@ 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.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) + } else { + safeOutputsConfigLog.Printf("Warning: allowed-state-reason value %q is not a supported GitHub API value; valid values: %v", v, closeIssueStateReasonValues) + } + } + 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 + } + 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 +400,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..88f02868cb6 100644 --- a/pkg/workflow/safe_outputs_tools_generation_test.go +++ b/pkg/workflow/safe_outputs_tools_generation_test.go @@ -436,3 +436,157 @@ 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{ + AllowedStateReason: []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 allowed-state-reason. +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["allowed-state-reason"]) +} + +// 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"}, + } + 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["allowed-state-reason"]) +} + +// 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") +} + +// 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"]) +}