Skip to content

fix(cli): verify each system dependency install step before the next - #568

Closed
ihubanov wants to merge 5 commits into
Panniantong:mainfrom
ihubanov:fix/apt-source-safety-and-ci-lint
Closed

fix(cli): verify each system dependency install step before the next#568
ihubanov wants to merge 5 commits into
Panniantong:mainfrom
ihubanov:fix/apt-source-safety-and-ci-lint

Conversation

@ihubanov

@ihubanov ihubanov commented Aug 4, 2026

Copy link
Copy Markdown

Summary

  • verify every step of the gh CLI and Node.js install instead of ignoring exit codes
  • fetch the GitHub apt keyring to a sibling temp file and rename it into place
  • write /etc/apt/sources.list.d/github-cli.list only after that keyring exists, and roll it back if apt-get update then fails
  • reject an empty NodeSource setup script and remove it in a finally
  • require root and an apt-based system before touching /etc
  • check brew install exit codes, and route macOS through Homebrew instead of the apt path
  • move the keyring and source-list paths to module constants so the failure modes are testable
  • drop the unreachable Rich ImportError fallback in doctor
  • enforce ruff and mypy in CI, and clear the violations that had accumulated

Why

The gh CLI branch wrote an apt source whose signed-by= pointed at a keyring downloaded by an unchecked curl. When that download failed — offline, proxy, transient 404 — the machine was left with a source referencing a keyring that does not exist, which breaks apt-get update system-wide and not only for Agent Reach. except Exception then reported it as a cosmetic "gh CLI install failed", so the breakage surfaced long after the installer exited and with no indication of the cause.

The same pattern affected Node.js: bash exits 0 on an empty file, so a failed script download read as "setup succeeded" right up until apt-get found no package. That temp file also leaked whenever bash raised, because the os.unlink was not in a finally.

Non-root runs previously turned every step into a PermissionError folded into the same generic message, and non-apt systems (Fedora, Arch) had an /etc/apt source written for them regardless.

macOS had the same unchecked-exit-code problem in the Homebrew branch, and the Node.js branch was not platform-gated at all — a Mac without Node.js ran the NodeSource script and apt-get, then advised apt install nodejs npm. Node.js now installs via brew on Darwin, and the guidance matches the platform.

ruff and mypy were declared in [dev] and configured in pyproject.toml, but the workflow only ran pytest, so 20 ruff violations and one mypy error accumulated unnoticed. That error was in _cmd_doctor, where rprint was bound to Rich's print by the import and then reassigned to the builtin. rich is a hard dependency and doctor.py imports rich.markup at module scope, so the fallback could never have run — the import is now unconditional rather than guarded.

Validation

  • 444 passed on the PR head (428 existing + 16 new)
  • ruff check . — clean
  • mypy agent_reach — clean, 35 source files
  • new apt regression tests cover: failed keyring download, zero-byte keyring, apt-get update rollback, preservation of a pre-existing keyring, the happy path, non-root refusal, non-apt refusal, and both NodeSource failure modes
  • new Homebrew tests cover: non-zero brew install, timeout, missing brew, formula pass-through, and that the Darwin path never invokes apt-get, curl, bash, or dpkg
  • each failure-path test asserts the forbidden next step never runs
  • existing _install_system_deps and _cmd_doctor tests unchanged and passing
  • agent-reach doctor run against a real environment renders the report with markup applied

Known boundaries

  • Windows still has no automatic install path for either dependency; it prints guidance only, as before.

@Panniantong

Copy link
Copy Markdown
Owner

感谢推动安装器 fail-fast。merged #577 已在默认只读安装基础上统一检查核心依赖、可选渠道和 Skill 安装结果,并以非零状态传播失败,且有干净环境和 CI 验证,因此关闭此已取代大范围实现。

@Panniantong Panniantong closed this Aug 6, 2026
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