fix: mode-specific intent guidance for all intent-aware safe-output mutations - #46876
Conversation
- Add ISSUE_INTENT_REQUIRED_SUFFIX constant for issue_intent: true mode - Add ISSUE_INTENT_OPTIONAL_SUFFIX constant for omitted issue_intent mode - Add ADD_LABELS_STRICT_FIELD_DESC / ADD_LABELS_OPTIONAL_FIELD_DESC constants - Add isIssueIntentOmittedForTool() helper - Add makeIntentFieldDescriptionsRequired() helper to update rationale/confidence property descriptions from "Optional" to "Required" in strict mode - In strict mode: use required suffix, update rationale/confidence descriptions, update add_labels labels description and nested items descriptions - In omitted mode: use encouraged suffix, update add_labels labels description to prefer structured objects while keeping plain strings valid - Update existing tests to use new suffix strings - Add new tests covering omitted, required, and disabled modes for all 6 tools Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…spread, clarify test name and regex comment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Aligns intent-aware safe-output descriptions with strict, optional, and disabled runtime schemas.
Changes:
- Adds mode-specific intent guidance and label examples.
- Updates required field descriptions.
- Expands regression coverage across six mutation tools.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/generate_safe_outputs_tools.cjs |
Generates mode-specific intent guidance. |
actions/setup/js/generate_safe_outputs_tools.test.cjs |
Tests intent behavior across modes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/generate_safe_outputs_tools.cjs:47
- The example shows a bare object even though
labelshastype: "array". An agent copying this example would produce a schema-invalid payload; wrap the structured label in an array.
'Labels to add. Prefer structured label objects: {"name": "bug", "rationale": "The report describes reproducible incorrect behavior.", "confidence": "HIGH"}. Plain strings are also accepted for compatibility. Include rationale (string, max 280 chars) and confidence (LOW, MEDIUM, or HIGH) to improve transparency. Labels must exist in the repository.';
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Medium
| const ISSUE_INTENT_SUFFIX = "INTENT: Include rationale (string, max 280 chars) and confidence (string, exactly one of: LOW, MEDIUM, HIGH) with each call."; | ||
| const ISSUE_INTENT_REQUIRED_SUFFIX = "INTENT REQUIRED: rationale (string, max 280 chars) and confidence (exactly one of: LOW, MEDIUM, HIGH) are required for each call."; | ||
| const ISSUE_INTENT_OPTIONAL_SUFFIX = | ||
| "INTENT ENCOURAGED: Include rationale (string, max 280 chars) and confidence (exactly one of: LOW, MEDIUM, HIGH) with each call. These fields are optional but strongly encouraged — they improve transparency and should normally be included. Use suggest: true to route for human review."; |
| } | ||
| if (isIssueIntentEnabledForTool(tool.name, config[tool.name])) { | ||
| enhancedTool.description = `${enhancedTool.description || ""} ${ISSUE_INTENT_SUFFIX}`.trim(); | ||
| enhancedTool.description = `${enhancedTool.description || ""} ${ISSUE_INTENT_REQUIRED_SUFFIX}`.trim(); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: LGTM
The three-mode intent split (REQUIRED / ENCOURAGED / disabled) is correctly implemented and fully tested.
isIssueIntentOmittedForToolcleanly detects the default case without branch overlap with enabled/disabled paths.makeIntentFieldDescriptionsRequiredpatches top-levelrationale/confidencedescriptions; nested item descriptions foradd_labelsare separately handled with dedicated constants.- Old combined omitted+disabled test properly split into focused negative assertion (disabled) and new positive coverage test (omitted, all 6 tools).
- Strict
add_labelsnow consistently forbids plain strings in both description text and schema.
No correctness, security, or reliability issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 22.9 AIC · ⌖ 7.68 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (10 tests)
Verdict
References:
|
There was a problem hiding this comment.
Verdict: two issues require fixes before merge
Blocking: makeIntentFieldDescriptionsRequired marks fields as required in text but never updates inputSchema.required, creating a schema/description mismatch that validators and strict consumers will not catch (see line 56 comment). Blocking: the omitted-mode add_labels description says "Plain strings are also accepted" unconditionally, even when the source schema has no oneOf allowing strings — agents that trust the description will emit payloads the validator rejects (see line 113 comment).
Non-blocking
- Line 102:
delete labelsSchema.items.oneOfis dead code after the full object replacement — remove to avoid misleading future readers.
🔎 Code quality review by PR Code Quality Reviewer · 57.1 AIC · ⌖ 4.67 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/generate_safe_outputs_tools.cjs:56
makeIntentFieldDescriptionsRequired never updates inputSchema.required: The function rewrites description text to say "Required" but never adds rationale/confidence to inputSchema.required, so JSON Schema validators will not enforce them.
<details>
<summary>💡 Suggested fix</summary>
Add the fields to required inside the function:
const required = new Set(schema.required ?? []);
required.add('rationale');
required.add('confidence');
schema.required = [...required];…
actions/setup/js/generate_safe_outputs_tools.cjs:113
ADD_LABELS_OPTIONAL_FIELD_DESC applied unconditionally even when items.oneOf is absent: The omitted-mode branch replaces the labels description with text saying "Plain strings are also accepted", but never checks whether the source schema actually supports plain strings via items.oneOf.
<details>
<summary>💡 Detail</summary>
If the source tool's labels.items is already a plain {type: 'object'} (no oneOf), the description will incorrectly advertise plain string support that t…
actions/setup/js/generate_safe_outputs_tools.cjs:102
delete labelsSchema.items.oneOf is a no-op: After labelsSchema.items = strictItems (line 101), strictItems was spread from objectSchema which never had oneOf — that property lived on the old items wrapper. The delete deletes a non-existent property on the new object and does nothing.
<details>
<summary>💡 Detail</summary>
The old code needed the delete because it mutated the existing items object. Here, a new object is assigned, so oneOf is never present on it. The `delete…
There was a problem hiding this comment.
Skills-Based Review
Applied /tdd and /codebase-design — two targeted observations, no blocking issues.
Key themes:
- Test coverage gap (omitted mode): omitted-mode test does not assert rationale/confidence descriptions remain Optional.
- Control flow: isIssueIntentDisabled is a separate if rather than fused into the else if chain.
Highlights:
- Clean split of ISSUE_INTENT_SUFFIX into two well-named constants
- makeIntentFieldDescriptionsRequired is a focused helper
- Four new regression tests cover all three modes thoroughly
- PR description table clearly articulates before/after per mode
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 37.4 AIC · ⌖ 4.63 AIC · ⊞ 6.7K
Comment /matt to run again
| name: "add_labels", | ||
| description: "Adds labels.", | ||
| inputSchema: { | ||
| type: "object", |
There was a problem hiding this comment.
[/tdd] The omitted-mode test verifies that rationale/confidence are not in inputSchema.required, but doesn't assert that their descriptions still say "Optional" — so a future accidental call to makeIntentFieldDescriptionsRequired in the omitted path would go undetected.
💡 Suggested assertion
Add these assertions inside the loop over toolNames.filter(n => n !== 'add_labels'):
const props = tool.inputSchema.properties;
if (props?.rationale?.description) {
expect(props.rationale.description, `${name} rationale description should remain optional`).toMatch(/optional/i);
}
if (props?.confidence?.description) {
expect(props.confidence.description, `${name} confidence description should remain optional`).toMatch(/optional/i);
}This guards the invariant that omitted mode does not promote field descriptions to required.
@copilot please address this.
| } | ||
| } | ||
| } | ||
| if (isIssueIntentDisabledForTool(tool.name, config[tool.name])) { |
There was a problem hiding this comment.
[/codebase-design] The isIssueIntentDisabled branch at line 297 runs as an independent if after the if/else if chain for enabled/omitted. This means a tool explicitly configured with issue_intent: false will still enter isIssueIntentOmittedForTool — it won't, because isIssueIntentOmittedForTool returns false when disabled — but the structure is brittle: it's easy for a future reader to add another else if that inadvertently runs for disabled tools, or to add logic before the stripIssueIntentSchemaFields call that applies to all non-enabled tools.
💡 Suggestion
Merge the disabled branch into the else chain to make the three modes mutually exclusive at the structural level:
if (isIssueIntentEnabledForTool(...)) {
// strict
} else if (isIssueIntentDisabledForTool(...)) {
stripIssueIntentSchemaFields(enhancedTool);
} else if (isIssueIntentOmittedForTool(...)) {
// encouraged
}This makes it immediately obvious the modes are exhaustive and mutually exclusive.
@copilot please address this.
|
@copilot run pr-finisher skill |
|
@copilot please run the Unresolved review threads:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
… examples - Update ISSUE_INTENT_OPTIONAL_SUFFIX to say 'alongside rationale and confidence' so agents know suggest goes with those fields (inside each label object for add_labels, not at the top level) - Update ADD_LABELS_OPTIONAL_FIELD_DESC to include suggest: true in the example object, making its per-label placement unambiguous - In strict mode for assign_to_agent, replace inline example calls with versions that include rationale and confidence so examples match the generated schema - Add regression test for strict-mode assign_to_agent example update - Add assertion that omitted-mode add_labels description shows suggest inside the label object example Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…les and pre-condition - Capture source description in variable and assert it lacks rationale/confidence before transformation, proving the replacement is non-trivial - Replace simple regex matches with matchAll iteration: verifies that EVERY assign_to_agent() call in the output includes both rationale= and confidence=, rather than just checking anywhere in the description Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Two commits address the review feedback:
Strict-mode
Regression tests verify: pre-condition (source has no rationale/confidence in examples), post-condition (every example call includes both fields), and the INTENT REQUIRED suffix is still present. |
…hain, guards, test assertions Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all 4 unresolved review threads in c61d246:
PR Code Quality Reviewer BLOCKING (makeIntentFieldDescriptionsRequired never updates required): PR Code Quality Reviewer BLOCKING (ADD_LABELS_OPTIONAL_FIELD_DESC unconditional): Omitted-mode Non-blocking cleanup: Removed the dead |
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
Generated tool descriptions for intent-aware mutations (
add_labels,set_issue_type,set_issue_field,close_issue,assign_to_user,assign_to_agent) were inconsistent with their runtime schemas. Most critically,add_labelsin strict mode still described plain strings as valid andrationale/confidenceas optional — causing agents to emit payloads the safe-output processor would reject entirely.Behavior by mode
issue_intent: truerationale/confidencedescriptions updated to say "Required";add_labelslabels description is object-only with exampleissue_intentomittedadd_labelsprefers structured objects while keeping plain strings validissue_intent: falseChanges
generate_safe_outputs_tools.cjsISSUE_INTENT_SUFFIXwith two mode-specific constants:ISSUE_INTENT_REQUIRED_SUFFIXandISSUE_INTENT_OPTIONAL_SUFFIXADD_LABELS_STRICT_FIELD_DESC/ADD_LABELS_OPTIONAL_FIELD_DESCconstants for thelabelsproperty description in each modeisIssueIntentOmittedForTool()helper to detect the default/omitted casemakeIntentFieldDescriptionsRequired()to rewrite top-levelrationale/confidenceproperty descriptions from "Optional…" to "Required…" in strict modemakeIntentFieldDescriptionsRequired(), replacesadd_labelslabels description and updates nestedrationale/confidenceitem descriptionsadd_labelslabels description to prefer structured objectsgenerate_safe_outputs_tools.test.cjsINTENT REQUIRED:suffix textadd_labelsassertions verify the description contains no "plain strings accepted" language and no "optional" for required fieldsExample: strict mode
add_labelsschema + description (was conflicting, now consistent){ "labels": { "description": "Labels to add. Each label must be an object with required fields: name (string), rationale (string, max 280 chars), and confidence (exactly one of: LOW, MEDIUM, HIGH). Plain string label names are not permitted. Example: [{\"name\": \"bug\", \"rationale\": \"The report describes reproducible incorrect behavior.\", \"confidence\": \"HIGH\"}].", "type": "array", "items": { "type": "object", "required": ["name", "rationale", "confidence"], "properties": { ... } } } }pr-sous-chef run https://github.com/github/gh-aw/actions/runs/29761732160
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.