eslint-factory: flag core.error + core.setFailed(prefix + msg) as redundant

[Copilot]

The no-core-error-then-setfailed rule only fired on exact source-text matches, missing the codebase's dominant pattern where setFailed prepends an error code to the same message — 9 live sites, zero flagged.

Changes

• no-core-error-then-setfailed.ts — adds isSetFailedArgPrefixedVersion: recognises when setFailedArg is a TemplateLiteral whose content ends with the full content of errorArg (also a TemplateLiteral), with only a non-empty prefix in front. The prefix may include template expressions (e.g. ${ERR_API}: ). Suffix/interleaved additions remain unflagged.

• no-core-error-then-setfailed.test.ts — moves the former valid prefix-pattern case to invalid; adds two new invalid cases (literal prefix, expression prefix); adds a suffix valid case.

Now flagged (previously missed)

core.error(`Failed to add reaction: ${errorMessage}`);
core.setFailed(`${ERR_API}: Failed to add reaction: ${errorMessage}`);
//             ^^^^^^^^^^^^ prefix — entire errorArg content is a strict suffix

Remains valid (suffix adds distinct context)

core.error(`Failed: ${msg}`);
core.setFailed(`Failed: ${msg} [extra context]`);  // suffix — not flagged

• Fixes eslint-factory: no-core-error-then-setfailed's strict text-equality check never fires on the codebase's dominant core.error/setF [Content truncated due to length] #49491

---

Run: https://github.com/github/gh-aw/actions/runs/30689325959

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 14.6 AIC · ⌖ 13.9 AIC · ⊞ 8.1K · ◷
_{Comment /souschef to run again}

[github-actions]

✅ 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.

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

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

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[Copilot]

Pull request overview

Extends the ESLint rule to detect redundant core.error() calls when core.setFailed() prefixes the same template message.

Changes:

• Adds template-literal prefix matching.
• Adds prefix and suffix test cases.
• Regenerates one unrelated workflow lock file.

Show a summary per file

File	Description
eslint-factory/src/rules/no-core-error-then-setfailed.ts	Implements prefixed-message detection.
eslint-factory/src/rules/no-core-error-then-setfailed.test.ts	Tests literal/expression prefixes and suffix exclusion.
.github/workflows/daily-byok-ollama-test.lock.yml	Adds generated model-routing and pricing configuration.

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: 3
• Review effort level: Balanced

[github-actions]

The isSetFailedArgPrefixedVersion implementation is correct and well-guarded. The null checks for cooked values, expression count guards, and suffix/junction quasi matching all look sound. Test cases properly cover literal prefix, expression prefix, and the suffix non-match case. No blocking issues.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 12.5 AIC · ⌖ 7.15 AIC · ⊞ 5.3K

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 100/100 — Excellent

Analyzed 2 test(s): 2 design, 0 implementation, 0 violation(s).

📊 Metrics (2 tests)

Metric	Value
Analyzed	2 (TypeScript vitest)
✅ Design	2 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	2 (100%)
Duplicate clusters	0
Inflation	No (0.32:1 ratio)
🚨 Violations	0

Test	Location	Classification
code: core.error(...); core.setFailed(ERR: ...);	line 84	Design, error case, suggestion verified
code: core.error(...); core.setFailed(${ERR_API}: ...);	line 94	Design, error case, real-world pattern

Verdict

✅ passed. 0% implementation tests (threshold: 30%). Both new test cases verify the core ESLint rule logic for detecting prefixed template literals with exact output assertions.

Highlights:

• Real-world coverage: second test mirrors actual codebase pattern from add_reaction.cjs:184-185
• Minimal test inflation: 22 new test lines vs 68 source lines
• Complete error specification: both tests verify messageId, suggestions, and exact output format

🧪 Test quality analysis by Test Quality Sentinel · haiku45 · 11.6 AIC · ⌖ 6.75 AIC · ⊞ 8.4K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 100/100. 0% implementation tests (threshold: 30%). Both new test cases verify the core ESLint rule logic for detecting prefixed template literals with exact output assertions. Real-world coverage mirrors add_reaction.cjs pattern.

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — requesting changes for two targeted issues.

📋 Key Themes & Highlights

Issues

• Null guard gap (no-core-error-then-setfailed.ts:75): intermediate quasis are compared with !== without guarding for cooked === null. Since null !== null is false, two quasis whose escape sequences are both invalid would be incorrectly treated as equal, creating false-positive lint reports. The junction-quasi check at lines 81–82 already does the right thing — apply the same pattern to the inner loop.
• Missing test (no-core-error-then-setfailed.test.ts): the expression-first errorArg path (${msg} with no leading text) is not covered. Since endsWith('') is always true, the empty-leading-quasi edge case relies entirely on the prefixExprCount > 0 guard — a test would lock in this behaviour.

Positive Highlights

• ✅ Algorithm design is sound: suffix additions are correctly excluded, expression alignment via source-text comparison is robust
• ✅ Comprehensive JSDoc comment clearly lays out all six match conditions
• ✅ New tests mirror an actual codebase pattern (add_reaction.cjs:184-185), making the intent concrete

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 36.8 AIC · ⌖ 7.61 AIC · ⊞ 7K
_{Comment /matt to run again}

[github-actions]

[/diagnosing-bugs] Null guard missing for intermediate quasis — when a template literal contains an invalid escape sequence, cooked is null, and null !== null evaluates to false, causing two structurally different null quasis to be treated as equal (false positive).

💡 Suggested fix

const sfCooked = sfTpl.quasis[prefixExprCount + i].value.cooked;
const errCooked = errTpl.quasis[i].value.cooked;
if (sfCooked === null || errCooked === null || sfCooked !== errCooked) return false;

This brings the inner-quasi check in line with the null guard already applied at lines 81–82 for the junction quasi.

@copilot please address this.

[github-actions]

[/tdd] Missing edge-case: errorArg starts with an expression (empty leading quasi). When errTpl.quasis[0].value.cooked === '' and the junction quasi ends with '', endsWith('') is always true, so the prefix guard at line 85 becomes the only safeguard. Add a test to document the expected behaviour:

💡 Suggested test case

// Expression-first error arg with expression prefix in setFailed
{
  code: 'core.error(`${msg}`); core.setFailed(`ERR: ${msg}`);',
  errors: [{ messageId: 'noCoreErrorThenSetFailed', ... }],
},

This exercises the empty-leading-quasi path and confirms that prefixExprCount > 0 correctly drives the non-empty-prefix guard when errFirstCooked === ''.

@copilot please address this.

[github-actions]

Verdict: Comment (no blocking issues)

Solid, well-tested implementation of a legitimate rule improvement. Findings below are non-blocking hardening/test-coverage suggestions.

💡 Themes

• The new prefix-match heuristic in is textually-based (source-text equality of expressions, cooked-value equality of quasis). This is consistent with the existing exact-match approach in the rule but widens the matching surface, so false positives (flagging genuinely non-redundant pairs) are somewhat more likely than before, though the risk is mitigated by this only producing a suggestion, not an autofix.
• Test coverage is good for the primary new cases (literal prefix, expression prefix, suffix-not-flagged) but misses a couple of boundary conditions in the new function: zero-expression error templates with a prefix expression, and the null-guard branch.
• No correctness bugs found in the array-indexing logic itself (quasis/expressions loop bounds were checked and are consistent with invariants).

🔎 Code quality review by PR Code Quality Reviewer · auto · 64.8 AIC · ⌖ 10.6 AIC · ⊞ 7.8K
_{Comment /review to run again}

[github-actions]

Expression-prefix matching relies purely on textual source comparison, which can be fooled by non-obvious cases.

💡 Details

Both exactMatch and isSetFailedArgPrefixedVersion compare sourceCode.getText(expr) for equality. This was already the approach for the exact-match case, but the new prefix-matching path widens the blast radius: it now also compares multiple expressions per call pair (all of errTpl.expressions) and glues cooked quasi text together, so more real-world call sites will be auto-matched and their core.error() calls silently removed by the removeErrorCall suggestion.

Textual equality does not guarantee semantic equality — e.g. a shadowed err in nested scope, or an expression whose evaluation has a side effect visible only through equality-of-text but not equality-of-runtime-value (rare, but the existing isSideEffectFree check only protects against side effects in the removed call, not against divergent runtime values between the two call sites).

Suggested hardening: since this is a suggestion (not autofix), reviewers already vet it manually, so the risk is moderate rather than critical. Still worth documenting this limitation explicitly in the function doc comment so future maintainers don't assume textual equality implies semantic equality.

[github-actions]

New prefix-matching branches lack tests for zero-expression error templates and null-cooked quasis, leaving two guard conditions unverified.

💡 Details

isSetFailedArgPrefixedVersion has at least two branches with no covering test case:

1. errTpl.expressions.length === 0 (a plain literal-only core.error(\Failed`)) paired with a setFailedtemplate that has a prefix expression, e.g.core.setFailed(`${ERR_CODE}: Failed`). This exercises prefixExprCount > 0with the loop overerrTpl.expressionsnever running — a distinct code path from the two invalid tests added, which both useerrTpl.expressions.length === 1`.
2. The cooked === null guard (if (junctionCooked === null || errFirstCooked === null) return false;) — reachable when a template literal contains an invalid escape sequence — has no test, so a regression that inverts or removes this null check would go undetected.

Adding two more invalid/valid cases would close this gap and lock in the intended behavior for both edge cases.

[github-actions]

PR Triage

• Category: refactor | Risk: low | Priority: medium
• Score: 43/100 (impact 20 + urgency 8 + quality 15)
• Recommended action: batch_review

Note: ESLint tooling rule addition, has tests, CI green

Generated by 🔧 PR Triage Agent · auto · 48.9 AIC · ⌖ 6.24 AIC · ⊞ 7.9K · ◷

[gh-aw-bot]

@copilot please address the remaining review feedback, refresh the branch if GitHub allows it, and run the pr-finisher skill before handoff.

Current maintainer-facing blockers:

• changes-requested review from Matt Pocock Skills Reviewer on null-cooked quasi guarding in the intermediate-quasi loop
• add coverage for the expression-first / empty-leading-quasi edge case called out in review
• once updated, summarize whether any unresolved reviewer comments can be closed

Please summarize which comments are fully addressed, whether any stale blocking reviews remain, and what still needs maintainer action.

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 14.6 AIC · ⌖ 13.9 AIC · ⊞ 8.1K · ◷
_{Comment /souschef to run again}

done

[Copilot]

@copilot please address the remaining review feedback, refresh the branch if GitHub allows it, and run the pr-finisher skill before handoff.

Current maintainer-facing blockers:...

Addressed all remaining review feedback in commit b5cf65d:

Changes made:

1. BinaryExpression support — Added isSetFailedArgStringConcatPrefixedVersion which flags core.setFailed("ERR: " + \Failed: ${msg}`)when the right operand is identical in source toerrorArg`. Corresponding invalid test added.

2. Null-cooked quasi guarding — The intermediate-quasi loop (previously sfTpl.quasis[...].value.cooked !== errTpl.quasis[...].value.cooked) now explicitly extracts each cooked value, checks for null (produced by invalid Unicode escape sequences like \u{INVALID}), and returns false if either is null.

3. Expression-first / empty-leading-quasi edge case — Added an invalid test: core.error(\${msg} failed`); core.setFailed(`ERR: ${msg} failed`);whereerrorArghas an empty leading quasi. The matcher handles this correctly since"ERR: ".endsWith("") is always true, and the prefix non-empty check (junctionCooked.length > 0`) guards against false exact-equal matches.

Lock file note: The daily-byok-ollama-test.lock.yml change in the PR is the correct compiled output for the current daily-byok-ollama-test.md source (the .md was updated in main before this branch, and make recompile produces the same result). make check-workflow-drift confirms no drift.

Stale blocking reviews: The Matt Pocock Skills Reviewer blocking review on null-cooked quasi guarding and expression-first edge cases is now fully addressed. No other unresolved reviewer comments require maintainer action.

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.84.2