Conversation
`scripts/start.js` is the entry point for `npm run start`. When the developer's shell has `CI`, `CONTINUOUS_INTEGRATION`, or any `CI_*` variable set (commonly `CI_TOKEN`, `CI_BUILD_ID`, etc.), `is-in-ci` — loaded transitively by `ink` inside the spawned child — reports CI mode and `ink` disables interactive rendering, leaving the CLI hanging silently after the banner. The bundled path already handles this via an esbuild alias that replaces `is-in-ci` with a constant `false` (google-gemini#4822). That alias is never applied in the unbundled `npm run start` flow, so the same silent-hang regression still bites contributors with a `CI_TOKEN` in their environment. This change strips the offending vars from the env passed to the dev child and prints a single stderr line listing what was removed, plus a one-line note that the variables are not propagated to shell-tool subprocesses in dev — pointing developers who need them to the bundled build. Scope is limited to `scripts/start.js`, so the bundled and production paths are untouched. Verified manually: - with `CI_TOKEN=x CI_BUILD_ID=y npm run start --version`, both vars are listed in the warning and stripped before spawn - with no CI_* vars set, no warning is printed Fixes google-gemini#22452
Reviewer pointed out that `is-in-ci@2.0.0` (the version pinned in this repo's `ink` dependency) only checks `CI` and `CONTINUOUS_INTEGRATION` — it does NOT look at any `CI_*`-prefixed variable, contrary to what issue google-gemini#22452 stated. Source of truth: // node_modules/ink/node_modules/is-in-ci/index.js const check = (key) => key in env && env[key] !== '0' && env[key] !== 'false'; const isInCi = check('CI') || check('CONTINUOUS_INTEGRATION'); The previous patch over-filtered and would clear legitimate developer env vars like `CI_TOKEN` / `CI_BUILD_ID` from the CLI process and from shell-tool subprocesses without that being needed to keep `ink` interactive. This commit: - Narrows the filter to exactly `CI` and `CONTINUOUS_INTEGRATION` - Mirrors `is-in-ci`'s truthiness rule (skip `0` / `false` / unset), so `CI=0` no-ops just like upstream - Updates the comment and stderr warning to say "CI-detection" instead of "CI-related", and explicitly notes that `CI_*` vars are preserved
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a regression where the development environment would hang silently when 'CI' or 'CONTINUOUS_INTEGRATION' environment variables were present. By filtering these specific variables in the development launcher, the CLI ensures that the 'ink' library maintains interactive rendering capabilities during local development, matching the behavior of the production bundled build. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates scripts/start.js to strip CI-detection environment variables, specifically CI and CONTINUOUS_INTEGRATION, before spawning the development child process. This change ensures that the ink library maintains interactive rendering during npm run start, preventing the process from hanging in environments where these variables are set. I have no feedback to provide as there were no review comments.
Summary
npm run starthangs silently after the banner whenever the developer's shell hasCI=trueorCONTINUOUS_INTEGRATION=true.is-in-ci— loaded transitively byinkinside the spawned child — reads those env vars and makesinkdisable interactive rendering.The bundled path closes this via an esbuild alias that replaces
is-in-ciwith a constantfalse(#4822). That alias is never applied in the unbundled dev flow, so the silent-hang regression still bites contributors. See #22452 for the report.This change strips the offending vars from the env passed to the dev child in
scripts/start.jsand prints a one-line stderr warning listing what was removed, plus a short note that the variables are not propagated to shell-tool subprocesses in dev (pointing developers who need them to the bundled build).Scope is intentionally limited to
scripts/start.js, so the bundled and production paths are untouched.Scope of the filter
The filter targets exactly the two variables
is-in-ci@2.0.0actually consults:Issue #22452 claimed any
CI_*-prefixed variable also triggered the hang, but theis-in-cisource above shows that is not the case at the version pinned here. The filter therefore intentionally does not touchCI_TOKEN,CI_BUILD_ID, or any otherCI_*developer-set token — those continue to propagate to the CLI process and to shell-tool subprocesses, matching the bundled-build behavior.The filter also mirrors
is-in-ci's truthiness rule (skip0,false, or unset), so a developer who explicitly setsCI=0to mark "not in CI" still sees no-op behavior here.Why this approach
Two alternatives considered and rejected:
is-in-cito mirror the bundled alias: requires a--importpreloader or ESM loader hook for the dev flow, more moving parts, and still requires changing the spawn invocation.packages/cli/index.tsat process startup: would also run in the bundled binary, duplicating the alias work and printing the warning in production for users who never see a hang.Scrubbing only in the dev launcher is the smallest change that covers the bug, leaves production semantics untouched, and is naturally gated by the dev-mode entry point.
Test plan
Verified manually on Windows (the change is platform-neutral; pure env filtering in Node):
CI=true CI_TOKEN=x node scripts/start.js --version→ warning lists onlyCI;CI_TOKENpreservedCI=0 node scripts/start.js --version→ no warning (matchesis-in-cifalsy rule)CONTINUOUS_INTEGRATION=1 node scripts/start.js --version→ warning listsCONTINUOUS_INTEGRATIONCI/CONTINUOUS_INTEGRATIONset → no warning printednode -c scripts/start.js— syntax OKnpx eslint scripts/start.js --max-warnings 0— cleanNote: I did not add an automated test because
scripts/start.jsis a small integration-flavored launcher and exercising it end-to-end would require driving a real interactive Node child. Happy to extract the filter into a tiny pure helper with a unit test if reviewers prefer.Fixes #22452