Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

install.sh: do not assume sudo consent when there is no terminal#7435

Open
LeoBorcherding wants to merge 9 commits into
unslothai:mainunslothai/unsloth:mainfrom
LeoBorcherding:fix-7307-p7-no-tty-sudo-installLeoBorcherding/unsloth:fix-7307-p7-no-tty-sudo-installCopy head branch name to clipboard
Open

install.sh: do not assume sudo consent when there is no terminal#7435
LeoBorcherding wants to merge 9 commits into
unslothai:mainunslothai/unsloth:mainfrom
LeoBorcherding:fix-7307-p7-no-tty-sudo-installLeoBorcherding/unsloth:fix-7307-p7-no-tty-sudo-installCopy head branch name to clipboard

Conversation

@LeoBorcherding

Copy link
Copy Markdown
Collaborator

Addresses Problem 7 of #7307.

Problem

_smart_apt_install() in install.sh prints a consent prompt, then decides the
answer itself when there is no terminal:

printf "    Accept? [Y/n] "
if [ -r /dev/tty ]; then
    read -r REPLY </dev/tty || REPLY="y"
else
    REPLY="y"
fi
case "$REPLY" in
    [nN]*) ... exit 1 ;;
    *)
        sudo apt-get update -y </dev/null
        sudo apt-get install -y $_STILL_MISSING </dev/null
        ;;
esac

Two things go wrong when /dev/tty is unreadable:

  1. It assumes consent for a sudo escalation nobody approved.
  2. It then runs sudo with stdin redirected from /dev/null, so on any host
    where sudo needs a password, sudo cannot prompt and dies with its own raw
    error.

The result is the worst of both paths. The script neither degrades gracefully nor
explains itself, even though the no-sudo branch a few lines below already prints
exactly the message the user needs. This hits containers, CI, and locked-down
corporate machines. It also prints Accept? [Y/n] into logs where nothing can
answer it.

Fix

Probe with sudo -n true, which succeeds only when no password is required.

  • No terminal and sudo needs a password: exit with the missing packages and
    the exact command to run, mirroring the existing no-sudo message, instead of
    invoking sudo and letting it fail.
  • No terminal but sudo is passwordless: still escalate. This is the one case
    where unattended escalation is legitimate, and it now logs why it proceeded.
  • Readable /dev/tty: unchanged. The prompt is simply moved inside the
    branch so it only prints when something can answer it.

TAURI_MODE already had the right shape for this (tauri_log "NEED_SUDO"; exit 2,
letting the Rust caller handle elevation). This gives the headless shell path
equivalent honesty.

Tests

Extended tests/sh/test_apt_distro_prompt.sh, which already covers this function.
It drives the real _smart_apt_install across all four TTY/sudo combinations,
rewriting /dev/tty to a fixture path the same way the existing cases rewrite
/etc/os-release, with sudo, apt-get, command -v and _is_pkg_installed
stubbed. 20 assertions pass.

Against the previous install.sh, five of the new assertions fail. The captured
output is the bug itself:

    Accept? [Y/n] SUDO_RAN: apt-get update -y
SUDO_RAN: apt-get install -y cmake
EXIT:0

An unanswerable prompt, then escalation despite sudo -n reporting that a
password is required. The tty present and no sudo binary assertions pass both
before and after, confirming those paths are untouched.

This file was not in studio-backend-ci.yml's shell-test allowlist, so it never
ran in CI. Registered it there, since the coverage is otherwise inert. It is
hermetic: it stubs every external command and only reads install.sh via sed.

…lothai#7307 P7)

_smart_apt_install printed an "Accept? [Y/n]" prompt, and when /dev/tty was
unreadable it set REPLY=y and escalated anyway. Every sudo call in that branch
redirects stdin from /dev/null, so on any host where sudo needs a password the
install died on sudo's own error rather than the actionable message the no-sudo
path already prints. Containers, CI and locked-down corporate machines hit this.

Probe with `sudo -n true` first. If there is no terminal to prompt on and sudo
would need a password, exit with the missing packages and the exact command to
run, matching the no-sudo path. Passwordless sudo still escalates unattended,
which is the one case where that is legitimate, and says so in the log.

With a readable /dev/tty the behaviour is unchanged, and the prompt now only
prints when something can actually answer it.

Extend tests/sh/test_apt_distro_prompt.sh to drive the real function across all
four TTY/sudo combinations, rewriting /dev/tty to a fixture path the same way
the existing cases rewrite /etc/os-release. Against the old install.sh five of
these assertions fail. Register the file in studio-backend-ci.yml's shell suite,
which did not run it before.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 66bb45dba9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread install.sh Outdated
Comment thread install.sh Outdated
…7307)

Codex review follow-ups on the no-TTY sudo escalation guard.

`test -r /dev/tty` only reads the device node's permission bits. Inside
containers and systemd units those bits look fine while open() fails with
ENXIO, so the guard still fell through to a prompt nobody could answer.
_can_read_tty() does a real open. The subshell is load-bearing: in dash a
failed redirection on the special builtin `:` exits the script.

`sudo -n true` proves only that `true` is allowed. Under a command-specific
rule like `NOPASSWD: /usr/bin/apt-get` it is the wrong question in both
directions. _sudo_runs_unattended() asks the sudoers policy about the exact
argument vectors we are about to elevate, via `sudo -n -l --`, which checks
without running and fails instead of prompting.

Tests cover both: a NOPASSWD-on-trivia-but-not-apt-get sudoers stub, and a
readable-but-unopenable /dev/tty faked with a unix socket (skipped where the
platform cannot produce that shape).
@LeoBorcherding

Copy link
Copy Markdown
Collaborator Author

Addressed both review points in e5a4b4d2b, and merged current main in (b70f90079), so this is up to date with base.

P1, tty probe. Replaced test -r /dev/tty with _can_read_tty(), which does a real open (( : </dev/tty )). The subshell is deliberate: in dash a failed redirection on the special builtin : exits the script, so the probe has to be isolated. Both call sites now use it, the escalation guard and the prompt.

P2, sudo probe. Replaced sudo -n true with _sudo_runs_unattended(), which runs sudo -n -l -- <cmd> <args> against the exact argument vectors that get elevated (apt-get update -y and apt-get install -y $_STILL_MISSING). That asks the sudoers policy without executing anything and fails rather than prompting, so a command-specific NOPASSWD: /usr/bin/apt-get rule is read correctly in both directions. If listpw makes sudo -l itself require auth, the probe fails closed and we print the actionable manual command, which is the safe direction.

Tests in tests/sh/test_apt_distro_prompt.sh cover both: a sudoers stub that is NOPASSWD on trivial commands but not on apt-get, and a readable-but-unopenable /dev/tty faked with a unix socket (open() returns ENXIO, same as the container case). The socket case self-skips where the platform cannot produce that shape. 22 passed, 0 failed locally.

The earlier Repo tests (CPU) failure was pre-existing and unrelated: the previous base commit dffea2af2 fails the same job on main with the identical 12 errors, all downstream of bitsandbytes failing to load libhipblas.so.2 inside unsloth_zoo. No Python is touched by this diff.

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b70f900791

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread install.sh Outdated
Comment on lines +672 to +673
_sudo_runs_unattended() {
sudo -n -l -- "$@" >/dev/null 2>&1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Probe authentication instead of command authorization

On a no-TTY host where apt-get is permitted but still has the PASSWD tag, sudo -n -l -- apt-get ... can return 0 even though executing the command noninteractively fails: list mode checks whether the command is authorized, not whether its execution is passwordless. This matches sudo --help, which describes -l as “list user's privileges or check a specific command” and -n only as disabling prompts; with a sudoers entry using Options: authenticate, I confirmed the list command returned 0 while sudo -n apt-get ... returned 1. Consequently this helper sets _sudo_unattended=true, and the later sudo calls with /dev/null still produce the raw authentication failure this change intends to prevent. Determine whether the matched entry has !authenticate rather than relying on the sudo -l exit status, and make the stub reproduce real list-mode semantics.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correct, and fixed in 774ff5d. The list-mode probe is gone; the only remaining mention of it is the comment at install.sh:724 recording why it was abandoned. Head asks the question by running the real commands with -n.

Your reading matches sudo(8), which defines -n as "Avoid prompting the user for input of any kind. If a password is required for the command to run, sudo will display an error message and exit", while -l only lists privileges. Authorization and authentication are separate questions, and only execution answers the second one.

Verified across the tty and sudoers matrix, including the authorized-but-PASSWD case you describe: head declines and escalates nothing.

LeoBorcherding and others added 2 commits July 25, 2026 05:25
Codex follow-up. `sudo -n -l -- apt-get ...` answers authorization, not
authentication: on a host where apt-get is permitted but still carries the
PASSWD tag, list mode exits 0 while the actual run needs a password, so the
guard reported unattended and the escalation died exactly as unslothai#7307 described.

Inferring the answer from list output means parsing for `!authenticate`, which
is human-readable text that varies by sudo version. Drop the inference. In the
no-terminal branch, run the real commands with `sudo -n`: -n never prompts, so
it cannot block on a closed stdin, and its exit status is the question we were
trying to answer. If it is refused, print the actionable manual command as
before. The terminal branch is unchanged: prompt, then plain sudo, which may
ask for a password because someone is there to type it.

The test stub now models sudo properly (-n refuses and runs nothing when a
password is needed) instead of special-casing the probe's argv.
Resolves the conflict in .github/workflows/studio-backend-ci.yml.

Main replaced the hardcoded shell-test allowlist with auto-discovery over
tests/sh/test_*.sh plus an explicit skip list, guarded by
tests/studio/test_ci_shell_suite_coverage.py. That covers what this branch's
allowlist entry was for, so the entry is dropped and main's version of the
step is taken unchanged. test_apt_distro_prompt.sh is picked up by the glob.

install.sh and tests/sh/test_apt_distro_prompt.sh are unchanged by this merge.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fdf0ff5237

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread install.sh Outdated
Comment on lines +735 to +736
echo " sudo likely needs a password here and there is no terminal to"
echo " enter one on, so this cannot be done unattended."

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Distinguish apt failures from sudo authentication failures

When sudo is passwordless but either apt-get command fails because of a bad repository, network outage, dpkg lock, or package conflict, this same branch reports that sudo likely needs a password and directs the user to rerun the commands manually. sudo --help defines -n only as “non-interactive mode, no prompts are used,” so a nonzero result can be the elevated command's own exit status rather than an authentication failure. Preserve or neutrally report the apt failure instead of diagnosing it as a missing password.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correct. Reproduced both ways: with a nonexistent package and with a simulated DNS failure, sudo authenticated fine, apt-get returned 100, and the branch still told the user that a password was probably needed.

Fixed in 33f0c4d. Your reasoning matches sudo(8), which says the exit status "is the exit status of the program that was executed" when the command runs, and 1 for an authentication or configuration failure, so the status alone does not separate the two cases. The message now reports both possibilities and points at the real error rather than guessing:

    Either sudo needs a password here, or apt-get itself
    failed; see the error above. With no terminal to
    authenticate on, this cannot be done unattended.

The underlying apt error was already reaching the user on stderr; it was only the summary line that was wrong.

Comment thread install.sh Outdated
Comment on lines +728 to +729
if sudo -n apt-get update -y </dev/null &&
sudo -n apt-get install -y $_STILL_MISSING </dev/null; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Reject cached credentials before unattended elevation

On hosts where an authentication timestamp applies globally or otherwise remains valid for this headless process, sudo -n succeeds for a PASSWD-tagged apt-get command, so a recent unrelated sudo authentication is treated as “passwordless sudo” and packages are installed without an Accept response. The new direct invocation is fresh evidence beyond the earlier authorization-probe issue: sudo --help defines -n only as “non-interactive mode, no prompts are used” and separately exposes -k to invalidate a timestamp. Require an actual NOPASSWD rule, bypass cached credentials, or require explicit unattended opt-in before executing these elevated commands.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correct, and this was reachable. Fixed in 33f0c4d by adding -k to both probes, so they succeed only under a real NOPASSWD rule:

if sudo -n -k apt-get update -y </dev/null &&
    sudo -n -k apt-get install -y $_STILL_MISSING </dev/null; then

On the side effect you would rightly worry about: per sudo(8), -k used with a command only makes sudo "ignore the user's cached credentials" for that invocation and "will not update the user's cached credentials". It does not clear the timestamp the way bare sudo -k does, so an interactive session in another terminal does not have to re-authenticate afterwards.

Reproduced before and after with a sudoers model carrying a valid timestamp for a PASSWD-tagged apt-get: previously it installed with nobody having answered the prompt, now it declines and runs nothing as root. Covered by a new cached mode in tests/sh/test_apt_distro_prompt.sh, which fails against the previous commit.

…rd for apt failures

Two review findings on the headless escalation branch.

A cached authentication timestamp from an earlier, unrelated elevation made
`-n` succeed for a PASSWD-tagged apt-get, so packages installed with nobody
having answered the prompt. Add `-k` so the probe ignores the timestamp and
only a real NOPASSWD rule counts as passwordless. Per sudo(8), `-k` alongside
a command ignores the cached credentials for that invocation and "will not
update the user's cached credentials", so an interactive session elsewhere
does not have to re-authenticate afterwards.

A nonzero status from the elevated apt-get was reported as "likely needs a
password" even when sudo had authenticated fine and apt itself failed on a bad
repository, a dpkg lock or a network outage. sudo returns the command's own
exit status when the command runs, so the two cases are not distinguishable
from the status alone. Report both possibilities and point at the real error.

tests/sh/test_apt_distro_prompt.sh: teach the sudo stub about -k, add a cached
mode, and assert both behaviours. The three new assertions fail against the
previous commit.
_can_read_tty proves the device opens, not that anyone is there to answer. A
read that hits EOF still fell back to REPLY=y and escalated, so the branch that
does have a terminal kept the behaviour this change removes from the branch
that does not. A drained or half-closed terminal reached it.

Default to n instead, which is what the post-install autostart prompt at the
bottom of this file already does on the same condition. Enter still means yes:
that is a successful read of an empty line, not a failed read.

tests/sh/test_apt_distro_prompt.sh: add an eof tty fixture, which opens
normally and returns EOF immediately. Both new assertions fail against the
previous commit.
@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 545041881d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…tus claim

Comment-only. The earlier note said a nonzero status from the elevated apt-get
was not distinguishable from the status alone; sudo(8) is more specific than
that. sudo exits 1 on an authentication or configuration failure and passes the
command's own status through when the command runs, while apt-get(8) returns
100 on error, so the two usually are distinguishable. sudo also exits 1 when
the command cannot be executed, which is why the message still states both
causes rather than naming one.
@danielhanchen

Copy link
Copy Markdown
Member

@codex review

1 similar comment
@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 3c1b676516

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment-only pass over the branch's own comments in both files. Same intent,
fewer lines: drop restatement, keep the parts a reader cannot derive from the
code (why test -r is the wrong probe, why the subshell around the redirection
is load-bearing under dash, what -k buys over -n, and why a nonzero status
does not by itself name the cause).

Verified to touch nothing but comments and blank lines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.