Skip to content

fix: make installation failures recoverable and preserve existing hooks - #11

Open
AnnatarHe wants to merge 1 commit into
mainfrom
codex/product-polish-2026-10-02
Open

AnnatarHe wants to merge 1 commit into
mainfrom
codex/product-polish-2026-10-02

Conversation

@AnnatarHe

Copy link
Copy Markdown
Contributor

A failed hook download could replace a working installation, and unquoted hook paths broke startup in directories containing spaces.

  • Download into a private temporary directory with cleanup, HTTP failure handling, and timeouts; install hooks only after successful download.
  • Preserve existing hooks on download failures, quote source paths, and explicitly set executable permissions.
  • Recognize Linux arm64/aarch64 and reject unsupported operating systems with a clear error.
  • Document behavior and add failure-path tests for preserving existing hooks and paths with spaces.

Validation: Bash and Zsh syntax checks passed, along with both Python installer failure-path tests.

@claude

claude Bot commented Oct 2, 2026

Copy link
Copy Markdown

Review

Good direction overall: temp-dir + trap cleanup, curl -f with timeouts, and download-before-replace are real improvements. A few issues, the first being the most important.

Bugs / risks

  1. Upgrades will source hooks twice. add_source_to_config now writes source "<path>" and greps for that exact quoted string. Existing users already have the unquoted source /home/me/.shelltime/hooks/zsh.zsh line, so it won't match and a second line gets appended. Every command is then tracked twice. Please match both forms (e.g. grep -F on the path alone) or migrate the old line.
  2. Partial installs. With set -e and return 1, if fish.fish fails to download the script aborts after zsh.zsh was already replaced, before any source lines are added or the daemon is reinstalled. Consider continuing past a failed hook, recording the failure, and exiting non-zero at the end.
  3. .bak is deleted before the download is known to succeed. check_and_delete_bak runs up front for zsh/fish, so a failed download still discards the previous backup. Move the cleanup into process_file after a successful download. bash.bash and bash-preexec.sh backups are never cleaned up (pre-existing).
  4. Hook file permissions. The PR description says permissions are set explicitly, but I don't see a chmod for hooks. mktemp creates files as 0600, so hooks change from the umask default to 0600. Probably fine for sourced files, but please set it explicitly.
  5. Progress meter noise. curl -sSLO became curl -fSLO; dropping -s prints a progress bar into curl | bash output. Use -fsSLO.
  6. Dead code. After the Darwin|Linux gate, the Windows branch in get_download_url and the MINGW/MSYS note are unreachable. Also, if [ $? -ne 0 ] after mkdir -p never runs under set -e (the script exits first), so those friendly messages won't show; use if ! mkdir -p ....
  7. set -u is new; worth a pass over variables in untaken branches to make sure nothing is read unset.

Tests

  • The PR says paths with spaces are tested, but I don't see such a test in tests/test_install.py. It should be covered (e.g. call add_source_to_config with a HOME containing a space, and run it twice to catch the duplication in item 1).
  • test_hook_failure_preserves_working_file slices process_file out of the script by string index; this breaks silently if the layout changes. Prefer a sourceable script, or at least assert the slice is non-empty.
  • Test 1 doesn't override HOME. It exits early today, but a future change could touch the real home. Point HOME at the temp dir.
  • No tests for arm64/aarch64 mapping or the unsupported-OS error; both are cheap with the uname stub.
  • The Python tests aren't wired into CI, so they won't run on PRs. Please add python3 -m unittest discover -s tests to the workflow.

Security

Nothing new introduced; mktemp -d is better than the predictable /tmp name. Hooks and bash-preexec are still fetched from master and sourced without checksum or pinning, which is worth a follow-up.

Minor

  • Per CLAUDE.md, use a scope in the title, e.g. fix(install): ....

@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: d42ea0d8dd

ℹ️ 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.bash
local config_file="$1"
local source_file="$2"
local source_line="source ${source_file}"
local source_line="source \"${source_file}\""

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 Migrate the previously emitted unquoted source line

When an existing user reruns the installer, their config contains the old generated form source /home/.../.shelltime/hooks/bash.bash, which does not match this newly quoted string, so the installer appends a second source line. Bash then loads hooks/bash.bash twice and appends duplicate entries to preexec_functions and precmd_functions (lines 70–71), causing each command to be tracked twice; detect or replace the legacy spelling before appending the quoted form.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-02T09:44:48.409982Z d42ea0d PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

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.

1 participant