Handle string-form create_issue.labels across safe-output validation and schema/tool definitions#38738
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
create_issue.labels in safe-output validation to prevent false “no safe outputs”
|
@copilot Add this compatibility description to the specification and the tool description, and update the tool JSON to ensure it complies with the schema. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in cc6361c. I added the labels string-compatibility note to the safe outputs specification and create_issue tool description, and updated tool JSON/schema definitions to accept |
create_issue.labels in safe-output validation to prevent false “no safe outputs”create_issue.labels across safe-output validation and schema/tool definitions
There was a problem hiding this comment.
Pull request overview
This PR fixes intermittent “No Safe Outputs Generated” failures by making create_issue.labels tolerant to agents emitting labels as a comma-separated string, and keeping schema/tool docs aligned with that compatibility behavior.
Changes:
- Allow
create_issue.labelsto be provided as eitherstring[]or a comma-separatedstringin the JSON schemas/tool definitions. - Normalize comma-separated label strings into a trimmed string array during safe-output validation.
- Add a unit test and update the safe-outputs specification examples to reflect the compatibility contract.
Show a summary per file
| File | Description |
|---|---|
| schemas/agent-output.json | Expands create_issue.labels schema type to ["array","string"]. |
| pkg/workflow/js/safe_outputs_tools.json | Documents and allows labels as array | string for the create_issue tool schema. |
| pkg/workflow/js/safe_outputs_tools.json | Documents and allows labels as array | string for the create_issue tool schema. |
| actions/setup/js/safe_outputs_tools.json | Keeps runtime tool schema in sync with the compiler copy (adds array | string + docs). |
| docs/src/content/docs/specs/safe-outputs-specification.md | Updates spec examples to show labels as ["array","string"] and documents compatibility. |
| actions/setup/js/safe_output_type_validator.test.cjs | Adds a focused unit test for string-label normalization behavior. |
| actions/setup/js/safe_output_type_validator.cjs | Adds normalization logic to split/trim/filter comma-separated label strings before array validation. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 2
| "labels": { | ||
| "type": "array", | ||
| "type": ["array", "string"], | ||
| "description": "Optional labels to add to the issue", | ||
| "items": { | ||
| "type": "string" |
| // Backward compatibility: agents sometimes provide comma-separated labels as a string. | ||
| // Normalize this into a string array before strict array validation. | ||
| if (fieldName === "labels" && typeof value === "string") { | ||
| value = value | ||
| .split(",") |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #38738 does not have the 'implementation' label (has_implementation_label=false) and has only 6 new lines of code in business logic directories (default_business_additions=6), well below the 100-line threshold. Neither Condition A nor Condition B is met. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two correctness issues need fixing before merge
The backward-compat normalization for comma-separated labels is a sound idea, but two issues need to be addressed.
Blocking findings (2)
1. Schema–runtime mismatch for add_labels and create_pull_request (see inline comment on line 414)
The runtime normalises any labels field named "labels" regardless of tool type, so add_labels and create_pull_request silently accept strings too — but their tool schemas still declare type: "array". Either extend the schema update to cover those tools (preferred, since the behaviour is beneficial for all three), or restrict the normalisation to create_issue via an itemType guard.
2. Empty string bypasses required-field validation for add_labels (see inline comment on line 418)
labels: "" normalises to [], which passes Array.isArray and therefore satisfies the required: true constraint on add_labels.labels. The call then silently applies zero labels. Add a post-normalisation guard that returns an error when value.length === 0 && validation.required.
🔎 Code quality review by PR Code Quality Reviewer · 274.6 AIC · ⌖ 14.3 AIC · ⊞ 17.3K
| if (validation.type === "array") { | ||
| // Backward compatibility: agents sometimes provide comma-separated labels as a string. | ||
| // Normalize this into a string array before strict array validation. | ||
| if (fieldName === "labels" && typeof value === "string") { |
There was a problem hiding this comment.
Normalization scope wider than schema update: this condition fires for every field named labels — including add_labels.labels (required) and create_pull_request.labels — but only create_issue's schema was updated to ["array", "string"]. The LLM still sees type: "array" for those tools yet the runtime silently converts string input.
💡 Details and fix options
Tools affected by the runtime normalisation but not by the schema update:
add_labels.labels— required field;safe_outputs_tools.jsonstill declarestype: "array"create_pull_request.labels— optional field; same schema mismatch
The normalisation is arguably good behaviour for all three tools, but the inconsistency means:
- Agents that read the schema may be confused about which tools accept strings.
- A future engineer removing the
create_issuebranch might inadvertently break the hidden contract for the other tools.
Option A (prefer this): extend the schema update to add_labels and create_pull_request as well, and add tests for both.
Option B (narrower scope): restrict the normalisation to create_issue only by adding an itemType guard:
if (fieldName === "labels" && itemType === "create_issue" && typeof value === "string") {| value = value | ||
| .split(",") | ||
| .map(item => item.trim()) | ||
| .filter(Boolean); |
There was a problem hiding this comment.
Empty-string input silently produces an empty array and passes required-field validation: labels: "" → split(",") → [""] → filter(Boolean) → []. Array.isArray([]) is true, so add_labels.labels (which is required: true) will pass validation with an empty labels list.
💡 Impact and fix
An add_labels call with labels: "" passes the JS validator but then silently does nothing at the GitHub API level (zero labels to add). The operator sees no error but no label is applied.
The fix is to reject normalised-empty results when the field is required:
if (fieldName === "labels" && typeof value === "string") {
value = value.split(",").map(item => item.trim()).filter(Boolean);
// Guard: empty string must not bypass required validation
if (value.length === 0 && validation.required) {
return {
isValid: false,
error: `Line ${lineNum}: ${itemType} requires a non-empty '${fieldName}' field`,
};
}
}Alternatively, add a minItems: 1 check to the general array validation path for any required array field.
| expect(Array.isArray(result.normalizedItem.labels)).toBe(true); | ||
| }); | ||
|
|
||
| it("should normalize comma-separated labels string to array", async () => { |
There was a problem hiding this comment.
Test coverage gap: only create_issue with a two-element comma string is tested, leaving undocumented (and unverified) the behaviour for the other two tools that share the same code path.
💡 Suggested additions
Add at least these cases:
// Verify the normalization is intentional for add_labels
it("should normalize comma-separated labels string for add_labels", async () => {
const { validateItem } = await import("./safe_output_type_validator.cjs");
const result = validateItem({ type: "add_labels", labels: "bug, enhancement" }, "add_labels", 1);
expect(result.isValid).toBe(true);
expect(result.normalizedItem.labels).toEqual(["bug", "enhancement"]);
});
// Guard against empty-string silent pass for a required array field
it("should reject empty string for required labels field", async () => {
const { validateItem } = await import("./safe_output_type_validator.cjs");
const result = validateItem({ type: "add_labels", labels: "" }, "add_labels", 1);
expect(result.isValid).toBe(false); // fails today — empty array passes required check
});The second test will fail with the current code, surfacing the empty-string issue noted on the normalization block.
|
``
|
|
``
|
|
@copilot run pr-finisher skill |
…escription Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 984a91d:
Local |
Daily Reliability Review runs were intermittently raising “No Safe Outputs Generated” even when the agent emitted a
create_issueoutput. The failure path was a strict validator rejection whenlabelsarrived as a comma-separated string instead of an array.Root cause
safe_output_type_validatorrequiredcreate_issue.labelsto be an array and rejected string input ("a,b"), causing the emitted safe output item to be dropped and the run to be reported as producing no safe outputs.Change
labelscreate_issuetool schema/metadata to document and accept compatibility input:labelsnow supportsarray | stringin tool JSON/schema definitionsTest coverage
create_issueacceptslabels: "reliability, telemetry"and normalizes to["reliability", "telemetry"].