Add issue-intent prompt suffix to issue mutation safe-output tools#42776
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42776 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
This PR updates the safe-outputs tool generator to (when the issue_intents runtime feature is enabled) append an “INTENT” suffix to the descriptions of issue-mutation tools so agents are prompted to provide rationale and confidence, and adds a targeted regression test to ensure the suffix is applied only to the intended tools.
Changes:
- Add
GH_AW_RUNTIME_FEATURES-gated description suffixing forset_issue_type,set_issue_field, andadd_labels. - Add a focused test asserting the suffix is applied only to those three tools (and not to unrelated tools).
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/generate_safe_outputs_tools.cjs | Adds runtime-feature-gated “issue intent” description suffixing for specific issue tools. |
| actions/setup/js/generate_safe_outputs_tools.test.cjs | Adds a regression test covering the new runtime-gated suffix behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
| // 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 = (process.env.GH_AW_RUNTIME_FEATURES || "").split(","); |
| 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,issue_intents,another" }); |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (1 test)
|
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 43.3 AIC · ⌖ 7.49 AIC · ⊞ 1.6K
Comment /review to run again
| @@ -130,6 +131,9 @@ async function main() { | |||
| if (descSuffix) { | |||
| enhancedTool.description = (enhancedTool.description || "") + descSuffix; | |||
| } | |||
There was a problem hiding this comment.
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.
| if (descSuffix) { | ||
| enhancedTool.description = (enhancedTool.description || "") + descSuffix; | ||
| } | ||
| if (runtimeFeatures.includes("issue_intents") && ["set_issue_type", "set_issue_field", "add_labels"].includes(tool.name)) { |
There was a problem hiding this comment.
add_labels is not issue-scoped: the intent suffix says "Include rationale ... with each call" for issue mutations, but add_labels is also used to label pull requests. Appending issue-intent instructions to it will inject misleading guidance in PR contexts.
💡 Options
Either:
- Remove
add_labelsfrom the list and rely on the schema's optionalrationale/confidencefields if labelling with intent matters. - Rename the feature-flag to something broader (e.g.
mutation_intents) so the instruction is accurate for all mutation tools. - Change the suffix text to be neutral (e.g. "INTENT: When mutating an issue, include rationale (max 280 chars) and confidence (LOW/MEDIUM/HIGH).") so it's scoped at the call-site, not tool-level.
As written, a model asked to label a PR will receive instructions that say it's performing an issue mutation, which could cause confusion or hallucination.
| // 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 = (process.env.GH_AW_RUNTIME_FEATURES || "").split(","); |
There was a problem hiding this comment.
Silent feature-flag mismatch: the runtime features string is split without trimming or filtering, so GH_AW_RUNTIME_FEATURES="other, issue_intents" (space after comma) will never match and silently skip the feature.
💡 Suggested fix
// current — fragile
const runtimeFeatures = (process.env.GH_AW_RUNTIME_FEATURES || "").split(",");
// better — trim tokens and drop the empty-string element produced when the var is unset
const runtimeFeatures = (process.env.GH_AW_RUNTIME_FEATURES || "")
.split(",")
.map(f => f.trim())
.filter(Boolean);Two distinct problems in one line:
- No
.trim(): a value like"other, issue_intents"(space after comma) will never match"issue_intents". - No
.filter(Boolean): when the env var is absent,"".split(",")returns[""](not[]), soruntimeFeatures.includes("")is silently true — harmless today, but a latent bug for any future empty-string check.
| 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).toContain(intentSuffix); | ||
| expect(result.find((/** @type {{name: string, description: string}} */ t) => t.name === "set_issue_field").description).toContain(intentSuffix); |
There was a problem hiding this comment.
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);
});There was a problem hiding this comment.
Review: Add issue-intent prompt suffix to issue mutation safe-output tools
The change is small, focused, and well-reasoned. Feature-flag gating with GH_AW_RUNTIME_FEATURES is the right pattern, and the split-comma parsing is safe (empty string never matches issue_intents).
Two non-blocking suggestions in inline comments:
- Null-safety consistency (generator, line 135): Use
(enhancedTool.description || "") +instead of+=to match the defensive pattern used by thedescSuffixblock just above. - Explicit negative test (test file, line 330): Add a parallel test case that confirms the suffix is not applied when
issue_intentsis absent from the feature flag, guarding against accidental future regression.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 34.6 AIC · ⌖ 6.56 AIC · ⊞ 4.9K
| enhancedTool.description = (enhancedTool.description || "") + descSuffix; | ||
| } | ||
| if (runtimeFeatures.includes("issue_intents") && ["set_issue_type", "set_issue_field", "add_labels"].includes(tool.name)) { | ||
| enhancedTool.description += " INTENT: Include rationale (max 280 chars) and confidence (LOW/MEDIUM/HIGH) with each call."; |
There was a problem hiding this comment.
Non-blocking – consistency: The descSuffix block above uses (enhancedTool.description || "") + suffix to guard against a missing description, but this line uses += directly. While all source tools carry a description today, using the same defensive pattern keeps the code consistent and prevents a future "undefinedINTENT: ..." surprise:
enhancedTool.description = (enhancedTool.description || "") + " INTENT: Include rationale (max 280 chars) and confidence (LOW/MEDIUM/HIGH) with each call.";@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); | ||
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /grill-with-docs — commenting with a few targeted suggestions; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Missing negative test: The new test only covers the "feature enabled" path; the "feature absent → no suffix" guard is untested.
- Duplicated string literals: The intent suffix text and the tool-name list both appear in both production and test code; a named export would single-source them.
- Documentation gap:
GH_AW_RUNTIME_FEATURESis consumed but absent from the header JSDoc env var table.
Positive Highlights
- ✅ Clean, minimal implementation — 4 lines of production code to close the schema/description gap.
- ✅ Feature flag is additive and non-breaking; existing behavior is preserved when
issue_intentsis absent. - ✅ Test covers both inclusion (3 tools) and exclusion (
create_issue) with a realistic multi-value env var, which is exactly the right boundary to test.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 43.9 AIC · ⌖ 7.68 AIC · ⊞ 6.6K
Comment /matt to run again
| // 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 = (process.env.GH_AW_RUNTIME_FEATURES || "").split(","); |
There was a problem hiding this comment.
[/grill-with-docs] GH_AW_RUNTIME_FEATURES is consumed here but not listed in the file-header JSDoc env var table — future maintainers will miss it when scanning the env var docs.
💡 Suggested addition
Add an entry to the header block alongside the other env vars:
* GH_AW_RUNTIME_FEATURES - Comma-separated list of runtime feature flags
* Supported values: issue_intents
* Default: ""
All other consumed env vars are documented there; keeping this consistent makes the file self-documenting.
@copilot please address this.
| @@ -130,6 +131,9 @@ async function main() { | |||
| if (descSuffix) { | |||
| enhancedTool.description = (enhancedTool.description || "") + descSuffix; | |||
There was a problem hiding this comment.
[/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.
| @@ -130,6 +131,9 @@ async function main() { | |||
| if (descSuffix) { | |||
| enhancedTool.description = (enhancedTool.description || "") + descSuffix; | |||
| } | |||
There was a problem hiding this comment.
[/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.
| runScript({ GH_AW_RUNTIME_FEATURES: "other,issue_intents,another" }); | ||
|
|
||
| const result = JSON.parse(fs.readFileSync(outputPath, "utf8")); | ||
| const intentSuffix = "INTENT: Include rationale (max 280 chars) and confidence (LOW/MEDIUM/HIGH) with each call."; |
There was a problem hiding this comment.
[/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.
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in
Checks run: targeted vitest tests, |
|
🎉 This pull request is included in a new release. Release: |
When
issue_intentsis enabled, the tool schemas already exposerationaleandconfidence, but tool descriptions did not instruct the agent to provide them. This change adds a runtime-gated intent instruction so issue mutation calls are nudged to include intent metadata consistently.Problem addressed
set_issue_type,set_issue_field, andadd_labelscould be called without intent metadata despite schema support forrationaleandconfidence.Generator update (
actions/setup/js/generate_safe_outputs_tools.cjs)GH_AW_RUNTIME_FEATURESand, whenissue_intentsis present, appends an intent suffix to descriptions for:set_issue_typeset_issue_fieldadd_labelsTargeted test coverage (
actions/setup/js/generate_safe_outputs_tools.test.cjs)issue_intentsis enabled, and not for unrelated tools.