Skip to content

fix: install script TTY handling on Unix#1032

Open
vladfrangu wants to merge 3 commits intomasterfrom
fix/install-script-on-unix
Open

fix: install script TTY handling on Unix#1032
vladfrangu wants to merge 3 commits intomasterfrom
fix/install-script-on-unix

Conversation

@vladfrangu
Copy link
Member

@vladfrangu vladfrangu commented Mar 2, 2026

Closes #1027

Summary

  • Fix the install shell script's TTY handling when running via curl | bash — shell-level </dev/tty redirects don't support raw mode properly for Node.js/Inquirer, so we now set APIFY_OPEN_TTY=1 and let Node.js open /dev/tty itself as a native tty.ReadStream
  • Bypass Inquirer entirely for the /dev/tty prompt path (Inquirer's internal readline cannot be closed, which hangs the process), using a lightweight single-keypress reader with proper cleanup
  • Skip adding PATH entries to shell config if already present
  • Add dev-test-install.sh for local testing of the install flow

Test plan

  • Run cat scripts/install/dev-test-install.sh | bash and verify the Y/n prompt works (accepts single keypress, clears line with answer)
  • Verify the process exits cleanly after the prompt (no extra keypress needed)
  • Run apify install directly in a terminal and verify the normal Inquirer prompt still works
  • Re-run the install and verify it skips adding PATH entries if already configured

🤖 Generated with Claude Code

@github-actions github-actions bot added this to the 135th sprint - Tooling team milestone Mar 2, 2026
@github-actions github-actions bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Mar 2, 2026
@B4nan
Copy link
Member

B4nan commented Mar 5, 2026

Claude review from my end here, it sounds pretty fine, I would include it in v1.3, but please take a look at those points.

  Overall Assessment

  Good PR that solves a real UX problem — the curl | bash install flow was broken because stdin is consumed by the pipe, making Inquirer's prompt non-functional. The approach of signaling via APIFY_OPEN_TTY=1 and opening /dev/tty directly is the correct
  pattern for this.

  That said, I have several concerns ranging from a potential bug to design issues:

  ---
  Issues

  1. 🔴 Module-level process.env.HOME! — side effect at import time

  const defaultInstallDir = process.env.APIFY_CLI_INSTALL ?? join(process.env.HOME!, '.apify');
  const defaultBinDir = process.env.FINAL_BIN_DIR ?? join(defaultInstallDir, 'bin');

  These run at import time — and _register.ts imports InstallCommand unconditionally for every CLI invocation. If HOME is ever unset (e.g., in a container or CI), path.join(undefined, '.apify') will throw and crash the entire CLI, not just the install
  command.

  Fix: Move these inside promptAddToShell() as local variables, or make them lazy getters.

  2. 🔴 PR claims "skip adding PATH entries if already present" but there's no deduplication logic

  The description says:
  Skip adding PATH entries to shell config if already present

  But the code never checks oldContent for existing entries. The only guard is the install marker, which is existing behavior. If a user deletes .install-marker and re-runs, or if the config already has the entries from a manual addition, the lines will
  be appended again.

  Suggestion: Add a simple check:
  if (oldContent.includes('APIFY_CLI_INSTALL')) {
      info({ message: chalk.gray(`PATH entries already present in ${tildify(configFile)}`) });
  } else {
      // append
  }

  3. 🟡 File descriptor leak if new ReadStream(fd) throws

  In confirmFromTty:
  const fd = openSync('/dev/tty', 'r');
  ttyStream = new ReadStream(fd);

  If new ReadStream(fd) throws, fd is leaked — the finally block only destroys the stream (which was never assigned). The raw fd should be tracked separately and closed in the finally.

  } finally {
      if (ttyStream) {
          ttyStream.setRawMode(false);
          ttyStream.pause();
          ttyStream.destroy();
      } else if (fd !== undefined) {
          closeSync(fd);
      }
  }

  4. 🟡 readFile .catch(() => '') silently swallows permission errors

  const oldContent = await readFile(configFile, 'utf-8').catch(() => '');

  This is fine for ENOENT (file doesn't exist), but it also swallows EACCES and other errors. If the file exists but isn't readable, the code will silently overwrite it with just the new lines. Consider catching only ENOENT:

  const oldContent = await readFile(configFile, 'utf-8').catch((e) => {
      if (e.code === 'ENOENT') return '';
      throw e;
  });

  5. 🟡 The if (!installDir) guard is now dead code

  const installDir = process.env.PROVIDED_INSTALL_DIR ?? defaultInstallDir;

  if (!installDir) {
      warning({ message: chalk.gray(`Install directory not found, cannot add to shell`) });
      return;
  }

  Since defaultInstallDir is always a non-empty string (assuming HOME is set), installDir can never be falsy. The guard is now unreachable. Either remove it or restructure the fallback logic so the guard still serves a purpose.

  6. 🟢 Minor: allowedToAutomaticallyDo mutated for unknown shell

  default:
      configFile = '~/.bashrc';
      // But never automatically add to the file as we don't know the shell
      allowedToAutomaticallyDo = false;
      break;

  This is a good defensive change — ~/.bashrc is a literal tilde string, not a resolved path, so writeFile would fail or write to the wrong place. Worth a comment clarifying that configFile here is display-only.

  7. 🟢 dev-test-install.sh

  Fine to commit for developer convenience, but consider adding a note in the file or README that it's for local development only — it runs yarn and yarn build-bundles and assumes you're at the repo root. It could confuse someone who stumbles on it
  expecting a production install script.

  ---
  Shell script changes look correct

  The TTY detection in install.sh is the standard pattern:
  if ! [ -t 0 ] && [ -r /dev/tty ]; then
      APIFY_OPEN_TTY=1 ...

  Moving GITHUB/github_repo after the AVX2 block is a harmless reorder (no dependency on target).

  ---
  Summary

  The core fix (custom TTY keypress reader bypassing Inquirer) is sound and well-implemented. The main blockers are the module-level side effect (#1) and the missing deduplication logic that the PR claims to have (#2). The fd leak (#3) and overly broad
  catch (#4) are worth fixing but less critical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-tooling Issues with this label are in the ownership of the tooling team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Installation using scripts requires fiddling with env vars

2 participants