Normalize JSON-encoded safe-output labels - #50364
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
TriageCategory: bug | Risk: low | Score: 53/100 (impact 20, urgency 15, quality 18) Recommended action: batch_review Small, well-scoped fix (29+/6-, 3 files) for JSON-encoded safe-output label normalization, includes tests. Draft PR — good candidate to batch with other small safe-output related fixes.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ 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 (0 additions in default business logic dirs). |
There was a problem hiding this comment.
Pull request overview
Normalizes JSON-encoded create-issue labels while preserving comma-separated compatibility.
Changes:
- Parses valid JSON-array label strings before validation.
- Adds regression coverage for
'["cookie"]'. - Registers the maintainer workflow guidance file.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/safe_output_type_validator.cjs |
Adds JSON-array label normalization. |
actions/setup/js/safe_output_type_validator.test.cjs |
Tests the reported regression. |
.github/skills/agentic-workflows/SKILL.md |
Adds maintainer guidance to the router list. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠Applied /diagnosing-bugs and /tdd — no blocking issues; two observations worth considering.📋 Key Themes & Highlights
#### Key Themes- Scope gap: The JSON-array normalisation is applied only to create_issue labels, but add_labels, remove_labels, and update_issue share a similar string-coercion path via ISSUE_INTENT_LABEL_TYPES. If agents emit JSON-encoded strings for those types the same bug would recur.- Thin test coverage: One happy-path test is added; edge cases such as whitespace padding (' ["cookie"] '), invalid JSON ('["cookie"'), and non-array JSON ('"cookie"') are not yet covered.#### Positive Highlights- ✅ Clear try/catch fallback preserves backward-compatible comma-separated parsing- ✅ typeof value === "string" re-check after the JSON branch prevents double-normalisation- ✅ PR description shows the concrete before/after clearly
📋 Key Themes & Highlights
#### Key Themes- Scope gap: The JSON-array normalisation is applied only tocreate_issue labels, but add_labels, remove_labels, and update_issue share a similar string-coercion path via ISSUE_INTENT_LABEL_TYPES. If agents emit JSON-encoded strings for those types the same bug would recur.- Thin test coverage: One happy-path test is added; edge cases such as whitespace padding (' ["cookie"] '), invalid JSON ('["cookie"'), and non-array JSON ('"cookie"') are not yet covered.#### Positive Highlights- ✅ Clear try/catch fallback preserves backward-compatible comma-separated parsing- ✅ typeof value === "string" re-check after the JSON branch prevents double-normalisation- ✅ PR description shows the concrete before/after clearly🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 18.2 AIC · ⌖ 7.94 AIC · ⊞ 7.1K
Comment /matt to run again
There was a problem hiding this comment.
The fix is correct and well-structured. The trimmedValue sentinel check, JSON parse attempt, and comma-split fallback form a clean layered normalization. The regression test covers the reported case. No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 10.8 AIC · ⌖ 7.41 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report
📊 Metrics (1 test)
|
There was a problem hiding this comment.
Verdict: approve with non-blocking follow-ups — the JSON-array label normalization works for the intended ["cookie"] case, but the fallback logic has two edge-case gaps.
💡 Themes
- Non-array JSON values (e.g. an object) silently fall through to comma-split parsing of the raw string instead of being rejected, producing malformed labels.
- Non-string items inside a JSON array bypass the trim/filter normalization applied to the comma-separated path, causing inconsistent behavior between the two code paths.
- Test coverage only covers the happy path (
'["cookie"]'); no tests for malformed JSON or non-string array elements.
None of these are crash/security risks, but they're worth tightening given this fix's own motivation was a subtle silent-corruption bug.
🔎 Code quality review by PR Code Quality Reviewer · auto · 85.1 AIC · ⌖ 4.48 AIC · ⊞ 7.9K
Comment /review to run again
| } catch { | ||
| // Fall back to comma-separated parsing below. | ||
| } | ||
| } |
There was a problem hiding this comment.
JSON-array elements are not validated/coerced to strings before assignment, so non-string items (numbers, objects, nested arrays) silently bypass the trim/filter normalization the comma-separated path applies.
💡 Details and fix
When value = parsedValue on line 550, any non-string element (e.g. labels: '[1, {"a":1}]') is passed straight through to the downstream hasInvalidItem check, which will reject it — but with a generic error rather than being normalized the way trimmed comma-separated strings are. More importantly, string elements from JSON are not trimmed, so '[" cookie "]' yields " cookie " untrimmed, unlike the comma-separated path which trims every item.
Suggested fix:
if (Array.isArray(parsedValue) && parsedValue.every(item => typeof item === "string")) {
value = parsedValue.map(item => item.trim()).filter(Boolean);
}This keeps both normalization paths consistent (trimmed, non-empty strings) and avoids let non-string JSON elements slip past without the same guarantees as the comma-separated fallback.
| if (trimmedValue.startsWith("[")) { | ||
| try { | ||
| const parsedValue = JSON.parse(trimmedValue); | ||
| if (Array.isArray(parsedValue)) { |
There was a problem hiding this comment.
When JSON.parse succeeds but yields a non-array (e.g. an object or number), value stays the original raw string and silently falls through to comma-split parsing — producing a bogus single-element label like [object Object] instead of failing validation.
💡 Details and fix
Example: labels: '{"a":1}' starts with {, not [, so it skips the startsWith("[") branch entirely and goes straight to comma-split, yielding ['{"a":1}'] — a single garbage label silently accepted. But even a JSON value that does start with [ but parses to something odd, e.g. '[1,2].toString()'-style edge cases or template artifacts, could produce unexpected values passed straight into the comma-split fallback as a full string.
More concretely, if trimmedValue.startsWith("[") is true but Array.isArray(parsedValue) is false (e.g. '["a"].length' won't parse, but a bracketed non-array JSON value like a nested structure that parses to, say, a string "[abc]" after some encoding round-trip) the code falls through with value still equal to the original unparsed string (not parsedValue), and that raw string — including its literal [ and ] characters — gets comma-split, producing a single malformed label containing brackets.
Suggested fix: explicitly reject (or log/no-op) when parsing succeeds but doesn't yield an array, rather than silently falling back to comma-splitting the original bracketed string:
if (trimmedValue.startsWith("[")) {
try {
const parsedValue = JSON.parse(trimmedValue);
if (Array.isArray(parsedValue)) {
value = parsedValue;
} else {
value = []; // or leave as invalid array, don't fall through to comma-split
}
} catch {
// Fall back to comma-separated parsing below.
}
}
|
@copilot Please follow up on the remaining review feedback, refresh the branch if GitHub allows it, and run the Outstanding review notes:
There are no currently failed checks in the compact snapshot.
|
… tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit
|
|
🎉 This pull request is included in a new release. Release: |
Create-issue safe outputs treated JSON-encoded label arrays as comma-separated strings. A payload containing
["cookie"]was sanitized into the literal[cookie]label.Changes
Normalize labels
Regression coverage