From 128565a828bff94cbdabfc1f59388d6ccb516b66 Mon Sep 17 00:00:00 2001 From: Michael Sitarzewski Date: Thu, 3 Sep 2026 07:15:08 -0500 Subject: [PATCH] fix(install): refuse --path only for colliding tools; re-land installer test suite (#772) (#828) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Reapply "test(install): add a regression suite for install.sh + CI on Linux and macOS (#772)" (#827) This reverts commit 4bab3cf4a222fc51717ba884c2f91a350cc57187. * fix(install): refuse --path only for tools that would overwrite each other; re-land installer test suite (#772) Re-lands the install.sh regression suite from #772 (reverted in #827) together with the guard change that makes it pass, so CI goes green in one step. Background. #825 made --path refuse more than one --tool, on #819's report that several tools sharing one destination clobber each other. That was right in spirit and over-broad in practice, and it was implemented without verifying the premise. Measured by installing one agent with every tool into a sandbox and comparing what landed: -.md (raw copy) claude-code, copilot .md (converted) gemini-cli, opencode, qwen, zcode agency-/SKILL.md antigravity, osaurus Tools in the same group write identical filenames and silently overwrite each other (qwen + gemini-cli lose a file while both print [OK]). Tools in different groups coexist (claude-code + codex, claude-code + qwen). Every other tool's output is distinct. The guard now refuses --path only for a colliding pair, naming both tools and the reason, and allows the rest. path_collision_group() holds the measured table; re-measure if a converter's naming changes. The suite's two-tool --path cases used claude-code + copilot, which collide: the "installs exactly one agent" count of 1 was passing because copilot had overwritten claude-code's identical file. They now use claude-code + codex and assert that BOTH outputs survive, which a single count cannot show; a new case asserts the colliding pair is refused. codex has no committed output, so those cases convert (only the two raw-copiers work under --no-convert in a fresh checkout). Verified under bash 3.2 (the macOS CI leg): 29 passed, 0 failed, 1 xfail. Guard spot-checked against all three measured groups plus a cross-group pair. Suite, workflow and CONTRIBUTING note by @SergiorCode (#772). Refs #772 #819 #825 #827. Co-Authored-By: SergiorCode Co-Authored-By: Claude Fable 5.1 * fix(install): auto-convert never fired in a fresh checkout — README.md read as generated output ensure_converted decided a converted tool's output was present if its integrations// directory contained any file. Every one of those directories ships a committed README.md, so in a fresh checkout the check always found a file, skipped convert.sh, and the installer then hard-failed "integrations/ missing. Run convert.sh first." — the exact flow #426's auto-convert was added to prevent. This affected every converted tool; it was masked locally by generated outputs left behind in the working tree, and surfaced only when the test suite ran in a clean checkout. Only files other than README.md now count as generated output. Verified in a clean worktree under bash 3.2: the suite's serial-control case (claude-code + codex into one --path) now auto-converts codex and both outputs land; 29 passed, 0 failed, 1 xfail. Refs #426 Co-Authored-By: Claude Fable 5.1 --------- Co-authored-by: SergiorCode Co-authored-by: Claude Fable 5.1 --- .github/workflows/test-install.yml | 30 +++ CONTRIBUTING.md | 5 + scripts/install.sh | 39 +++- scripts/test-install.sh | 321 +++++++++++++++++++++++++++++ 4 files changed, 390 insertions(+), 5 deletions(-) create mode 100644 .github/workflows/test-install.yml create mode 100755 scripts/test-install.sh diff --git a/.github/workflows/test-install.yml b/.github/workflows/test-install.yml new file mode 100644 index 00000000..4318ac30 --- /dev/null +++ b/.github/workflows/test-install.yml @@ -0,0 +1,30 @@ +name: Test Installer + +# No path filter on purpose: the installer's contract can break from the other +# side too — a renamed division, a file that loses its frontmatter, a change to +# lib.sh — so these run on every PR. +on: + pull_request: + push: + branches: [main] + +jobs: + test-install: + name: install.sh behavior (${{ matrix.os }}) + runs-on: ${{ matrix.os }} + strategy: + fail-fast: false + matrix: + # macOS ships bash 3.2, Linux ships bash 5 — the scripts must pass on both. + os: [ubuntu-latest, macos-latest] + steps: + - uses: actions/checkout@v4 + + - name: Shell syntax + run: | + for f in scripts/*.sh; do bash -n "$f"; done + + - name: Run installer tests + run: | + chmod +x scripts/test-install.sh scripts/install.sh + ./scripts/test-install.sh diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fbecb4cf..d136a503 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -240,6 +240,11 @@ Want agency-agents to install into a new tool (a CLI, editor, or agent runtime)? 4. **`.gitignore`** — add a rule for your tool's generated output under `integrations//`. **This step is required and easy to miss.** Converted agent/skill files are generated locally by `convert.sh` and are **never committed** (see "Things we'll always close" below) — only `integrations//README.md` is tracked. Match an existing per-tool entry. 5. **`integrations//README.md`** — a short doc for the integration (every tool has one; it's the only committed file in the tool's directory). 6. **Run `./scripts/check-tools.sh`** — it must pass. It cross-checks `tools.json` against `install.sh` and `convert.sh` and flags anything missing. +7. **Run `./scripts/test-install.sh`** — it must pass. It installs into throwaway + sandboxes (never your real `$HOME`) and pins the installer's observable + contract: where files land, that `--path` beats the tool's env var, that + `--division` / `--agent` / `--agents-file` filter, that `--dry-run` writes + nothing, and that paths with spaces survive. CI runs it on Linux and macOS. If your PR commits the converted output (the generated `integrations//*` files), CI and review will ask you to remove it and add the `.gitignore` rule instead. diff --git a/scripts/install.sh b/scripts/install.sh index dfc430da..4e1b9103 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -283,6 +283,20 @@ install_file() { } # resolve_dest — --path > $ENV_VAR > default. +# path_collision_group — tools in the same group write identical +# filenames into a shared --path and would overwrite each other; empty means +# the tool's output is distinct and may share a path with anything. Derived by +# installing one agent with every tool into a sandbox and comparing what +# landed; re-measure if a converter's output naming changes. +path_collision_group() { + case "$1" in + claude-code|copilot) printf 'raw-source-md' ;; # -.md + gemini-cli|opencode|qwen|zcode) printf 'slug-md' ;; # .md + antigravity|osaurus) printf 'agency-skill' ;; # agency-/SKILL.md + *) printf '' ;; + esac +} + resolve_dest() { local tool="$1" def="$2" var="" [[ -n "$OVERRIDE_PATH" ]] && { printf '%s' "$OVERRIDE_PATH"; return; } @@ -324,7 +338,11 @@ ensure_converted() { $AUTO_CONVERT || return 0 case "$tool" in claude-code|copilot) return 0 ;; esac local d="$INTEGRATIONS/$tool" - if [[ ! -d "$d" ]] || [[ -z "$(find "$d" -type f 2>/dev/null | head -1)" ]]; then + # Every integrations// ships a committed README.md, so "any file + # present" mistook the README for generated output and never converted in a + # fresh checkout (the installer then hard-failed " missing"). Only files + # other than the README count as output. + if [[ ! -d "$d" ]] || [[ -z "$(find "$d" -type f ! -name 'README.md' 2>/dev/null | head -1)" ]]; then warn "$tool: integration files missing — running convert.sh --tool $tool" "$SCRIPT_DIR/convert.sh" --tool "$tool" >/dev/null 2>&1 \ && ok "$tool: generated integration files" \ @@ -1258,11 +1276,22 @@ main() { $duplicate || _cleaned+=("$_t") done _tool_list=("${_cleaned[@]}") - # --path is a single-destination override; with several tools every one of - # them would land in the same directory and clobber each other. + # --path is one shared directory. Tools that write the same filenames into + # it silently overwrite each other; tools with distinct outputs coexist. + # Refuse only the colliding combinations (see path_collision_group). if [[ -n "$OVERRIDE_PATH" && ${#_tool_list[@]} -gt 1 ]]; then - err "--path sets ONE destination; use it with exactly one --tool (got ${#_tool_list[@]}: ${_tool_list[*]})." - exit 1 + local _ta _tb _ga _gb + for _ta in "${_tool_list[@]}"; do + _ga="$(path_collision_group "$_ta")"; [[ -z "$_ga" ]] && continue + for _tb in "${_tool_list[@]}"; do + [[ "$_tb" == "$_ta" ]] && continue + _gb="$(path_collision_group "$_tb")" + if [[ "$_ga" == "$_gb" ]]; then + err "--path is one shared directory, and $_ta and $_tb write the same filenames into it — they would overwrite each other. Use one of them per --path (tools with distinct outputs may share one)." + exit 1 + fi + done + done fi fi diff --git a/scripts/test-install.sh b/scripts/test-install.sh new file mode 100755 index 00000000..01a14611 --- /dev/null +++ b/scripts/test-install.sh @@ -0,0 +1,321 @@ +#!/usr/bin/env bash +# +# test-install.sh — regression tests for scripts/install.sh. +# +# install.sh is the largest script in the repo and every install bug so far has +# been a silent one: agents land in the wrong directory, a path with a space is +# split into two, a filter installs everything. These tests pin the observable +# contract — where files land and how many — so those regressions fail loudly. +# +# Design constraints (same as the rest of scripts/): +# * bash 3.2 + BSD userland, no jq, no GNU-only flags. +# * Never touches the real $HOME. Every case runs with HOME set to a fresh +# sandbox, so a broken default path writes into the sandbox, not your config. +# * Only exercises the two source tools (claude-code, copilot) so no case +# depends on convert.sh output being present or fresh. +# +# Usage: ./scripts/test-install.sh [-v] +# -v echo the installer's own output for failing cases + +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +INSTALL="$SCRIPT_DIR/install.sh" +# shellcheck source=scripts/lib.sh +. "$SCRIPT_DIR/lib.sh" + +VERBOSE=false +[[ "${1:-}" == "-v" ]] && VERBOSE=true + +passed=0 +failed=0 +xfailed=0 +SANDBOX_ROOT="$(mktemp -d "${TMPDIR:-/tmp}/agency-install-tests.XXXXXX")" +trap 'rm -rf "$SANDBOX_ROOT"' EXIT + +pass() { printf ' ok %s\n' "$1"; passed=$((passed + 1)); } +fail() { + printf ' FAIL %s\n' "$1" + [[ -n "${2:-}" ]] && printf ' %s\n' "$2" + failed=$((failed + 1)) +} + +# assert_eq