fix(eslint-factory): exempt console.error/console.warn from prefer-core-logging - #46552
Conversation
…re-logging rule console.error and console.warn write to process.stderr, while core.error and core.warning emit workflow commands to process.stdout. For stdio MCP servers that own stdout as a JSON-RPC channel, the substitution is not behavior-preserving and would corrupt the stream. - Remove `error` and `warn` from CONSOLE_TO_CORE map so they are never flagged - Update tests: console.error/console.warn now have valid tests documenting the exemption - Add prefer-core-logging README section with stream-semantics rationale Closes #46541 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46552 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Exempts stderr-based console methods from unsafe stdout rewrites.
Changes:
- Removes
console.error/console.warnmappings. - Adds exemption tests and stream-semantics documentation.
- Preserves other console-to-core mappings.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/prefer-core-logging.ts |
Removes unsafe stderr rewrites. |
eslint-factory/src/rules/prefer-core-logging.test.ts |
Tests error/warn exemptions. |
eslint-factory/README.md |
Documents covered methods and rationale. |
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: 2
- Review effort level: Medium
| // NOTE: console.error and console.warn are intentionally excluded. | ||
| // Those methods write to process.stderr, while core.error / core.warning emit | ||
| // GitHub Actions workflow commands to process.stdout. For processes that own | ||
| // stdout as a data/protocol channel (e.g. stdio MCP servers), rewriting | ||
| // stderr logging to stdout would corrupt the stream. The substitution is not | ||
| // behavior-preserving and must not be auto-applied. |
| `console.warn("warning");`, | ||
| `const core = require("@actions/core"); console.warn("warning");`, | ||
| ], | ||
| invalid: [], |
![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 — root cause properly addressed with good regression coverage.
📋 Key Themes & Highlights
Key Themes
- Bug fix is correct: Removing
warnanderrorfromCONSOLE_TO_COREis the right, minimal change. The root cause (wrong stream semantics) is addressed, not just the symptom. - Tests are solid: Invalid cases converted to valid ones with clear explanatory comments. The MCP-server fixture (
console.error("MCP transport error:", ...)) pins the intent nicely. - Documentation is thorough: README callout clearly explains the stderr vs stdout distinction.
Positive Highlights
- ✅ Minimal, surgical change — existing
log/info/debugbehavior is entirely untouched - ✅ Code comments in both the rule and tests explain why the exemption exists
- ✅ README covers both the coverage table and the explicit exclusion rationale
One minor suggestion (non-blocking): the console.warn valid block has 2 fixtures, while console.error has 3 (including a multi-arg MCP-server-like case). Adding a parallel multi-arg fixture for console.warn would make the symmetry explicit, but this is purely cosmetic.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 15 AIC · ⌖ 4.42 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
Review: prefer-core-logging exemption for console.error/console.warn
The rationale is sound — stdout/stderr stream semantics differ, and rewriting console.error/console.warn to core.error/core.warning would corrupt stdio-protocol channels such as MCP servers. The implementation, tests, and docs are consistent.
Two existing inline comments (already filed) cover the remaining gaps:
- Stale
meta.docs.descriptionstill mentionscore.errorandcore.warning(line 11) - Missing
console.infoinvalid test — the suite doesn't verifyconsole.info → core.infostill fires (line 115)
No additional blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 13.9 AIC · ⌖ 8.12 AIC · ⊞ 5K
There was a problem hiding this comment.
Looks good
The exemption of console.error/console.warn from prefer-core-logging is correct and well-justified. The stderr-vs-stdout stream semantics rationale is sound (especially relevant for stdio MCP servers). Implementation is minimal and precise: dropping the two entries from CONSOLE_TO_CORE is the right approach since rule detection is gated on that map. Tests and README are accurate.
🔎 Code quality review by PR Code Quality Reviewer · 21 AIC · ⌖ 4.33 AIC · ⊞ 5.6K
Comment /review to run again
🧪 Test Quality Sentinel Report
📊 Metrics (11 tests)
|
|
🎉 This pull request is included in a new release. Release: |
prefer-core-loggingwas offering an autofix rewritingconsole.error/console.warn→core.error/core.warning. These two families use different streams:console.error/console.warnwrite to stderr;core.*methods emit GitHub Actions workflow commands to stdout. For stdio MCP servers that own stdout as a JSON-RPC channel, the substitution silently corrupts the stream.Changes
prefer-core-logging.ts— DroperrorandwarnfromCONSOLE_TO_CORE. The rule no longer reports or suggests replacements for those two methods.console.log,console.info, andconsole.debug→core.*behavior is unchanged.prefer-core-logging.test.ts— Convert the twoconsole.error/console.warninvalid cases into valid cases that document the stderr exemption. Adds explicit MCP-server-like fixtures (e.g.console.error("MCP transport error:", ...)) to pin the intent.README.md— Addprefer-core-loggingrule entry and section. Includes a covered-methods table and an explicit callout explaining whyconsole.error/console.warnare excluded: