Skip to content

feat: Universal installer with legacy framework uninstall procedures#53

Merged
dpolishuk merged 18 commits into
mainfrom
feature/feature-universal-installer-with-legacy-framework-uninstall-procedures
May 3, 2026
Merged

feat: Universal installer with legacy framework uninstall procedures#53
dpolishuk merged 18 commits into
mainfrom
feature/feature-universal-installer-with-legacy-framework-uninstall-procedures

Conversation

@dpolishuk
Copy link
Copy Markdown
Owner

@dpolishuk dpolishuk commented May 2, 2026

Epic

Closes hyper-8lq: Feature: Universal installer with legacy framework uninstall procedures

Summary

  • Promotes scripts/install.sh into the universal public entrypoint that works both from a local checkout and via GitHub raw curl | bash pipe, with bootstrap clone logic and argument preservation.
  • Adds --hosts <list> argument parsing with comma-separated agent selection and Pi delegation through the universal entrypoint.
  • Implements safe legacy framework cleanup for hyperpowers, myhyperpowers, and superpowers with --remove-legacy and --replace-legacy flags.
  • Converts scripts/setup-pi.sh into a minimal compatibility shim that delegates to install.sh --hosts pi.
  • Updates README.md to publish universal GitHub one-liners and deprecate setup-pi.sh.

Tasks Completed

  • hyper-2ff: Make install.sh the universal curl bootstrap entrypoint
  • hyper-v2h: Implement legacy framework cleanup in universal installer
  • hyper-aeg: Convert setup-pi.sh to compatibility shim delegating to install.sh
  • hyper-236: Remediation - fix critical installer bugs from end-of-epic review

Test Plan

  • node --test tests/install-script.test.js — 32 tests passing
  • node --test tests/*.test.js — 383 tests passing (full suite)
  • node scripts/sync-codex-skills.js --check — ok

Summary by CodeRabbit

  • New Features

    • Universal installer supports macOS/Linux via curl|bash, explicit Pi host handling, and new CLI flags for hosts, conflict handling, legacy removal/replacement.
  • Documentation

    • README Quick Start updated to show the universal installer and revised Pi install instructions.
  • Maintenance

    • Safer legacy migration with quarantine/manifest-driven cleanup, more conservative purge/uninstall behavior, and deprecation/forwarding wrapper for the old Pi installer.
  • Tests

    • Expanded coverage for bootstrap, delegation, legacy cleanup, purge, mixed-hosts, and Pi forwarding.

Copilot AI review requested due to automatic review settings May 2, 2026 20:22
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 2, 2026

Warning

Rate limit exceeded

@dpolishuk has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 31 minutes and 8 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 776f72c9-1187-4aa9-8f7b-7c2bcf373b55

📥 Commits

Reviewing files that changed from the base of the PR and between 1d18c1e and 33a61fc.

📒 Files selected for processing (2)
  • scripts/install.sh
  • tests/install-script.test.js
📝 Walkthrough

Walkthrough

Adds a universal stdin-safe installer bootstrap, manifest-aware legacy cleanup with quarantine, new CLI flags including --hosts/--remove-legacy/--replace-legacy, Pi detection/delegation to Bun, converts setup-pi.sh to a forwarding wrapper, updates README, and adds extensive tests for these flows.

Changes

Installer Modernization and Legacy Cleanup

Layer / File(s) Summary
Bootstrap Detection & Cloning
scripts/install.sh
Adds stdin-safe bootstrap: is_xpowers_checkout(), clone_xpowers_checkout(), bootstrap_from_checkout() to clone repo into a temp dir and exec the cloned scripts/install.sh.
CLI & Orchestration
scripts/install.sh
Adds --hosts <list>, --remove-legacy, --replace-legacy; parses host lists (including pi/all), moves conflict detection earlier, runs legacy removal before installs when requested, and supports Pi delegation.
Legacy Quarantine Infrastructure
scripts/install.sh
Introduces LEGACY_QUARANTINE_DIR, quarantine_item(), remove_legacy_from_manifest(), remove_legacy() to quarantine and remove legacy items with collision-safe naming and manifest-driven cleanup (supports --dry-run).
Marker-Driven Purge & Uninstall
scripts/install.sh
When --purge and no manifest present, limits removal to markers/version/backup files instead of broader agent dirs; updates uninstall/purge behavior accordingly.
Agent Discovery / Pi Support
scripts/install.sh
Adds pi to AGENT_LABELS/AGENT_ORDER, detect_pi(), uninstall_pi(), status_pi(), and skips Pi in per-agent loop when delegated.
Pi Install Delegation
scripts/install.sh, scripts/install.ts
Delegates Pi installs to bun scripts/install.ts --hosts pi when appropriate; enforces Bun requirement and adjusts exec vs subprocess behavior depending on host selection.
Pi Installer Deprecation Wrapper
scripts/setup-pi.sh
Replaces full Pi installer with a forwarding/deprecation wrapper that resolves location, allowlists/forwards supported flags, strips --hosts, and execs install.sh --hosts pi with forwarded args.
Documentation
README.md
Updates Quick Start and Pi install snippets to use `scripts/install.sh
Tests
tests/install-script.test.js
Adds git/bootstrap fixtures and stdin-bootstrap tests; extensive legacy-cleanup tests (dry-run, replace, manifest/non-manifest behaviors, idempotency, quarantine/path-traversal safety, marker-driven purge); updates README test; adds setup-pi.sh piped-exec guard test.

Sequence Diagram

sequenceDiagram
    participant User
    participant Shell as "bash (stdin)"
    participant Bootstrap as "bootstrap logic"
    participant Git as "git/clone"
    participant Installer as "cloned install.sh"
    participant LegacyMgr as "legacy manager"
    participant Bun as "bun scripts/install.ts (Pi)"
    participant AgentHome as "agent home (.claude, pi, ...)"

    User->>Shell: curl ... | bash (no local checkout)
    Shell->>Bootstrap: bootstrap_from_checkout(args)
    Bootstrap->>Git: is_xpowers_checkout()? / clone if needed
    Git-->>Bootstrap: cloned repo path
    Bootstrap->>Installer: exec cloned install.sh with args
    Installer->>Installer: parse --hosts / --remove-legacy / --replace-legacy
    alt hosts includes pi and delegation chosen
        Installer->>Bun: bun scripts/install.ts --hosts pi [--yes/--allow-conflicts/...]
        Bun-->>Installer: delegated exit/status
    end
    alt --remove-legacy / --replace-legacy
        Installer->>LegacyMgr: remove_legacy()
        LegacyMgr->>AgentHome: quarantine_item(path) / remove_legacy_from_manifest()
        AgentHome-->>LegacyMgr: quarantined / removed / markers updated
    end
    Installer->>AgentHome: perform install/uninstall per agent
    AgentHome-->>Installer: status
    Installer-->>User: success / error
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Poem

🐰 I hopped from curl into a cloned home,

I hid old bits in quarantine soil and loam,
Flags point the way, Pi hums through Bun’s tune,
The installer springs — beneath the quiet moon.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.45% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main changes: introducing a universal installer (install.sh) and adding legacy framework uninstall procedures, which are the primary objectives of this changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/feature-universal-installer-with-legacy-framework-uninstall-procedures

Tip

💬 Introducing [Slack Agent](https://www.coderabbit.ai/agent): Turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value).


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 31 minutes and 8 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

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: 0d6fda4ed9

ℹ️ 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 scripts/install.sh Outdated
Comment thread scripts/install.sh
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@scripts/install.sh`:
- Around line 1337-1346: The legacy-removal branch currently calls remove_legacy
while PURGE may be set, causing hard-deletes; before calling remove_legacy in
the block guarded by REMOVE_LEGACY/REPLACE_LEGACY, explicitly reject/abort when
PURGE=true (and UNINSTALL is not true) by checking the PURGE variable and
erroring out (similar to the later “--purge requires --uninstall” check), e.g.
add a guard that inspects PURGE and prints an error asking for --uninstall or to
unset --purge, then exit 1; reference the REMOVE_LEGACY/REPLACE_LEGACY condition
and the remove_legacy function when adding this guard.
- Around line 275-303: The code currently falls back to using xpowers_manifest
as a "legacy" manifest causing live XPowers installs to be treated as legacy;
change the selection logic so remove_legacy_from_manifest and quarantine_item
are only run on a verified legacy manifest: require either a dedicated legacy
marker (e.g., check for a file like "${agent_home}/.hyperpowers-legacy-marker")
or validate the manifest contents for a legacy header before assigning
manifest_to_use to xpowers_manifest; keep the existing behavior for
legacy_manifest, and ensure DRY_RUN and PURGE gating remains; update references
to legacy_manifest, xpowers_manifest, manifest_to_use,
remove_legacy_from_manifest and quarantine_item accordingly so live
.xpowers-manifest is never used as a legacy manifest unless the explicit legacy
marker/validation passes.

In `@scripts/setup-pi.sh`:
- Around line 17-35: The wrapper currently strips unknown flags (in the while
loop that builds forward_args and then calls exec bash
"${SCRIPT_DIR}/install.sh" --hosts pi "${forward_args[@]}"), causing legitimate
flags like --help, --version, --status, --remove-legacy, and --replace-legacy to
be discarded; update the case block to explicitly accept and forward these safe
non-host flags by adding them to forward_args (e.g., include
--help|--version|--status|--remove-legacy|--replace-legacy in the pattern that
appends to forward_args), and for any truly unsupported option add a default
branch that prints a clear error/usage and exits non-zero instead of silently
dropping the flag so unsupported requests fail fast.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9cd87246-ea09-4cfd-8676-d1b35fb825dc

📥 Commits

Reviewing files that changed from the base of the PR and between c19c19d and 0d6fda4.

📒 Files selected for processing (4)
  • README.md
  • scripts/install.sh
  • scripts/setup-pi.sh
  • tests/install-script.test.js

Comment thread scripts/install.sh Outdated
Comment thread scripts/install.sh
Comment thread scripts/install.sh
Comment thread scripts/setup-pi.sh
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR turns scripts/install.sh into the main public installer entrypoint for XPowers, aiming to support both local checkouts and curl | bash usage while adding legacy-install cleanup for older frameworks. In the broader codebase, it expands the shell installer’s role from a local multi-agent installer into the user-facing installation and migration path.

Changes:

  • Added bootstrap logic to scripts/install.sh so it can re-exec from a cloned checkout when run from stdin, plus new --hosts, --remove-legacy, and --replace-legacy flags.
  • Replaced the old Pi-specific shell installer with a small compatibility shim that delegates to install.sh --hosts pi.
  • Updated README installation docs and added/expanded installer tests for bootstrap, Pi delegation, and legacy cleanup flows.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.

File Description
tests/install-script.test.js Adds regression tests for stdin bootstrap, Pi delegation, legacy cleanup, purge behavior, and setup-pi shim behavior.
scripts/setup-pi.sh Replaces the old Pi installer with a deprecation shim that forwards supported args to install.sh --hosts pi.
scripts/install.sh Implements bootstrap-from-checkout behavior, host-list parsing, Pi delegation, and legacy uninstall/replacement logic.
README.md Publishes the universal installer commands and documents legacy replacement/removal flows plus the new Pi install command.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread scripts/install.sh Outdated
Comment thread scripts/install.sh Outdated
Comment thread scripts/install.sh Outdated
Comment thread scripts/install.sh Outdated
Comment thread scripts/install.sh Outdated
Comment thread scripts/install.sh
Comment thread scripts/setup-pi.sh
Comment thread scripts/install.sh Outdated
Comment thread scripts/install.sh Outdated
Comment thread scripts/install.sh Outdated
…gation args, host selection, setup-pi shim
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

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: f0e23a62da

ℹ️ 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 scripts/install.sh Outdated
Comment thread scripts/install.sh Outdated
@dpolishuk
Copy link
Copy Markdown
Owner Author

Reviewing all unresolved threads now. Will address remaining issues.

…resolution, multi-manifest cleanup

- Track pi_delegated flag: only skip Pi in install loop when Bun delegation
  actually occurred. Fixes --all/--hosts all auto-selecting Pi but never
  installing it because delegation ran before detect_all().

- SELECT_ALL flag: --all and --hosts all now set SELECT_ALL=true. Resolve
  step merges all detected agents with any explicitly-selected ones,
  deduplicating. Fixes --hosts all,pi only installing Pi instead of all hosts.

- Process every legacy manifest per home: remove_legacy() now deduplicates
  by manifest path instead of agent_home, so multiple manifests in the same
  home (e.g. .hyperpowers-manifest + .superpowers-manifest) are all cleaned up.

All 383 tests pass.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

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: 7e9d810159

ℹ️ 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 scripts/install.sh Outdated
Comment thread scripts/install.sh
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
scripts/install.sh (1)

1375-1402: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Pi delegation runs before host resolution, breaking --all/--hosts all,pi flows.

Line 1375 computes has_pi from pre-resolution SELECTED_AGENTS, so delegation misses Pi for --all and can prematurely exec Pi-only for --hosts all,pi. Downstream, Line 1539 can then hit missing install_pi/validate_pi calls.

💡 Suggested fix
-  # Pi delegation: TypeScript installer handles Pi install only
-  local has_pi=false
-  local pi_delegated=false
-  for agent in "${SELECTED_AGENTS[@]}"; do
-    [[ "$agent" == "pi" ]] && has_pi=true
-  done
-
-  if [[ "$has_pi" == true && "$MODE" == "install" ]]; then
-    ...
-  fi
+  # Pi delegation state (actual delegation happens after final host resolution)
+  local pi_delegated=false
...
   # --- Resolve selected agents ---
   if [[ ${`#SELECTED_AGENTS`[@]} -eq 0 ]] || [[ "$SELECT_ALL" == true ]]; then
     ...
   fi
+
+  # Pi delegation: TypeScript installer handles Pi install only
+  local has_pi=false
+  for agent in "${SELECTED_AGENTS[@]}"; do
+    [[ "$agent" == "pi" ]] && has_pi=true
+  done
+
+  if [[ "$has_pi" == true && "$MODE" == "install" ]]; then
+    pi_delegated=true
+    ...
+    if [[ ${`#SELECTED_AGENTS`[@]} -eq 1 ]]; then
+      cd "${REPO_ROOT}" && exec bun scripts/install.ts "${PI_ARGS[@]}"
+    else
+      cd "${REPO_ROOT}"
+      if ! bun scripts/install.ts "${PI_ARGS[@]}"; then
+        FAILED_AGENTS+=("pi")
+      fi
+    fi
+  fi

Also applies to: 1423-1440, 1539-1543

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/install.sh` around lines 1375 - 1402, The pi delegation logic
currently checks SELECTED_AGENTS before host resolution which breaks flows like
--all or --hosts all,pi; update the logic so has_pi and pi_delegated are
computed from the resolved hosts list (the post-resolution array used for
install_pi/validate_pi) instead of the pre-resolution SELECTED_AGENTS, and
ensure the exec path for bun scripts/install.ts only runs when the resolved
hosts list contains exactly "pi" (otherwise run bun without exec and append "pi"
to FAILED_AGENTS on non-zero exit); update references around the PI_ARGS
construction (use MODE, FORCE, ALLOW_CONFLICTS, bun and scripts/install.ts) to
operate on the resolved host set so downstream install_pi/validate_pi calls are
not skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@scripts/install.sh`:
- Around line 1035-1045: uninstall_pi currently only removes the extension
directory (ext_dir) but leaves the Pi metadata file (${home}/.xpowers-version)
so status can still report “installed”; update uninstall_pi to also remove the
metadata file (compute the same home from AGENT_PATHS[pi] or ${HOME}/.pi/agent
and set a variable like meta="${home}/.xpowers-version"), honoring DRY_RUN (echo
the removal when DRY_RUN is true, rm -f when not), and apply the same metadata
removal logic to the related uninstall code path around lines 1109-1116 to avoid
uninstall/status drift.
- Around line 232-236: The deletion branch currently calls rm -rf on
slash-suffixed directory targets which can follow symlinks and remove the link
target; change the logic to detect and unlink symlinks first using the
symlink-safe form of the path (strip a trailing slash when testing: e.g. test -L
"${target%/}"), call unlink or rm -f on the symlink, otherwise if it is a real
directory use rm -rf on "$target"; keep the file branch unchanged and ensure
count++ still happens when a removal occurs (referencing the variables entry,
target and count).

---

Duplicate comments:
In `@scripts/install.sh`:
- Around line 1375-1402: The pi delegation logic currently checks
SELECTED_AGENTS before host resolution which breaks flows like --all or --hosts
all,pi; update the logic so has_pi and pi_delegated are computed from the
resolved hosts list (the post-resolution array used for install_pi/validate_pi)
instead of the pre-resolution SELECTED_AGENTS, and ensure the exec path for bun
scripts/install.ts only runs when the resolved hosts list contains exactly "pi"
(otherwise run bun without exec and append "pi" to FAILED_AGENTS on non-zero
exit); update references around the PI_ARGS construction (use MODE, FORCE,
ALLOW_CONFLICTS, bun and scripts/install.ts) to operate on the resolved host set
so downstream install_pi/validate_pi calls are not skipped.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4886c117-84ea-4551-8374-74f7e9a59ebc

📥 Commits

Reviewing files that changed from the base of the PR and between 0d6fda4 and 7e9d810.

📒 Files selected for processing (3)
  • scripts/install.sh
  • scripts/setup-pi.sh
  • tests/install-script.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
  • scripts/setup-pi.sh
  • tests/install-script.test.js

Comment thread scripts/install.sh
Comment thread scripts/install.sh
…nstall cleanup

- Reject --dry-run for Pi install delegation with clear error. install.ts
  does not support --dry-run; failing closed prevents accidental mutations
  when user runs --hosts pi --dry-run or --all --dry-run.

- Full Pi uninstall cleanup: uninstall_pi() now removes AGENTS.md XPowers
  section (preserving user content), .xpowers-version, and .xpowers-manifest
  in addition to extensions/xpowers. Previously only the extension directory
  was removed, leaving orphaned markers behind.

All 383 tests pass.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

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: a58cbc56a8

ℹ️ 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 scripts/install.sh
Comment thread scripts/install.sh Outdated
dpolishuk added 2 commits May 3, 2026 06:10
…nifest removal

- remove_legacy_from_manifest() and uninstall_from_manifest() now check
  -L before -d when removing directory entries. Previously, a symlink to a
  directory would pass [[ -d "" ]] (which follows symlinks) and
  rm -rf would recursively delete the link target instead of just unlinking
  the symlink.

- Normalize target path by stripping trailing slash before type checks.

All 383 tests pass.
…S.md marker parity, detect_claude consistency

- Remove pi filtering from SELECTED_AGENTS after delegation so mixed-host
  runs correctly report Pi in the success/failure summary.
- Fix uninstall_pi AGENTS.md section markers to match install.ts
  (BEGIN XPOWERS PI / BEGIN HYPERPOWERS PI instead of old BEGIN XPowers AGENTS).
- Add || true to detect_claude for consistency with other detect functions.
- Add test: mixed claude+pi install reports both agents in summary.

Fixes review comments: Copilot #3177181374, Codex #3177953132, CodeRabbit #3177942314.
Closes hyper-nnl.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

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: 16090e8858

ℹ️ 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 scripts/install.sh
Comment thread scripts/install.sh Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@scripts/install.sh`:
- Around line 1485-1487: The current change of directory before invoking the Pi
delegation (the cd "${REPO_ROOT}" immediately before running bun
scripts/install.ts with "${PI_ARGS[@]}") persists and breaks later relative
installs (e.g., install_codex()); instead run the Pi install inside a subshell
or restore cwd after the call so the caller's working directory is
preserved—wrap the cd + bun invocation in a subshell (or alternatively use
pushd/popd around the bun scripts/install.ts "${PI_ARGS[@]}" call) so only that
delegated invocation runs from REPO_ROOT and main() / install_codex() continue
using the original working directory.
- Around line 1068-1070: Replace the in-place sed call that removes the "<!--
BEGIN HYPERPOWERS PI -->" block (currently using sed -i on variable tmp_md) with
a portable approach that does not rely on sed -i: run sed to write the filtered
output to a temporary file (e.g., "${tmp_md}.new") and then mv it back to
tmp_md, or detect GNU vs BSD sed and use the appropriate -i syntax (GNU: -i;
BSD/macOS: -i ''), ensuring the removal targets the '/<!-- BEGIN HYPERPOWERS PI
-->/,/<!-- END HYPERPOWERS PI -->/d' range so the HYPERPOWERS PI block is
reliably deleted on both Linux and macOS.

In `@tests/install-script.test.js`:
- Around line 1021-1028: The test "install.sh marker-driven purge removes exact
files without manifest" is creating a .xpowers-manifest fixture so it never hits
the marker-driven fallback; remove the line that writes .xpowers-manifest (or
replace with a separate test that omits creating .xpowers-manifest) so
uninstall_from_manifest() cannot take the normal manifest path and the
missing-manifest purge logic is exercised.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a3d9d53e-1eb9-458b-895a-5d83aa66d593

📥 Commits

Reviewing files that changed from the base of the PR and between 7e9d810 and 16090e8.

📒 Files selected for processing (2)
  • scripts/install.sh
  • tests/install-script.test.js

Comment thread scripts/install.sh Outdated
Comment thread scripts/install.sh Outdated
Comment thread tests/install-script.test.js Outdated
- Gate legacy-removal behind install mode (Codex P1 #3178104600):
  Reject --remove-legacy/--replace-legacy when MODE is not install.

- Scan legacy manifests even when namespace paths are absent (Codex P2 #3178104601):
  Decouple manifest processing from candidate existence so manifests are
  processed regardless of whether the namespace directory still exists.

- Fix sed -i BSD/macOS incompatibility in Pi uninstall (CodeRabbit #3178108278):
  Replace two-stage sed (one with -i) with a single portable sed using
  multiple -e expressions.

- Keep Pi delegation in subshell to preserve cwd (CodeRabbit #3178108283):
  Wrap cd REPO_ROOT + bun install.ts in a subshell so the working directory
  is restored after Pi delegation, preserving relative-path installs.

- Fix test that never reaches marker-driven fallback (CodeRabbit #3178108285):
  Remove .xpowers-manifest fixture from marker-driven purge test so it
  actually exercises the missing-manifest fallback branch.

Closes hyper-g8x, hyper-pse, hyper-7y0, hyper-cgn, hyper-ktn.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

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: bc5c2be06d

ℹ️ 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 scripts/install.sh Outdated
…when Bun is missing

When Pi is included in a mixed-host run (e.g. --hosts claude,pi) but Bun
is missing or --dry-run is used, the script used to exit immediately and
never install the other requested hosts. Now Pi is marked as failed and
the script continues with remaining agents. Pi-only runs still hard-exit
for clear UX.

- Consolidate Pi skip conditions (missing bun, dry-run) into pi_skip_reason.
- For Pi-only: error and exit 1 (unchanged UX).
- For mixed-host: error, add pi to FAILED_AGENTS, continue with loop.
- Add test: mixed claude+pi skips Pi when Bun is missing and continues.

Fixes Codex P1 #3178122620.
Closes hyper-3nx.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

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: 37b4aebe5c

ℹ️ 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 scripts/install.sh
…xits before install

Review comment #3178170260: --replace-legacy --dry-run previewed legacy
removal but then proceeded into actual install flow, writing files despite
documented preview-only semantics.

- Exit after legacy removal preview when REPLACE_LEGACY is combined with DRY_RUN.
- Guard the main install loop to skip actual installation when DRY_RUN is true,
  preventing all install --dry-run mutations (not just replace-legacy).
- Add tests:
  - --replace-legacy --dry-run exits before installing
  - --hosts claude --dry-run does not write any files

Closes hyper-30y.
@dpolishuk
Copy link
Copy Markdown
Owner Author

All review comments have been addressed in commits f0e23a6 through 1d18c1e. The remaining unresolved threads are code-correct and only need resolution in the GitHub UI.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

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: 1d18c1eb3b

ℹ️ 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 scripts/install.sh Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
scripts/install.sh (1)

1320-1320: 💤 Low value

Remove unused variable ORIGINAL_ARGS.

This variable is declared but never referenced anywhere in the script.

🧹 Proposed fix
   local REMOVE_LEGACY=false
   local REPLACE_LEGACY=false
   local SELECT_ALL=false
   local -a SELECTED_AGENTS=()
-  local -a ORIGINAL_ARGS=("$@")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/install.sh` at line 1320, Remove the unused variable by deleting the
declaration of ORIGINAL_ARGS (the line declaring local -a ORIGINAL_ARGS=("$@"))
so there are no unused local arrays left in the script; search for the symbol
ORIGINAL_ARGS to confirm there are no other references before removing to avoid
breaking any intended behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@scripts/install.sh`:
- Line 1320: Remove the unused variable by deleting the declaration of
ORIGINAL_ARGS (the line declaring local -a ORIGINAL_ARGS=("$@")) so there are no
unused local arrays left in the script; search for the symbol ORIGINAL_ARGS to
confirm there are no other references before removing to avoid breaking any
intended behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a46f5f11-b604-4e2d-9a7d-ad7f7e08fb1b

📥 Commits

Reviewing files that changed from the base of the PR and between 16090e8 and 1d18c1e.

📒 Files selected for processing (2)
  • scripts/install.sh
  • tests/install-script.test.js

dpolishuk added a commit that referenced this pull request May 3, 2026
…t only home dir

Review comment #3178194570: detect_pi() only checked ~/.pi, so on fresh
machines where pi was in PATH but its home directory hadn't been created
yet, Pi was silently omitted from --all installs.

- Also detect Pi when the 'pi' command is available in PATH, consistent
  with detect_gemini().
- Add test: --all detects Pi via PATH executable without ~/.pi.

Closes hyper-o8f.
…t only home dir

Review comment #3178194570: detect_pi() only checked ~/.pi, so on fresh
machines where pi was in PATH but its home directory hadn't been created
yet, Pi was silently omitted from --all installs.

- Also detect Pi when the 'pi' command is available in PATH, consistent
  with detect_gemini().
- Add test: --all detects Pi via PATH executable without ~/.pi.

Closes hyper-o8f.
@dpolishuk dpolishuk force-pushed the feature/feature-universal-installer-with-legacy-framework-uninstall-procedures branch from fe14723 to 93bab4f Compare May 3, 2026 13:57
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

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: 93bab4f1cd

ℹ️ 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 scripts/install.sh
…efore execution

Review comment #3178221697: --hosts parser appends every token directly
into SELECTED_AGENTS without deduplication, so --hosts claude,claude
executes the same host twice. On uninstall, the second pass fails because
the manifest was already removed, producing a false overall failure.

- Deduplicate SELECTED_AGENTS after resolution before execution.
- Add test: --hosts claude,claude only mentions one Claude Code install.

Closes hyper-nim.
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