Skip to content

Add issue-intent prompt suffix to issue mutation safe-output tools #42776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Jul 1, 2026
Merged

Add issue-intent prompt suffix to issue mutation safe-output tools #42776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98e1054
Initial plan
Copilot Jul 1, 2026
7dc20fe
Add issue_intents description suffix for issue tools
Copilot Jul 1, 2026
5fd6983
Refactor runtime feature parsing for tool descriptions
Copilot Jul 1, 2026
12050f6
Use shared runtime feature parsing in safe-output generator
Copilot Jul 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions actions/setup/js/generate_safe_outputs_tools.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,14 @@
* Default: ${RUNNER_TEMP}/gh-aw/safeoutputs/tools_meta.json
* GH_AW_SAFE_OUTPUTS_TOOLS_PATH - Output path for the generated tools.json
* Default: ${RUNNER_TEMP}/gh-aw/safeoutputs/tools.json
* GH_AW_RUNTIME_FEATURES - Newline-delimited runtime features in key or key=value format
* Parsed using runtime_features.cjs helpers
*/

const fs = require("fs");
const path = require("path");
const { ERR_CONFIG } = require("./error_codes.cjs");
const { parseRuntimeFeatures, hasRuntimeFeature } = require("./runtime_features.cjs");

const ADD_COMMENT_DEFAULT_DISCUSSIONS_NOTE =
"NOTE: By default, this tool does not require discussions:write permission. Set 'discussions: true' in the workflow's safe-outputs.add-comment configuration to enable discussion comments and request this permission.";
Expand Down Expand Up @@ -117,6 +120,7 @@ async function main() {
// This filters out non-tool config entries like dispatch_workflow, call_workflow,
// mentions, max_bot_mentions, etc.
const enabledToolNames = new Set(Object.keys(config).filter(k => sourceToolNames.has(k)));
const runtimeFeatures = parseRuntimeFeatures(process.env.GH_AW_RUNTIME_FEATURES);

// Filter predefined tools to those enabled in config and apply enhancements
const filteredTools = allTools
Expand All @@ -130,6 +134,9 @@ async function main() {
if (descSuffix) {
enhancedTool.description = (enhancedTool.description || "") + descSuffix;

@github-actions github-actions Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] The intent suffix string is a hardcoded literal with no named constant — if the wording ever changes, it must be updated in both the production code and the test, risking a silent divergence.

💡 Suggested refactor

Extract the suffix to a named constant near the other description constants at the top of the file:

const ISSUE_INTENT_SUFFIX = " INTENT: Include rationale (max 280 chars) and confidence (LOW/MEDIUM/HIGH) with each call.";

Then reference it in both the if block and the test assertion. This matches the pattern already used for ADD_COMMENT_DEFAULT_DISCUSSIONS_NOTE etc.

@copilot please address this.

}

@github-actions github-actions Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent undefined guard: enhancedTool.description += ... produces "undefined INTENT: ..." if a tool has no description property.

💡 Suggested fix

Every other description mutation in this function uses the defensive || "" pattern:

// existing descSuffix pattern (correct)
enhancedTool.description = (enhancedTool.description || "") + descSuffix;

// new code should match
enhancedTool.description = (enhancedTool.description || "") + " INTENT: Include rationale (max 280 chars) and confidence (LOW/MEDIUM/HIGH) with each call.";

While production tools always have descriptions today, the codebase's own established pattern uses || "" precisely to prevent this class of bug. The new code breaks that consistency and will silently corrupt the description if ever a tool without a description is added.

@github-actions github-actions Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The feature-flag list (["set_issue_type", "set_issue_field", "add_labels"]) is duplicated between production and test code. A missing tool name in one list won't be caught by the existing test since the test is structured to check a fixed set rather than asserting against the production list.

💡 Suggested improvement

Export the list as a module-level constant:

const ISSUE_INTENT_TOOLS = new Set(["set_issue_type", "set_issue_field", "add_labels"]);

Import it in the test so that any future addition to the production set automatically propagates to the test assertion loop. This gives you true parity rather than two manually maintained copies.

@copilot please address this.

if (hasRuntimeFeature(runtimeFeatures, "issue_intents") && ["set_issue_type", "set_issue_field", "add_labels"].includes(tool.name)) {
enhancedTool.description = `${enhancedTool.description || ""} INTENT: Include rationale (max 280 chars) and confidence (LOW/MEDIUM/HIGH) with each call.`.trim();
}

if (tool.name === "add_comment") {
enhancedTool.description = updateAddCommentDescription(enhancedTool.description, config.add_comment);
Expand Down
44 changes: 44 additions & 0 deletions actions/setup/js/generate_safe_outputs_tools.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,4 +306,48 @@ describe("generate_safe_outputs_tools", () => {
expect(addCommentTool.description).toContain("Discussion comments are disabled for this workflow");
expect(addCommentTool.description).not.toContain("Supports reply_to_id for discussion threading.");
});

it("adds issue intent suffix for issue tools when issue_intents runtime feature is enabled", () => {
fs.writeFileSync(
toolsSourcePath,
JSON.stringify([
{ name: "set_issue_type", description: "Sets issue type.", inputSchema: { type: "object", properties: {} } },
{ name: "set_issue_field", description: "Sets issue field.", inputSchema: { type: "object", properties: {} } },
{ name: "add_labels", description: "Adds labels.", inputSchema: { type: "object", properties: {} } },
{ name: "create_issue", description: "Creates a GitHub issue.", inputSchema: { type: "object", properties: {} } },
])
);
fs.writeFileSync(configPath, JSON.stringify({ set_issue_type: {}, set_issue_field: {}, add_labels: {}, create_issue: {} }));
fs.writeFileSync(toolsMetaPath, JSON.stringify({ description_suffixes: {}, repo_params: {}, dynamic_tools: [] }));

runScript({ GH_AW_RUNTIME_FEATURES: "other\nissue_intents\nanother=true" });

const result = JSON.parse(fs.readFileSync(outputPath, "utf8"));
const intentSuffix = "INTENT: Include rationale (max 280 chars) and confidence (LOW/MEDIUM/HIGH) with each call.";

@github-actions github-actions Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] There is no test verifying that the suffix is not appended when issue_intents is absent from GH_AW_RUNTIME_FEATURES (empty string, or other unrelated features). The positive case is tested but the negative guard (feature absent → no suffix) is missing.

💡 Suggested test case 
it("does not add issue intent suffix when issue_intents feature is not enabled", () => {
  // same setup...
  runScript({ GH_AW_RUNTIME_FEATURES: "other_feature" }); // no issue_intents
  const result = JSON.parse(fs.readFileSync(outputPath, "utf8"));
  for (const name of ["set_issue_type", "set_issue_field", "add_labels"]) {
    expect(result.find(t => t.name === name).description).not.toContain(intentSuffix);
  }
});

Without this, a future change that removes the feature guard would pass the existing tests.

@copilot please address this.

expect(result.find((/** @type {{name: string, description: string}} */ t) => t.name === "set_issue_type").description).toContain(intentSuffix);
expect(result.find((/** @type {{name: string, description: string}} */ t) => t.name === "set_issue_field").description).toContain(intentSuffix);

@github-actions github-actions Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing feature-disabled test: the test only asserts the suffix is absent for create_issue while the feature is enabled. A regression that unconditionally appended the suffix would pass all current assertions.

💡 Suggested addition

Add a sibling test (or an extra assertion in the existing one) that verifies the suffix is NOT present when GH_AW_RUNTIME_FEATURES is absent or does not include issue_intents:

it("does NOT add issue intent suffix when issue_intents is not in runtime features", () => {
  fs.writeFileSync(toolsSourcePath, JSON.stringify([
    { name: "set_issue_type", description: "Sets issue type.", inputSchema: { type: "object", properties: {} } },
  ]));
  fs.writeFileSync(configPath, JSON.stringify({ set_issue_type: {} }));
  fs.writeFileSync(toolsMetaPath, JSON.stringify({ description_suffixes: {}, repo_params: {}, dynamic_tools: [] }));

  runScript({ GH_AW_RUNTIME_FEATURES: "other,unrelated" }); // feature absent

  const result = JSON.parse(fs.readFileSync(outputPath, "utf8"));
  const intentSuffix = "INTENT: Include rationale";
  expect(result.find(t => t.name === "set_issue_type").description).not.toContain(intentSuffix);
});

expect(result.find((/** @type {{name: string, description: string}} */ t) => t.name === "add_labels").description).toContain(intentSuffix);
expect(result.find((/** @type {{name: string, description: string}} */ t) => t.name === "create_issue").description).not.toContain(intentSuffix);

@github-actions github-actions Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking – test gap: The test confirms the suffix is applied to the three issue tools when issue_intents is present and excluded from create_issue, but there is no explicit assertion that the three tools do not get the suffix when the feature flag is absent. Consider adding a negative case (run without GH_AW_RUNTIME_FEATURES=issue_intents) to guard against accidental unconditional application in future refactors.

@copilot please address this.

});

it("does not add issue intent suffix when issue_intents runtime feature is not enabled", () => {
fs.writeFileSync(
toolsSourcePath,
JSON.stringify([
{ name: "set_issue_type", description: "Sets issue type.", inputSchema: { type: "object", properties: {} } },
{ name: "set_issue_field", description: "Sets issue field.", inputSchema: { type: "object", properties: {} } },
{ name: "add_labels", description: "Adds labels.", inputSchema: { type: "object", properties: {} } },
])
);
fs.writeFileSync(configPath, JSON.stringify({ set_issue_type: {}, set_issue_field: {}, add_labels: {} }));
fs.writeFileSync(toolsMetaPath, JSON.stringify({ description_suffixes: {}, repo_params: {}, dynamic_tools: [] }));

runScript({ GH_AW_RUNTIME_FEATURES: "other\nanother=true" });

const result = JSON.parse(fs.readFileSync(outputPath, "utf8"));
const intentSuffix = "INTENT: Include rationale (max 280 chars) and confidence (LOW/MEDIUM/HIGH) with each call.";
expect(result.find((/** @type {{name: string, description: string}} */ t) => t.name === "set_issue_type").description).not.toContain(intentSuffix);
expect(result.find((/** @type {{name: string, description: string}} */ t) => t.name === "set_issue_field").description).not.toContain(intentSuffix);
expect(result.find((/** @type {{name: string, description: string}} */ t) => t.name === "add_labels").description).not.toContain(intentSuffix);
});
});
Loading