[eslint-monster] Replace console.log with core.info in setup/js OTLP and outcome scripts#47416
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
console.log with core.info in setup/js OTLP and outcome scripts
|
✅ 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 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
Replaces direct runtime console.log calls with the shared core.info logging path.
Changes:
- Adds the logging shim to seven scripts.
- Migrates OTLP, outcome, AI-credit, generator, and API-test messages.
- Existing logging tests require corresponding updates.
Show a summary per file
| File | Description |
|---|---|
action_conclusion_otlp.cjs |
Migrates conclusion-span logging. |
action_setup_otlp.cjs |
Migrates setup-span logging. |
ai_credits_context.cjs |
Migrates credit provenance logging. |
emit_outcome_spans.cjs |
Migrates outcome telemetry logging. |
evaluate_outcomes.cjs |
Migrates evaluation summary logging. |
generate_safe_outputs_tools.cjs |
Normalizes debug logging. |
test-live-github-api.cjs |
Migrates live-test informational logging. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Medium
| try { | ||
| appendFileSync(filePath, `${key}=${value}\n`); | ||
| console.log(`[otlp] ${logLabel} written to ${fileLabel}`); | ||
| core.info(`[otlp] ${logLabel} written to ${fileLabel}`); |
There was a problem hiding this comment.
Fixed in commit 14e0979. The action_setup_otlp.test.cjs suite now sets up a mockCore object with vi.fn() stubs before module load (so shim.cjs's guard leaves it in place), and all console.log assertions have been replaced with mockCore.info. All 48 tests in this file pass.
|
|
||
| if (endpoints) { | ||
| console.log(`[otlp] sending conclusion span "${spanName}" to configured endpoints`); | ||
| core.info(`[otlp] sending conclusion span "${spanName}" to configured endpoints`); |
There was a problem hiding this comment.
Fixed in commit 14e0979. The action_conclusion_otlp.test.cjs suite now sets up a mockCore object before module load and all console.log assertions at lines 56, 81, 87, and 119 have been updated to assert mockCore.info. All 33 tests in this file pass.
|
|
||
| if (evaluations.length === 0 && (!summary || summary.total_outcomes === 0)) { | ||
| console.log("[outcome-otel] No outcome evaluations to export"); | ||
| core.info("[outcome-otel] No outcome evaluations to export"); |
There was a problem hiding this comment.
Fixed in commit 14e0979. The emit_outcome_spans.test.cjs suite now sets up a mockCore object before module load and all three console.log assertions (lines 171, 326, 359) have been updated to assert mockCore.info. All 3 tests in this file pass.
There was a problem hiding this comment.
Changes look clean and correct. The shim pattern is idempotent, the typeof core guard removal in generate_safe_outputs_tools.cjs is correct now that shim.cjs is always required, and error paths correctly retain console.error. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 12.6 AIC · ⌖ 5.12 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — clean, consistent migration. No blocking issues.
📋 Themes & Highlights
Positive Highlights
- ✅ Consistent pattern: every affected script now starts with
require("./shim.cjs")before usingcore.info - ✅
generate_safe_outputs_tools.cjscorrectly removes the deadtypeof core !== "undefined"guard — the shim makes that branch unreachable - ✅ Message text preserved verbatim, keeping log searchability intact
One Item to Note
test-live-github-api.cjs is a local dev script (shebang, reads GITHUB_TOKEN from env). After adding the shim, core.info(...) writes to stderr instead of stdout. This is by design per the shim comment (stderr avoids interleaving with JSON-RPC stdout), but developers running this interactively will see output on stderr. Minor UX trade-off, not a blocker.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 23.9 AIC · ⌖ 4.55 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
Review: COMMENT (non-blocking)
The → migration and injection are mechanically correct. The shim properly guards against re-installation in github-script contexts, routes to stderr to avoid stdout pollution in MCP servers, and provides all methods actually called by the changed files.
One non-blocking finding
**** has a partial migration: success paths were switched to core.info but all error paths remain as console.error. Both end up on stderr (the shim does the same), but the mixed API will still trigger lint warnings and confuses readers about which logging API is canonical for this file. Either complete the migration (core.error) or revert to console.* throughout since this is a standalone dev script.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 43 AIC · ⌖ 4.61 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/test-live-github-api.cjs:23
Incomplete migration: error paths still use console.error while success paths were switched to core.info, leaving this standalone dev script with a mixed logging surface.
<details>
<summary>💡 Details</summary>
The shim routes core.info to process.stderr via writeShimLog, so both core.info and console.error end up on stderr anyway — but mixing the two signals the conversion was incomplete. Linters will still flag the remaining console.error calls, and a future reader won't…
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit For each of the three affected test files, I set up a |
|
@copilot please run the Unresolved review feedback:
Context: maintainer follow-up requested fixing the failing workflow step at https://github.com/github/gh-aw/actions/runs/29964378716/job/89072723116#step:7:1
|
…pi.cjs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit The PR Code Quality Reviewer flagged The referenced CI failure (run 29964378716) was already fixed by the prior commit ( |
|
@copilot please run the Unresolved review feedback:
|
All code issues are addressed. Nothing further to fix:
The 3 review threads are still marked |
|
@copilot please run the Unresolved review feedback:
|
|
🎉 This pull request is included in a new release. Release: |
lint:setup-jsflagged a concentratedprefer-core-loggingstream inactions/setup/jswhere runtime scripts were still usingconsole.log. This PR switches those logs to the action logging shim path (core.info) while preserving message text and execution flow.Scope: targeted runtime scripts
action_conclusion_otlp.cjsaction_setup_otlp.cjsemit_outcome_spans.cjsevaluate_outcomes.cjsai_credits_context.cjsgenerate_safe_outputs_tools.cjstest-live-github-api.cjsLogging path normalization
console.log(...)withcore.info(...)in the affected call sites.Node execution compatibility
require("./shim.cjs")to these scripts socore.infois available consistently in non-github-scriptNode execution contexts.(run: https://github.com/github/gh-aw/actions/runs/29966259561)