Repository navigation
fix(install): put ant on PATH from install.sh, as install.ps1 does (#194) - #223
Nic-dorman wants to merge 7 commits into
Conversation
) After a successful install.sh run, `ant --version` failed with command not found on a fresh shell: the install dir was not on PATH (macOS's default /usr/local/bin is root-owned without Homebrew, and the ~/.local/bin workaround is not on macOS's default PATH), and the script only printed the export line. install.ps1 already adds its dir to the Windows user PATH. install.sh now: - defaults to ~/.local/bin on macOS as well as Linux, so it never needs sudo; - when INSTALL_DIR is not already on PATH, appends one guarded line to ~/.profile, the user's existing bash startup files (creating ~/.bashrc when bash is the login shell), ~/.zshenv when zsh is installed, and fish's conf.d when fish is (the rustup set), listing each file it changed. Re-runs add nothing; unwritable files are reported, not fatal; - honours --no-modify-path / ANT_NO_MODIFY_PATH=1 to keep the old print-the-line behaviour (install.ps1 honours the env var too); - notes when another ant is already on PATH, e.g. an earlier macOS install in /usr/local/bin that path_helper would put first. install-test.yml gains a fresh-shell check on Linux and macOS (stock /bin/bash 3.2, zsh login and non-login). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cOS login shells The macOS install-test run showed zsh login shells resolving `ant` to the runner image's Homebrew Apache Ant: /etc/zprofile's path_helper and `brew shellenv` run after ~/.zshenv and put /opt/homebrew/bin ahead of ~/.local/bin. Forcing ~/.local/bin to the front would change precedence for every tool in it, so the installer keeps appending and instead says, when another `ant` is on PATH, that it may be an unrelated program and how to get past it. The macOS check asserts ~/.local/bin is on PATH in login shells, and that `zsh -c` resolves this ant. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dd7fbf3 to
e4aaa87
Compare
MaidSafe-Hermes
left a comment
MaidSafe-Hermes
left a comment
There was a problem hiding this comment.
2/2 independent seats reviewed this head: sol-1 (gpt-6.1-sol, xhigh) returned CHANGES_REQUIRED; glm-1 (z-ai/glm-5.3) returned APPROVE. I adjudicated the disagreement against the source and reproduced both issues below, so this is REQUEST_CHANGES. GitHub checks: 18/18 successful. Local verification: bash -n passes; isolated probes reproduce the startup-file command execution and the relative INSTALL_DIR failure with CDPATH=.; the latter also fails in a mocked end-to-end installer run. These are installer regressions not covered by the ordinary-path CI matrix.
| # Guarded, so a shell that reads several of these files (Ubuntu's ~/.profile sources ~/.bashrc) | ||
| # still ends up with one entry. | ||
| # shellcheck disable=SC2016 # $PATH is meant to reach the startup file unexpanded. | ||
| posix_line="$(printf 'case ":${PATH}:" in *":%s:"*) ;; *) export PATH="%s:$PATH" ;; esac' "$dir" "$dir")" |
There was a problem hiding this comment.
[P2] Escape INSTALL_DIR before writing shell startup code. This formats the value directly into POSIX and fish source. If a valid override contains shell syntax, e.g. $(touch${IFS}$HOME/pwned), that text is executed when the generated .profile is sourced; I reproduced the file creation. Quote/escape the directory as a literal separately for POSIX and fish (or reject unsupported characters), and add a regression test for dollar signs, quotes, backticks, and backslashes.
| # Install binary | ||
| mkdir -p "$INSTALL_DIR" | ||
| # Absolute, so the PATH entry written below does not depend on where the installer was run from. | ||
| INSTALL_DIR="$(cd "$INSTALL_DIR" && pwd)" |
There was a problem hiding this comment.
[P2] Clear CDPATH when resolving a relative INSTALL_DIR. With CDPATH=. and INSTALL_DIR=bin, this cd prints ./bin to stdout; command substitution captures that plus pwd, so INSTALL_DIR contains two newline-separated paths and the following cp fails. I reproduced this in a mocked installer run. Use a CDPATH-cleared cd and add a test for a relative override with CDPATH set.
… relative one
The PATH lines written to the shell startup files formatted INSTALL_DIR
straight into shell code, so a directory name containing $(...),
backticks or quotes ran as code whenever a new shell started. Everything
after the literal $HOME prefix is now escaped for double quotes: \ " $
and ` for POSIX shells, and \ " and $ for fish. The escaping uses sed,
because bash's ${var//...} replacement quoting differs between 3.2 and
5.2. The `export PATH=...` line printed for the current shell is escaped
the same way. A directory name with a newline leaves the startup files
alone and says so.
Making a relative INSTALL_DIR absolute ran a plain `cd`, which with
CDPATH set prints the directory too, so INSTALL_DIR held two lines and
the copy failed. It now clears CDPATH, and `cd` and `mkdir` take `--`.
install-test gains regression steps:
- Linux and macOS: an install directory named with $(...), backticks,
quotes and a backslash must resolve literally in each new shell,
including dash, zsh and fish on Linux, and from the printed line,
without running anything.
- Linux: a relative INSTALL_DIR with CDPATH=. must install to the right
absolute path.
The shell lookups in these steps report a failure instead of exiting
silently.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… on PATH Escaping INSTALL_DIR in the startup lines (3211965) isn't enough on macOS. Login shells there run `eval $(path_helper -s)` from /etc/zprofile and /etc/profile, and path_helper prints every PATH entry inside double quotes without escaping. So a correctly written entry containing $(...) still runs, or is mangled, in every new login shell. The macOS install-test run showed it: zsh -lc had `.../odd quoted back\slash` on PATH, with the substitutions run and the quotes stripped. A directory whose name contains $, `, ", \ or a newline is now left out of every startup file. The installer prints a warning, plus an escaped `export PATH=` line for the current shell only. Every remaining character is literal inside the double quotes of the POSIX and fish lines, so the per-shell escaping is gone. The printed lines stay escaped. install-test now checks, on Linux and macOS: - such a name is refused, no startup file is written, the printed line resolves, and nothing runs, including through a macOS login shell; - a name with spaces, ', ( ), &, # and * is still added and resolves in every shell, through path_helper on macOS. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Both findings are addressed, in 3211965 and c8feea5. P2: escape INSTALL_DIR before writing shell startup code (install.sh:143). I went with your alternative: such names are refused rather than escaped. The first fix, 3211965, escaped the directory per syntax:
So c8feea5 refuses to add any directory whose name contains
Every other character is literal inside the double quotes of the POSIX and fish lines, so the per-syntax escaping was removed. P2: clear CDPATH when resolving a relative INSTALL_DIR (install.sh:331). Fixed: Regression tests in
Verified on dev2 in containers, before and after:
Checks that pass on both: a default install, a re-run leaving one block per file, a literal The new Linux steps' own
install-test on c8feea5 (npm 0.3.12): run 40514011527.
CI on c8feea5: all jobs pass except Security Audit. It fails only on RUSTSEC-2026-0387 (crypto-bigint 0.5.5), which is inherited from |
MaidSafe-Hermes
left a comment
MaidSafe-Hermes
left a comment
There was a problem hiding this comment.
Focused re-review of the two prior installer blockers (not a fresh full review). 2/2 seats reviewed this delta: sol-1 (gpt-6.1-sol, xhigh) and glm-1 (z-ai/glm-5.3); both found the prior command-execution and CDPATH issues fixed, and both found the remaining colon-in-PATH defect. I also reproduced the trailing-newline normalization defect below. I am requesting changes for these two remaining installer bugs. The Security Audit is failing at this head: cargo-audit reports crypto-bigint 0.5.5 / RUSTSEC-2026-0387 (timing variability in intended constant-time operations; advisory says upgrade to >=0.7.6). Cargo.lock is byte-identical to the PR base, so this failure is inherited, not introduced by this installer delta. Per the request, do not merge until Security Audit is green. Validation: bash -n passes; mocked installer probes confirm relative INSTALL_DIR works with CDPATH=. and confirm a trailing-newline INSTALL_DIR installs into the stripped, unintended directory. The focused install.sh Linux/macOS and install.ps1 Windows jobs pass; the separate npm Linux jobs fail on the existing Apache Ant uninstall check and bootstrap-peer log assertion.
| # the line, which is checked for line by line. | ||
| unsafe_on_path() { | ||
| case "$1" in | ||
| *'$'* | *'`'* | *'"'* | *\\* | *$'\n'*) return 0 ;; |
There was a problem hiding this comment.
[P2] Reject colon-containing INSTALL_DIR values before adding them to PATH. PATH is colon-delimited, but unsafe_on_path accepts colon. A value ending in colon writes an empty PATH component (which searches the current directory); an embedded colon adds an unintended second component. I reproduced the generated startup line. Refuse this value for both startup edits and the printed export, and add a regression test for a colon-containing path (including fish, where it can be re-added on each startup).
| mkdir -p -- "$INSTALL_DIR" | ||
| # Absolute, so the PATH entry written below does not depend on where the installer was run from. | ||
| # CDPATH cleared, or `cd` into a relative directory found through it prints that directory too. | ||
| INSTALL_DIR="$(CDPATH='' cd -- "$INSTALL_DIR" && pwd)" |
There was a problem hiding this comment.
[P2] Check for newline before capturing pwd, or preserve it through normalization. Bash command substitution strips trailing newline bytes. With INSTALL_DIR ending in a newline, mkdir creates that requested directory, but this assignment loses the newline; if the stripped path already exists, the installer exits successfully after copying ant into that different directory. I reproduced this with a mocked installer run. Reject the raw input before normalization (or use a sentinel and then reject), and add a trailing-newline regression test.
…e up front PATH is colon-separated, so a directory with `:` in its name can't be one of its entries. A trailing `:` added an empty entry, which searches the current directory, and one in the middle added a second, different directory; in fish the line could re-add itself on every start. Such a directory is now left off PATH, with or without --no-modify-path. The installer says why and suggests running ant by its full path, and prints no `export PATH=` line. An INSTALL_DIR containing a newline is now rejected before anything is downloaded or created. Making the path absolute with `$(... pwd)` stripped a trailing newline, so ant could be copied into the directory without it and the install still reported success. install-test's Linux job gains a step covering a colon in the middle, a trailing colon and a colon with ANT_NO_MODIFY_PATH=1. Each must be refused, with no startup file written (fish's included) and no export line printed. The step also checks that an INSTALL_DIR ending in a newline, whose stripped directory exists, fails and installs nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Both findings are addressed in e1648ce. P2: reject colon-containing INSTALL_DIR values before adding them to PATH (install.sh:95). Fixed. A directory with
P2: check for a newline before capturing pwd (install.sh:360). Fixed. An Regression tests: a new step in
Verified on dev2 in containers (the same harness as last round, with the new cases added):
All of last round's checks still pass on both: odd names refused, punctuation names added, CDPATH, default install, re-run. I also ran the new Linux step's own install-test on e1648ce (npm 0.3.12): run 40531438944. CI on e1648ce: all jobs pass except Security Audit. It fails only on RUSTSEC-2026-0387 (crypto-bigint 0.5.5), which is inherited from |
MaidSafe-Hermes
left a comment
MaidSafe-Hermes
left a comment
There was a problem hiding this comment.
Focused re-review of PR #223 at e1648ce25f0e37d81969d0874bb5343bdc5cc510. 2/2 seats reviewed this exact delta: sol-1 (gpt-6.1-sol, xhigh) and glm-1 (z-ai/glm-5.3). Both found the remaining newline-path defect below. The colon-in-PATH findings from the prior review are fixed in both PATH-modification modes.
P2 — reject newline in the effective install path before normalizing it (install.sh:359-360, install.sh:385). The new guard only checks the supplied INSTALL_DIR, before relative paths are resolved. A relative value such as INSTALL_DIR=. passes when the current working directory's name ends in a newline. The $(... pwd) command substitution at line 385 strips that trailing newline and silently changes the destination. I reproduced this: from /.../work , the exact normalization resolves to /.../work; if that sibling exists, the installer can overwrite its ant binary. Relative paths from a working directory containing an embedded newline also bypass the guard and can write a malformed multi-line PATH entry. Validate the effective absolute path without losing newline characters before installing or writing startup files, and add regressions for both cwd cases.
Checks: 17/18 current checks pass; Security Audit fails. The PR's Cargo.lock is byte-identical to its base (ccd4106...), so that audit failure is inherited, not introduced here. Local bash -n passes. ShellCheck was not run because it is not installed in this environment.
…t INSTALL_DIR The newline check looked only at INSTALL_DIR as given. A relative one was resolved later against the current directory, whose own name could hold the newline: - With INSTALL_DIR=. run from a directory whose name ends in a newline, `$(cd ... && pwd)` stripped it, and ant went into the sibling without it, overwriting any ant there. - A newline in the middle of the current directory's path reached the PATH lines. The check now runs on the effective path before anything is downloaded or created: INSTALL_DIR itself if absolute, otherwise "$PWD/$INSTALL_DIR". $PWD keeps trailing newlines, where `$(pwd)` doesn't. Making the path absolute is also lossless now: `pwd && printf x`, then exactly "\nx" is cut. Even an unforeseen route can't change the destination silently. install-test's colon/newline step gains two cases. INSTALL_DIR=. is run from a directory ending in a newline with its sibling present, and INSTALL_DIR=bin from one with a newline in the middle. Each must fail and install nothing in either place. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed in 65a1f87. P2: reject a newline in the effective install path before normalizing it (install.sh:359-360, 385). You're right: the check only covered
Regression tests: the Linux
Each must fail, and nothing may be installed in Verified on dev2 in containers, with the same harness plus these two cases:
Every earlier check still passes on both, including the default install and re-run, which go through the new The new step's own install-test on 65a1f87 (npm 0.3.12): run 40659873563. CI on 65a1f87: all jobs pass except Security Audit. It fails only on RUSTSEC-2026-0387 (crypto-bigint 0.5.5), which is inherited from |
MaidSafe-Hermes
left a comment
MaidSafe-Hermes
left a comment
There was a problem hiding this comment.
APPROVE — focused re-review of the changes since e1648ce, at head 65a1f87. The newline-path fix rejects relative install paths from cwd names containing newlines before download or creation; I verified both trailing-newline and embedded-newline cases against this head, and bash -n passes. No new defect found in this delta.
2/2 independent seats reviewed this same evidence and head (sol-1 gpt-6.1-sol xhigh; glm-1 z-ai/glm-5.3); both recommend APPROVE.
Merge condition: DO NOT MERGE until the Security Audit passes on the current PR head. It is currently failing (crypto-bigint 0.5.5 / RUSTSEC-2026-0387). Cargo.lock is unchanged from base, so this is inherited and not introduced by this installer delta. This approval is not a waiver of that audit gate.
#238 rotated the old [Unreleased] into the 0.3.7 to 0.3.12 sections, so this PR's entry moves to the new, empty [Unreleased], under the same heading it had before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #194: after a successful
install.shrun,ant --versionfailed with command-not-found in a new terminal. The install directory wasn't onPATH, and on macOS the default/usr/local/binis root-owned without Homebrew, so the install failed withoutsudo.install.ps1already adds its directory to the Windows userPATH. This bringsinstall.shto parity.What changes
Default dir:
~/.local/binon macOS as well as Linux, so the install never needssudo.PATH: when
INSTALL_DIRisn't already onPATH, one guarded line (case ":${PATH}:" in *":$HOME/.local/bin:"*) ;; *) export PATH=… ;; esac) is appended to the same set of files rustup uses, and each changed file is listed:~/.profile(every POSIX login shell)~/.bash_profile/~/.bash_login/~/.bashrcalready exist (~/.bashrcis created if bash is the login shell)~/.zshenvwhen zsh is installed (read by every zsh, interactive or not, so this covers every Mac)conf.d/ant.fishwhen fish is installedRe-runs add nothing. An unwritable file is reported, not fatal.
Opt-out:
--no-modify-path(curl … | bash -s -- --no-modify-path) orANT_NO_MODIFY_PATH=1gives the old print-the-line behaviour.install.ps1honours the env var too.Shadowing note: if another
antis already onPATH, the installer says so. It could be an earlier macOS install in/usr/local/bin, or an unrelated program: Apache Ant is also calledant, and both GitHub runner images ship it. macOS zsh login shells runpath_helperandbrew shellenvafter~/.zshenv, so/usr/local/binand/opt/homebrew/binend up ahead of~/.local/bin. I deliberately didn't force~/.local/binto the front, since that would change precedence for every tool in it.The issue proposed modifying
PATHopt-in. I made it default-on with an opt-out instead, likeinstall.ps1and rustup in non-interactive mode. Acurl | bashscript can't prompt (stdin is the script), and an opt-in would leave the agent flow (install, thenant --version) broken by default.Linear issue
Closes V2-1506
Risk tier
Compatibility
INSTALL_DIRmoves from/usr/local/binto~/.local/bin, andinstall.shnow writes shell startup files unless--no-modify-path/ANT_NO_MODIFY_PATH=1is set. No binary orant-corechange.Semver impact
Test evidence
Review fix
65a1f87(2026-10-28): a newline in the effective install path. A relativeINSTALL_DIRis checked as"$PWD/$INSTALL_DIR"before anything is downloaded or created, so a newline in the current directory's name is rejected too. The conversion to an absolute path is also lossless now, usingpwd && printf xand then cutting exactlyx.install-test.ymlon65a1f87with 0.3.12: run 40659873563. The three installer jobs pass. The Linux step rejects all three newline cases: one inINSTALL_DIR, a cwd ending in a newline withINSTALL_DIR=., and a cwd with an embedded newline withINSTALL_DIR=bin.e1648ce→65a1f87: Ubuntu 24.04 went from 7 of 52 checks failing to 52/52 passing, andbash:3.2from 7 of 48 failing to 48/48 passing. The 7 are the new cwd cases, including the reported install into the sibling directory.65a1f87: everything passes except Security Audit, which is the inherited RUSTSEC-2026-0387 (V2-1743).Review fix
e1648ce(2026-10-27): colon and newline. A directory with:in its name can't be a PATH entry. It is installed, but kept out of every startup file, and noexport PATH=line is printed, with or without--no-modify-path. AnINSTALL_DIRcontaining a newline is rejected before anything is downloaded, so$(… pwd)can no longer strip a trailing one and install somewhere else.install-test.ymlone1648cewith 0.3.12: run 40531438944. The three installer jobs pass, including the new Linux step: three colon names are refused, and the newline case is rejected. The npm (Linux) jobs fail as below.c8feea5→e1648ce:bash:3.2: 13 of 41 failed before, 41/41 pass after.e1648ce: everything passes except Security Audit, which is the inherited RUSTSEC-2026-0387 (V2-1743).Review fixes
3211965+c8feea5(2026-10-27). A directory whose name contains$, a backtick,",\or a newline is no longer written into startup files. macOS login shells re-read PATH throughpath_helperunescaped, so escaping alone wasn't enough;3211965's macOS run showed the substitutions running inzsh -lc. A relativeINSTALL_DIRworks withCDPATHset.install-test.ymlonc8feea5with 0.3.12: run 40514011527. The three installer jobs pass, including the new steps:',( ),&,#and*is added and resolves in bash, dash, zsh and fish on Linux, and throughpath_helperon macOS;CDPATH=.with a relativeINSTALL_DIR.The two npm (Linux) jobs fail as before, for the reasons below; ci(install-test): make the npm jobs test npm's ant, not Apache Ant's #228 fixes both.
e4aaa87→c8feea5:bash:3.2(bash 3.2.57; busyboxshandsed): 13 of 28 failed before, 28/28 pass after.run:scripts fail one4aaa87and pass onc8feea5.shellcheckis clean, and so isactionlintwith shellcheck.Re-run on the current head
e4aaa87(2026-10-27). The evidence below the line predates the 10-23 rebase. The rebase didn't change this PR's patch in any of its five files, andmainhasn't touchedinstall.sh,install.ps1orinstall-test.ymlsince. Dispatchedinstall-test.ymlone4aaa87with version 0.3.12, the currentlatest: run 40507506042bash -lcandbash -icresolve/home/runner/.local/bin/ant, andant --versionprintsant 0.3.12. The shadowing note names the image's/usr/bin/ant, and theANT_NO_MODIFY_PATH=1step prints the old not-in-PATH warning./bin/bashruns the installer): the block lands in.profileand.zshenv, andzsh -cresolves/Users/runner/.local/bin/ant.~/.local/binis onPATHinzsh -lc,zsh -icand/bin/bash -lc, andant --versionprintsant 0.3.12. The note names Homebrew's/opt/homebrew/bin/ant.test npm install (Linux): "ant is still on PATH after uninstall", because of the image's Apache Ant.--ignore-scripts: the binary logsusing 7 bundled bootstrap seed(s), but the job greps forembedded bootstrap peer.shellcheck(koalaman/shellcheck:stable) is clean.Ubuntu 24.04 container matrix, 37/37 pass. The installer runs as fresh users, and each check starts from a pristine
env -iPATH without~/.local/bin:bash -i,bash -l,zsh -c,fish -candsh -lall resolve~/.local/bin/ant, andant --versionprintsant 0.3.10. Exactly one block lands in.profile,.bashrc,.zshenvandant.fish, and no.bash_profileis created.zsh -c,zsh -ilcandbash -lresolve..zshenvand.profileare created,.bashrcis not, and new files have no leading blank line.--no-modify-pathandANT_NO_MODIFY_PATH=1: files untouched, old warning printed.ANT_NO_MODIFY_PATH=0still modifies..bashrcis reported under "Could not write to:" and the install still succeeds.INSTALL_DIRis written as an absolute path. A pre-existing/usr/local/bin/anttriggers the shadowing note, and there's no note otherwise. Already onPATHmeans nothing is changed or printed.bash 3.2.57 (
bash:3.2, the version stock macOScurl | bashruns): fresh, re-run, all-writes-failed and--no-modify-pathpaths all run cleanly underset -uwith empty arrays.install-test.ymlnow checks fresh shells after a default install:bash -lcandbash -ic./bin/bashruns the installer, thenzsh -c,zsh -lc(runspath_helper),zsh -icand/bin/bash -lc.The existing
INSTALL_DIRstep now setsANT_NO_MODIFY_PATH=1, which exercises the opt-out and keeps the new check meaningful. Dispatched against this branch with version 0.3.10: run 38253772727bash -lcandbash -icresolve~/.local/bin/ant, ahead of the image's Apache Ant at/usr/bin/ant, which the note flagged.zsh -cresolves it,~/.local/binis onPATHinzsh -lc,zsh -icand/bin/bash -lc, andant --versionprintsant 0.3.10.antto the runner's Homebrew Apache Ant. That's what prompted the second commit.install.sh, and this is the first timeinstall-testhas been dispatched since they were added (V2-1152).test npm install (Linux)fails "ant is still on PATH after uninstall" because of/usr/bin/ant(Apache Ant).--ignore-scriptsfails itsembedded bootstrap peerlog grep.New dependency
none
ADR
n/a
Mitigation / rollback
Revert the commit. Users who already ran the new installer keep one harmless guarded
PATHline, marked# Added by the ant installer, in their startup files.🤖 Generated with Claude Code