fix(cli): route bootstrap installation logs to stderr to prevent MCP stream pollution#57
Conversation
|
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 ( |
There was a problem hiding this comment.
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.
| 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(); | ||
| } |
There was a problem hiding this comment.
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.
| 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(); | |
| } |
|
Hey @AsifpMulla123 — clean replacement, no problem. The spacing fix on line 151 is in, the diff is much tidier, and the new One real issue I want to flag before merge — gemini caught a regression on 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), The fix is one line — bind the method to keep 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 Once that lands I'll merge. Happy to push the fix for you if you want — just say the word. 🙏 |
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.
|
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):
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. |
|
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 Thank you so much for your patience and for guiding me through to the finish line on this! 🙏 |
|
Hey @AsifpMulla123 — A couple of small things worth flagging for context, not blockers:
Merging now — this is your first merged PR to this repo and the CLI is meaningfully better for it. Looking forward to more 🙌 |
Linked issue
Closes #49
Type of change
What changed
Pivoted the installation telemetry and bootstrap text blocks inside
cli/bin/cli.jsfromconsole.logtoconsole.error. This guarantees that user-facing text messages route directly tostderr, keepingstdoutcompletely 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:
Output/Results:
stderr.output.jsonfile remained completely empty (0 bytes), confirming thatstdoutis unpolluted and safe for headless MCP configurations.Claim confirmation
If this PR closes a
good first issueorhelp wantedissue, please confirm:.github/workflows/star-check.yml) — the "Star Check" status will block merge until you star.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:runpasses locally (or pre-existing failures are unrelated to this change)npm run buildsucceeds locally## [Unreleased](skip for docs-only or test-only PRs)fix(panel): ...)Screenshots / GIFs
Additional notes