Skip to content

fix(cli): route bootstrap installation logs to stderr to prevent MCP stream pollution#57

Merged
hoainho merged 4 commits into
hoainho:mainfrom
AsifpMulla123:fix/cli-windows-stdout-leak
Jun 7, 2026
Merged

fix(cli): route bootstrap installation logs to stderr to prevent MCP stream pollution#57
hoainho merged 4 commits into
hoainho:mainfrom
AsifpMulla123:fix/cli-windows-stdout-leak

Conversation

@AsifpMulla123

Copy link
Copy Markdown
Contributor

Linked issue

Closes #49

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • Feature (non-breaking change that adds functionality)
  • Performance improvement
  • Documentation only
  • Refactor (no behavior change)
  • Test additions or fixes
  • Chore (tooling, deps, build config)

What changed

Pivoted the installation telemetry and bootstrap text blocks inside cli/bin/cli.js from console.log to console.error. This guarantees that user-facing text messages route directly to stderr, keeping stdout completely pure for strict JSON-RPC data streaming required by Model Context Protocol (MCP) clients.

Testing notes

Ran the non-interactive installation setup while explicitly redirecting standard output to an external JSON file:

$ node cli/bin/cli.js -y ./test-install > output.json

Output/Results:

  • All human-readable diagnostic text printed cleanly to the console via stderr.
  • The resulting output.json file remained completely empty (0 bytes), confirming that stdout is unpolluted and safe for headless MCP configurations.

Claim confirmation

If this PR closes a good first issue or help wanted issue, please confirm:

  • I starred the repo ⭐ — see CONTRIBUTING.md → How to claim. This is now enforced by CI (.github/workflows/star-check.yml) — the "Star Check" status will block merge until you star.
  • I commented I'll take this (or similar) on the issue before starting work, so two people don't accidentally race on the same issue. (Honor-system; not CI-enforced.)

If this PR is from a maintainer or a follow-up to a tracked plan, both can be skipped — just delete this section. Maintainer/bot/tracked-plan-labeled PRs are auto-exempted by the Star Check workflow.

Checklist

  • npm run test:run passes locally (or pre-existing failures are unrelated to this change)
  • npm run build succeeds locally
  • CHANGELOG.md entry added under ## [Unreleased] (skip for docs-only or test-only PRs)
  • PR title follows Conventional Commits (e.g., fix(panel): ...)
  • No new runtime dependencies added without prior discussion in a Discussion

Screenshots / GIFs

Screenshot (589)

Additional notes

@AsifpMulla123

Copy link
Copy Markdown
Contributor Author

Hey @hoainho! Apologies for the noise—had a slight Git branch syncing mishap on the previous thread when pushing the update.

This is the fresh, clean PR with the requested spacing fix on line 151 fully included (printSuccess(fullPath, true);). Ready for your review! 🙌

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the CLI tool to output installation messages to stderr instead of stdout when running in non-interactive mode, and standardizes the codebase to use double quotes. A review comment points out that assigning console methods directly to a variable and invoking them can cause an "Illegal invocation" TypeError in certain environments due to unbound context, and suggests using a wrapper function instead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread cli/bin/cli.js
Comment on lines +39 to 58
function printSuccess(fullPath, useStderr = false) {
const logger = useStderr ? console.error : console.log;
logger();
logger(pc.dim("─".repeat(50)));
logger();
logger(pc.bold("Next steps to load the extension in Chrome:"));
logger();
logger(
` ${pc.cyan("1.")} Open ${pc.yellow("chrome://extensions/")} in Chrome`,
);
logger(
` ${pc.cyan("2.")} Enable ${pc.yellow("Developer mode")} (toggle in top right)`,
);
logger(` ${pc.cyan("3.")} Click ${pc.yellow("Load unpacked")}`);
logger(` ${pc.cyan("4.")} Select the folder:`);
logger(` ${pc.green(fullPath)}`);
logger();
logger(pc.dim("─".repeat(50)));
logger();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Assigning console.log or console.error to a variable and invoking it directly as logger() can lead to a TypeError: Illegal invocation in certain JavaScript runtimes, strict environments, or testing/mocking frameworks where the this context is expected to be bound to the console object.

Using a simple wrapper function or binding the context using .bind(console) is a safer and more robust approach.

Suggested change
function printSuccess(fullPath, useStderr = false) {
const logger = useStderr ? console.error : console.log;
logger();
logger(pc.dim("─".repeat(50)));
logger();
logger(pc.bold("Next steps to load the extension in Chrome:"));
logger();
logger(
` ${pc.cyan("1.")} Open ${pc.yellow("chrome://extensions/")} in Chrome`,
);
logger(
` ${pc.cyan("2.")} Enable ${pc.yellow("Developer mode")} (toggle in top right)`,
);
logger(` ${pc.cyan("3.")} Click ${pc.yellow("Load unpacked")}`);
logger(` ${pc.cyan("4.")} Select the folder:`);
logger(` ${pc.green(fullPath)}`);
logger();
logger(pc.dim("─".repeat(50)));
logger();
}
function printSuccess(fullPath, useStderr = false) {
const log = (msg = "") => useStderr ? console.error(msg) : console.log(msg);
log();
log(pc.dim("─".repeat(50)));
log();
log(pc.bold("Next steps to load the extension in Chrome:"));
log();
log(
` ${pc.cyan("1.")} Open ${pc.yellow("chrome://extensions/")} in Chrome`,
);
log(
` ${pc.cyan("2.")} Enable ${pc.yellow("Developer mode")} (toggle in top right)`,
);
log(` ${pc.cyan("3.")} Click ${pc.yellow("Load unpacked")}`);
log(` ${pc.cyan("4.")} Select the folder:`);
log(` ${pc.green(fullPath)}`);
log();
log(pc.dim("─".repeat(50)));
log();
}

@AsifpMulla123 AsifpMulla123 changed the title Fix/cli windows stdout leak fix(cli): route bootstrap installation logs to stderr to prevent MCP stream pollution Jun 5, 2026
@hoainho hoainho added the pre-star-rule PR opened before the star-check rule landed (2026-06-01); exempt from Star Check CI label Jun 6, 2026
@hoainho

hoainho commented Jun 6, 2026

Copy link
Copy Markdown
Owner

Hey @AsifpMulla123 — clean replacement, no problem. The spacing fix on line 151 is in, the diff is much tidier, and the new pre-star-rule label is on this PR (same false-positive pattern from last time — you're in the stargazer list since 2026-06-04, the read-replica lag just hasn't caught up). Once the label is on, Star Check should auto-re-run and pass.

One real issue I want to flag before merge — gemini caught a regression on cli/bin/cli.js line 58:

function printSuccess(fullPath, useStderr = false) {
  const logger = useStderr ? console.error : console.log;  // ← line 58
  logger();
  logger(pc.dim("─".repeat(50)));
  ...
}

In a module (strict mode), console.log / console.error are bound methods — extracting the reference and calling it as a bare function throws TypeError: Illegal invocation in V8. This would crash the success path of npx @nhonh/react-debugger -y after a successful install, which is exactly the case the MCP JSON-RPC stream needs to stay clean. Your test ("stdout remains at 0 bytes while running non-interactively") likely caught the stderr routing but not the crash because the success message would have been about to be written.

The fix is one line — bind the method to keep this:

function printSuccess(fullPath, useStderr = false) {
  const logger = useStderr ? console.error.bind(console) : console.log.bind(console);
  ...
}

Or if you prefer the arrow wrapper (slightly more explicit):

function printSuccess(fullPath, useStderr = false) {
  const logger = useStderr
    ? (...args) => console.error(...args)
    : (...args) => console.log(...args);
  ...
}

I'd lean toward .bind(console) — it's the standard idiom, no extra allocations on each call.

Once that lands I'll merge. Happy to push the fix for you if you want — just say the word. 🙏

hoainho added a commit that referenced this pull request Jun 6, 2026
Resolves #55. Star Check now waits 8 minutes with exponential backoff (3 attempts × 1.5×: 3s → 4.5s → 6.75s) to handle GitHub's read-replica lag that produced false-negative Star Check failures for valid contributors.

Changes:
- `.github/workflows/star-check.yml`: retry loop + diagnostic metadata
- drops bogus Octokit 'paginate' call that was breaking the API URL
- preserves maintainer + bot exemptions (dependabot, gemini-code-assist, google-cla, github-actions, renovate)
- preserves `tracked-plan` and `pre-star-rule` label bypasses
- maintains `.sisyphus/` directory exclusion

Verified on PR #57 + #58: bypass label works for valid contributors, workflow logs now show clear 'attempt 1/2/3' timing for future debugging.

Closes the contributor pain loop. Every future PR will no longer hit a false-negative on the first CI run.
@hoainho

hoainho commented Jun 6, 2026

Copy link
Copy Markdown
Owner

Hey @AsifpMulla123 — gentle nudge (22h since your last push, 5h since my last message).

Wanted to flag that gemini-code-assist[bot] independently confirmed the same line 58 bug in its inline review on your PR (assigned medium-priority):

"Assigning console.log or console.error to a variable and invoking it directly as logger() can lead to a TypeError: Illegal invocation."

So the bug is real, the fix is 1 line, and the bot is now also pointing at it. The fix is:

- const logger = console.log;
+ const logger = console.log.bind(console);

(or same for the error one, depending on which logger line 58 is)

No rush at all — 22h is fine. If you're busy IRL or have other priorities, totally OK. Just wanted to make sure the line 58 fix didn't get lost behind the spacing cleanup. Let me know if you'd like me to push the 1-line fix to your branch so you can review and merge — happy to do that, or to wait for you.

Once line 58 is fixed this is a clean merge — your stderr/stdout plumbing logic is solid and the spacing cleanup is in.

@AsifpMulla123

Copy link
Copy Markdown
Contributor Author

Hey @hoainho! Apologies for the delay, and thank you for the gentle nudge. You and the Gemini bot are 100% correct—the detached reference would have definitely tripped a TypeError in strict mode.

I just went ahead and pushed the .bind(console) fix directly to the branch myself. The PR should be completely green, updated, and ready for a clean merge now.

Thank you so much for your patience and for guiding me through to the finish line on this! 🙏

@hoainho

hoainho commented Jun 7, 2026

Copy link
Copy Markdown
Owner

Hey @AsifpMulla123.bind(console) is exactly the right fix, and the 1-line diff is the cleanest way to preserve execution context here. Thank you for coming back to it on your own time, and for confirming against gemini's independent flag — that kind of cross-check is the difference between a PR that works and a PR that's durable.

A couple of small things worth flagging for context, not blockers:

  1. The gemini callout is the only place in the file using the detached console.{log,error} pattern — every other log call is bare console.log(...) or console.error(...) which goes through the prototype and is fine. So printSuccess was the lone unsafe extraction site. Your .bind(console) correctly keeps this === console on the receiving end, so the success banner will render as expected after a non-interactive install.

  2. Your non-interactive test (output.json stays at 0 bytes) is the exact scenario the MCP JSON-RPC path cares about — strict mode streams, no diagnostic noise, pure stdout. The bind fix doesn't change that behavior; it just stops a downstream TypeError on the success path that would have crashed the CLI right when the JSON output should be clean.

  3. CI is green (Build + Test 34s, Star Check 3s) and the pre-star-rule label is intact — your star from 2026-06-04 is in the stargazer list, the read-replica lag just never caught up. Not your bug; the workflow now retries with exponential backoff thanks to PR fix(ci): Star Check resilient to read-replica lag (fixes #55 false positive) #56 that landed earlier today.

Merging now — this is your first merged PR to this repo and the CLI is meaningfully better for it. Looking forward to more 🙌

@hoainho hoainho merged commit 6267d8b into hoainho:main Jun 7, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pre-star-rule PR opened before the star-check rule landed (2026-06-01); exempt from Star Check CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: CLI bootstrap leaks initialization diagnostic logs to stdout on Windows

2 participants