Add interactive upgrade prompt to update check notice#232
Conversation
The "update available" notice now becomes interactive: in a TTY session it asks "Update now?" and, on yes, runs the upgrade in the foreground so the user watches the installer's own output. The upgrade command is the detected install channel (brew/pipx/uv) when known, falling back to the canonical install-script one-liner (curl -LsSf .../install.sh | sh) when the channel is unknown — which runs through `sh -c` because of the pipe. Declining, a non-interactive stdin, CI, --json, and AAI_NO_UPDATE_CHECK all suppress the prompt, and every failure is swallowed so it can never break a command. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rFXiSw66eKCHjV8DsEQ7q
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| # Generic fallback when the install channel is unknown: the canonical one-liner | ||
| # installer, which re-installs over any existing copy (it runs through a shell | ||
| # because of the pipe — see ``_upgrade_argv``). | ||
| _INSTALL_SCRIPT_COMMAND = f"curl -LsSf {_INSTALL_SCRIPT_URL} | sh" |
There was a problem hiding this comment.
Introduces a curl | sh one-liner in _INSTALL_SCRIPT_COMMAND, which fetches and executes remote code at runtime. Review for security implications before allowing execution.
Details
✨ AI Reasoning
The change introduces a module-level constant _INSTALL_SCRIPT_COMMAND that contains a curl | sh pipeline. This constructs a runtime command string that will fetch and immediately execute remote shell script content. Dynamic execution of remote scripts via shell pipelines can hide what is run until runtime and is a common vector for supply-chain or remote-code execution attacks. The pattern is deliberate and was added by the PR (new line), so it should be flagged for review.
🔧 How do I fix it?
Ensure code is transparent and not intentionally obfuscated. Avoid hiding functionality from code review. Focus on intent and deception, not specific patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| """The argv for running ``command``. The install-script fallback is a shell | ||
| pipeline (``curl … | sh``) so it runs through ``sh -c``; the package-manager | ||
| commands are plain argv split on whitespace.""" | ||
| if command == _INSTALL_SCRIPT_COMMAND: |
There was a problem hiding this comment.
Constructs shell invocation ['sh','-c', command] for pipeline commands, enabling execution of dynamically sourced shell content; ensure the command is trusted and reviewed.
Details
✨ AI Reasoning
The newly added _upgrade_argv() returns ['sh', '-c', command] when the fallback pipeline command is chosen. Wrapping a dynamic string in sh -c executes the whole string via the shell, which can obscure exact operations performed and the sources of inputs. This was added in the PR (new function and branch) and increases the potential for executing unexpected shell constructs at runtime.
🔧 How do I fix it?
Ensure code is transparent and not intentionally obfuscated. Avoid hiding functionality from code review. Focus on intent and deception, not specific patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
|
|
||
|
|
||
| def run_foreground(argv: list[str]) -> int: | ||
| """Run ``argv`` to completion in the foreground and return its exit status. | ||
|
|
||
| The opposite of ``spawn_detached``: stdio is *inherited*, so the child's output | ||
| streams straight to the terminal. Backs the interactive update prompt, where the | ||
| user watches the brew/uv/curl installer run. S603 is ignored project-wide. | ||
| """ | ||
| return subprocess.run(argv, check=False).returncode |
There was a problem hiding this comment.
Adds run_foreground() that executes arbitrary argv (including shell-executed pipeline commands). Confirm the invoked commands are safe and audited before running.
Show fix
| def run_foreground(argv: list[str]) -> int: | |
| """Run ``argv`` to completion in the foreground and return its exit status. | |
| The opposite of ``spawn_detached``: stdio is *inherited*, so the child's output | |
| streams straight to the terminal. Backs the interactive update prompt, where the | |
| user watches the brew/uv/curl installer run. S603 is ignored project-wide. | |
| """ | |
| return subprocess.run(argv, check=False).returncode |
Details
✨ AI Reasoning
The new run_foreground() wrapper calls subprocess.run(argv, check=False) with inherited stdio (as intended in description). This will execute the argv produced by _upgrade_argv, including a sh -c wrapper for a curl | sh pipeline. Although not obfuscation, it materially enables executing remote, dynamic code in the user's terminal. This change was added in the PR and increases risk of running unreviewed remote script content.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
The tests (windows, py3.13) cell flaked on two pre-existing color-depth assertions (test_list_table_colors_status, test_render_steps_colors_status) unrelated to this PR's files; the same tests pass on main at this merge-base. Re-running with a fresh pytest-randomly seed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rFXiSw66eKCHjV8DsEQ7q
Both Windows matrix cells failed at the 'System deps (ffmpeg)' step: community.chocolatey.org returned 503 Service Unavailable, so ffmpeg never installed and the cells errored before tests ran. Pure infra flake; re-kicking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rFXiSw66eKCHjV8DsEQ7q
The Windows matrix and (intermittently) the Linux suite were failing on issues
unrelated to this PR's feature code:
1. Windows "System deps (ffmpeg)" died when community.chocolatey.org returned
503s — the existing per-attempt timeout retries a *hang* but not a fast 503,
so the loop exhausts with no ffmpeg. Add a fallback that pulls a static build
off GitHub's release CDN (a different, far more reliable origin) and puts it
on GITHUB_PATH.
2. test_list_table_colors_status / test_render_steps_colors_status flaked
green/red by pytest-randomly seed (on both Linux and Windows). Root cause:
Rich caches each Style's rendered ANSI in Style._ansi on first render without
keying on the color system, and theme.THEME's Style objects are shared across
every console make_console builds — so whichever console renders an aai.*
style first pins its color depth for the rest of the process, and a later
test asserting truecolor gets a stale 16-color downgrade. Add an autouse
conftest fixture that resets the per-Style cache before each test (same
hermeticity rationale as the existing rendering fixtures; _environ={} can't
fix it since the cache isn't env-keyed).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018rFXiSw66eKCHjV8DsEQ7q
2 participants
Summary
Extends the update-available notice with an interactive prompt that offers to run the upgrade immediately, completing the user experience for in-place CLI updates.
Key Changes
_maybe_prompt_upgrade()to offer an interactive "Update now?" prompt after the update notice renders, but only when stdin is a real terminal (respects non-interactive environments like CI and piped input)resolve_upgrade_command()to return the detected package-manager command (brew/pipx/uv) when available, falling back to a canonicalcurl | shinstaller script for unknown install channels_upgrade_argv()to properly construct subprocess arguments — the install-script fallback is a pipeline so it runs throughsh -c, while package-manager commands are split on whitespaceprocs.run_foreground()to run the upgrade subprocess with inherited stdio, so the user watches the installer output in real time_report_upgrade()to display success/failure messages after the upgrade completes, including instructions to restart the CLI or re-run the command manually on failure_confirm_upgrade()treats Ctrl-C (Abort) and Ctrl-D (EOF) as "no" rather than crashing, with default-No so a bare Enter declines the upgradeImplementation Details
stdio.stdin_is_tty()returns True, ensuring non-interactive sessions (CI, piped input) skip the prompt but still show the notice_INSTALL_SCRIPT_URL,_INSTALL_SCRIPT_COMMAND) for maintainabilitycheck=Falsein subprocess.run), allowing the CLI to report the failure gracefully rather than crashing_tty_console()that mirrors the pattern fromtest_update_check.pyhttps://claude.ai/code/session_018rFXiSw66eKCHjV8DsEQ7q