Skip to content

fix(build): stop piped installs from falling back to the cwd - #294

Open
arelchan wants to merge 1 commit into
mainfrom
fix/install_sh_piped_local_mode
Open

fix(build): stop piped installs from falling back to the cwd#294
arelchan wants to merge 1 commit into
mainfrom
fix/install_sh_piped_local_mode

Conversation

@arelchan

Copy link
Copy Markdown
Contributor

Summary

The one-line install silently installed a local checkout instead of the published release wheel.

install_raven picked its mode from dirname "$0". $0 names a real file only when
the installer runs as a file (./install.sh); piped through curl ... | sh the script
arrives on stdin, $0 is sh and its dirname is ., so the "is this a raven source
checkout?" test ran against the current working directory. Anyone running the documented
one-liner from inside a clone (a natural thing for a contributor to do) got a silent
editable install of that working tree: raven --version then reports whatever the
checkout is rather than the release, and the runtime reads a config that the checkout's
branch may not even support. The script prints Local raven source detected, but a user
who typed the official one-liner has no reason to read that as "I am not installing the
release".

Local mode now requires $0 to be an existing file, so a piped run always resolves the
release wheel. RAVEN_LOCAL_SRC=<dir> is the explicit opt-in for a piped run, and a value
that is not a raven checkout fails loudly rather than falling through to remote mode.

install.ps1 had the same fallback, through (Get-Location).Path when $PSScriptRoot is
empty under irm ... | iex; it is fixed the same way.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

Ran install.sh end to end against stub uv / curl / node / npm on the PATH, so the
chosen install command is observable without touching a real environment. Four cases, all
from inside a raven checkout:

Invocation Before After
cat install.sh | sh editable install of the cwd checkout tool install --force raven[channels] @ .../raven-0.1.11-py3-none-any.whl
sh ./install.sh editable install editable install (unchanged)
RAVEN_LOCAL_SRC=<checkout> piped n/a editable install of that checkout
RAVEN_LOCAL_SRC=/nonexistent-dir piped n/a x RAVEN_LOCAL_SRC is not a directory: /nonexistent-dir, exit 1
  • Relevant tests pass locally
  • Relevant lint / type checks pass locally
  • User-facing docs or screenshots are updated when needed

Notes on the unchecked box: the repo has no shell test suite and no shellcheck / shfmt hook,
so "tests" here means the stubbed end-to-end runs above; the pre-commit hooks that do exist
(ruff, prettier, eslint) do not cover .sh / .ps1. install.ps1 was reviewed by reading,
not executed, since this was verified on macOS. Both installers document RAVEN_LOCAL_SRC in
their header comment; the README one-liner is unchanged and needs no edit.

Risk

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

Notes: this narrows what a piped script will install from the machine it runs on, so it
removes a way for the installer to pick up unexpected local code. The documented developer
path (./install.sh inside a clone) is unchanged. The only behavior anyone could have relied
on is "pipe the installer while sitting in a clone and get an editable install"; that now
needs RAVEN_LOCAL_SRC=.. Rollback is a revert of this commit.

Related Issues

N/A

"$0" names a real file only when the installer runs as a file. Piped
through `curl ... | sh` the script arrives on stdin, "$0" is "sh" and its
dirname is "." -- so the local-source check ran against the current
directory. A one-line install started from inside a clone therefore
became a silent editable install of that working tree instead of the
published release wheel, leaving `raven --version` reporting whatever the
checkout happened to be and the runtime reading a config the checkout's
branch may not support.

Local mode now requires "$0" to be an existing file (./install.sh), and
RAVEN_LOCAL_SRC=<dir> is the explicit opt-in for a piped run; a value
that is not a raven checkout fails loudly instead of silently falling
through to remote mode. install.ps1 had the same fallback, through
(Get-Location).Path when $PSScriptRoot is empty under `irm ... | iex`.

Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned. One non-blocking suggestion inline.

The diagnosis is right and the fix is the correct shape: dirname "$0" under curl ... | sh really is ., so the old local-source check was reading the caller's cwd, and $PSScriptRoot-or-Get-Location had the same hole. Requiring $0 to be a real file (and $PSScriptRoot to be set) is the narrowest condition that separates the two, and RAVEN_LOCAL_SRC gives the piped case an explicit, loud opt-in.

What I verified

I extracted the new install_raven detection block verbatim into a probe script and ran every invocation shape that matters:

invocation cwd result
cat install.sh | sh / | bash / | zsh inside a raven clone remote (was local before this PR -- the bug)
./install.sh the clone local
sh install.sh the clone local
sh /abs/path/install.sh / local
bash <(cat install.sh) the clone remote ($0 is /dev/fd/N, not a regular file)
RAVEN_LOCAL_SRC=<clone> sh < install.sh / local
RAVEN_LOCAL_SRC=/etc/hosts - dies, "not a directory"
RAVEN_LOCAL_SRC=/nope/nope - dies, "not a directory"
RAVEN_LOCAL_SRC=/tmp - dies, "not a raven source checkout"

So the documented dev flow (git clone ... && cd raven && ./install.sh, the script's own header) is intact, and no invocation that used to reach local mode legitimately now falls back to remote. sh -n install.sh is clean. set -eu is safe here: the || true on the RAVEN_LOCAL_SRC cd keeps the command substitution from aborting the script, which the two failure rows above confirm.

I could not execute the PowerShell half (no pwsh on this machine), so install.ps1 is a read-only check. It reads correct and symmetric with the shell version: Test-RavenSource is guarded against a null $Dir, PowerShell's -and short-circuits so Select-String -Path is never handed a missing file, Test-RavenSource and Fail are both defined above Main's call site at the bottom of the file, and a set-but-non-source $PSScriptRoot leaves $scriptDir null and falls through to the wheel -- same as the shell.

Things I checked and am not raising

  • Docs. Nothing documented breaks. CONTRIBUTING.md sends developers to make install, which never touches these scripts; the README/CHANGELOG/release-notes one-liners are all remote installs, which is exactly the path this PR restores.
  • Comment fix. Changing "Otherwise install from git" to "Otherwise install the release wheel" corrects a stale comment -- the else branch resolves a .whl from /releases/latest and deliberately avoids a git install.
  • Tests. There are none for these scripts and none exist to weaken; tests/test_channels_manager.py only asserts on hint strings, which are untouched.
  • AGENTS.md. Branch name, Conventional-Commits header (60 chars), pure-ASCII body, and the Co-authored-by trailer all conform. commitlint.config.cjs enforces no scope enum, so fix(build) passes.
  • A contrived hole that survives. If a piped run's cwd contained a regular file literally named sh/bash and was a raven clone, [ -f "$0" ] would pass and local mode would trigger. I could not construct a realistic way for that to happen in a raven checkout, so I am not asking for it to be handled.

Comment thread install.sh
script_dir="$(CDPATH= cd -- "$(dirname -- "$0")" && pwd)"
is_raven_source "$script_dir" || script_dir=""
fi
if [ -n "$script_dir" ]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking suggestion: consider an info line when a piped run lands inside a checkout.

This flips a silent behaviour in both directions. Before, a developer standing in a clone who ran the one-liner got an editable install of their working tree; now they get the published wheel, with nothing in the output saying the choice was made or that it changed. That is the correct choice, but the surprise is symmetric to the bug you are fixing -- someone who had (knowingly or not) relied on the old behaviour gets a different raven --version and no clue why.

A single line in the remote branch, gated on is_raven_source "$PWD" being true while script_dir is empty, would cost nothing and make the escape hatch discoverable at exactly the moment it is wanted:

is_raven_source "$PWD" && info "In a raven checkout, but installing the release wheel (piped run). Set RAVEN_LOCAL_SRC=$PWD for an editable install."

is_raven_source is already a function now, so this is a one-liner. Same idea applies to install.ps1. Purely a suggestion -- merge without it if you would rather keep the remote path quiet.

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.

2 participants