eslint-factory: add empty-string suggestion for null/undefined in no-core-setoutput-non-string

[Copilot]

The no-core-setoutput-non-string autofix for null/undefined only offered String(...), producing the literal strings "null"/"undefined" — truthy non-empty values in Actions expressions — which is the exact failure mode the rule warns about.

Changes

• no-core-setoutput-non-string.ts: For null and undefined kinds, prepend a primary useEmptyString suggestion that replaces the value with "", keeping String(...) as a secondary option. Numeric, boolean, and .length kinds are unchanged.
• no-core-setoutput-non-string.test.ts: Updated null/undefined test cases to assert both suggestions in order.

// Before: only one suggestion
core.setOutput("result", null);
// → String(null)  ❌ produces truthy "null"

// After: two suggestions, empty-string first
core.setOutput("result", null);
// → ""            ✅ (useEmptyString — primary)
// → String(null)     (wrapWithString — secondary, for callers who want the literal word)

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #42723 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[Copilot]

Pull request overview

This PR improves the no-core-setoutput-non-string ESLint rule’s autofix UX for null/undefined values passed to core.setOutput, by adding an empty-string replacement suggestion ahead of the existing String(...) wrapping suggestion to avoid producing truthy "null"/"undefined" outputs in GitHub Actions expressions.

Changes:

• Added a new useEmptyString suggestion message and conditionally prepended it for null/undefined cases.
• Kept wrapWithString as a secondary suggestion for null/undefined, leaving other kinds unchanged.
• Updated tests to assert both suggestions (and their ordering) for null/undefined.

Show a summary per file

File	Description
eslint-factory/src/rules/no-core-setoutput-non-string.ts	Adds an empty-string suggestion for null/undefined before the existing String(...) suggestion.
eslint-factory/src/rules/no-core-setoutput-non-string.test.ts	Updates null/undefined test expectations to include both suggestions in order.

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: 0
• Review effort level: Low

[github-actions]

⚠️ PR Code Quality Reviewer failed during code quality review.

[github-actions]

Review: eslint-factory — empty-string suggestion for null/undefined

The change is correct, focused, and well-tested.

Correctness:

• isNullOrUndefined checks kind === "null" || kind === "undefined", exactly matching the values returned by nonStringKind() — no gaps or over-matching.
• The spread ...(isNullOrUndefined ? [...] : []) correctly prepends the suggestion only for null/undefined; numeric, boolean, and .length cases are untouched.
• useEmptyString fixer calls fixer.replaceText(valueArg, "\"\"") — replaces the whole argument node cleanly, consistent with wrapWithString.
• as const on both messageId fields is required by the typed suggestion API and is correct.
• TSESLint.RuleFixer import is added to the existing @typescript-eslint/utils destructuring — no new dependency.

Tests:
Both null and undefined test cases now assert suggestions in the correct order (useEmptyString first, wrapWithString second). Numeric/boolean/.length cases are intentionally unaffected.

Semantics:
The original bug — that the only autofix produced truthy literal strings "null"/"undefined" — is directly fixed. Empty-string is the correct primary suggestion when the intent is "no value".

Approved. ✅

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 24.3 AIC · ⌖ 9.01 AIC · ⊞ 4.9K

[github-actions]

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🧪 Test quality analysis by Test Quality Sentinel · 33.7 AIC · ⌖ 17.6 AIC · ⊞ 6.9K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 100/100. 0% implementation tests (threshold: 30%).

[pelikhan]

@copilot run pr-finisher skill

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — COMMENT (no blocking issues, a few improvement suggestions).

📋 Key Themes & Highlights

Key Themes

• Message/suggestion inconsistency: The nonStringValue error message still prescribes String() wrapping for all value kinds, which contradicts the new primary useEmptyString suggestion for null/undefined. Users who read the message without applying a suggestion could be steered toward the exact failure mode the rule warns about.
• Loose string coupling in isNullOrUndefined: The check compares against the human-readable strings returned by nonStringKind(). There is no compile-time guarantee these strings won't drift — a typed discriminant or shared constant would be safer.
• Implicit suggestion-order invariant in tests: The ordering of suggestions (empty-string first) is a meaningful behavioural contract; a brief comment to document this intent would make the tests more self-explanatory as specs.

Positive Highlights

• ✅ Root cause is correctly identified and addressed: the old autofix produced the very string the rule warns about.
• ✅ Conditional spread pattern for per-kind suggestions is clean and idiomatic.
• ✅ Explicit as const casts and typed TSESLint.RuleFixer parameters remove implicit any risks.
• ✅ Both test cases (null and undefined) are updated symmetrically with the correct two-suggestion assertion.
• ✅ The PR description is clear, well-structured, and includes before/after code examples.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 68.3 AIC · ⌖ 7.6 AIC · ⊞ 6.6K
_{Comment /matt to run again}

[github-actions]

[/diagnosing-bugs] The nonStringValue diagnostic message still directs users to "Wrap with String({{valueText}})" for all kinds — including null/undefined — contradicting the new primary useEmptyString suggestion. Users who read only the error message (not the suggestions) may still apply the wrong fix.

💡 Suggested update

Consider softening the error message so it does not prescribe String() for all value kinds:

"The setOutput value {{valueText}} is a {{kind}}. Implicit coercion may produce unexpected strings such as 'null' or 'true' in downstream workflow expressions. Use an explicit string conversion — see the available suggestions."

This removes the potentially misleading String() prescription and lets the per-kind suggestions guide the user.

@copilot please address this.

[github-actions]

[/tdd] The isNullOrUndefined check compares against the human-readable kind strings ("null" / "undefined") returned by nonStringKind(). These are implementation details with no type contract — if the return value ever changes (e.g. to "null literal"), this silently stops working with no compile-time error.

💡 Suggested improvement

Extract a typed discriminant or use a constant/set for the check:

const NULL_OR_UNDEFINED_KINDS = new Set(["null", "undefined"] as const);
const isNullOrUndefined = NULL_OR_UNDEFINED_KINDS.has(kind as string);

Or refactor nonStringKind to return a tagged union so the compiler catches future renames. At minimum, consider adding a test that explicitly names this coupling so it acts as a regression guard.

@copilot please address this.

[github-actions]

[/tdd] The useEmptyString suggestion test for undefined omits data — which is correct since the message has no placeholders — but the ordering assertion is implicit (array index). A future reorder would not be caught by the test name alone.

💡 Consider explicit ordering documentation

Add a brief comment in the test to make the ordering intent explicit as a spec:

suggestions: [
  // Primary: use empty string rather than the literal word 'undefined'
  { messageId: "useEmptyString", output: `core.setOutput("result", "");` },
  // Secondary: make coercion explicit for callers who need the literal
  { messageId: "wrapWithString", data: { valueText: "undefined" }, output: `core.setOutput("result", String(undefined));` },
],

This small addition turns the test into a readable spec for the ordering invariant.

@copilot please address this.

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

Done in 927505a. I ran the PR finisher pass: addressed the open rule/test feedback with minimal updates, re-ran local validation (make fmt && make lint && make test-unit && make test && make recompile), and pushed the changes.