[rominf]

Summary

• A bash, zsh, or ksh login shell left fish unset, so an interactive fish session never received the PATH block or completions. When fish is installed (fish is on PATH, or ~/.config/fish already exists), the Linux installer now writes that block and the completion script. rocm uninstall still removes the block.
• tests/install-sh/run.sh covers the shell matrix. @id:lifecycle-linux-fish-configured-from-bash covers the packaged binary and uninstall on the release-install lifecycle lane. The Linux build-and-test job runs the shell script.

Test plan

• sh tests/install-sh/run.sh passed locally (9 cases).
• @id:lifecycle-linux-fish-configured-from-bash was added and was not executed locally. It needs the release-install lifecycle lane (@lifecycle, @requires-os:linux).
• Eight unrelated workflow-contract tests failed locally because rocm-smi cannot run when /tmp is noexec. That is an environment limit, separate from this change.
• CI build-and-test runs tests/install-sh/run.sh when the heavy path is selected. The lifecycle scenario runs on the release-install lane.
• This worktree was not rebuilt with cargo build --workspace --all-targets. A later build of the origin/main checkout does not prove this branch compiles.

[nowycondro]

🤖 Automated review (Claude Code) · 8ca724d

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — needs work

Full review of the whole change.
No ticket named in the description.

Blocking

• Commit lacks a GitHub-"Verified" signature, tripping the repo's enforced commit-signatures gate — the PR's sole commit 8ca724d49574220c84894282a349f0796537644a
  The required CI check "Commit signatures + sign-off" fails with "signature is not GitHub-'Verified'" (the commit does carry a Signed-off-by: trailer, so DCO sign-off is present, but its cryptographic signature does not verify as GitHub-"Verified"). AGENTS.md §2 states this gate is now "enforced, not just policy": "a blocking CI gate (commit-signatures) re-checks every PR with GitHub 'Verified' status." The PR's mergeable_state is blocked, and this is the only failing required check as of this commit.
  Confidence 100 · mechanical · Fix: re-sign the commit with a key registered on the author's GitHub account (SSH or GPG), e.g. git commit --amend -S --no-edit && git push --force-with-lease, or for a range git rebase --exec 'git commit --amend --no-edit -S' <base>.

Non-blocking

None

Decisions for the author

• Detecting "fish is installed" via ~/.config/fish existing, not just command -v fish (install.sh:331-335) — tradeoff
  Pro: catches an interactive fish user whose login shell is bash/zsh/ksh, and lets the hermetic test suite exercise the path without a real fish binary. Con: a stray ~/.config/fish left by an uninstalled or never-really-used fish will now get an unsolicited PATH block and completions on every install/reinstall. The code's own comments make clear this is deliberate — flagging only so the bar for "installed" is a conscious choice.
• configure_fish_path/install_fish_completions (install.sh:340-380) largely re-implement the report/record pattern already inlined at install.sh:596-631 — non-blocking-improvement
  The "updated:/unchanged:/else" message switch and the profile_has_own_block + record_path_edit pair are duplicated almost verbatim between the login-shell block and the fish block (~20 duplicated lines). A small shared helper taking (profile, shell-kind, label) would remove the duplication; current behavior is correct and well-tested, so this is purely a maintenance-cost call.

Positive signals

• tests/install-sh/run.sh is a thorough hermetic harness — 9 cases crossing bash/zsh/ksh/fish login shells with fish-on-PATH, fish-config-dir-only, no-fish, reinstall idempotency, opt-out, and a custom profile path — and it ran and passed in CI on this commit ("Installer fish profile checks" step).
• The pre-existing apps/rocm/src/uninstall.rs::plan_path_edits/legacy_profile_candidates already generalizes over multiple recorded shell profiles and already lists ~/.config/fish/config.fish as a legacy candidate, so rocm uninstall correctly undoes the new fish block with no change needed there — confirmed by reading that code, not just trusting the PR description's claim.
• The new @lifecycle scenario lifecycle-28 is not merely "added but unexecuted" as the PR's own test plan states — it actually ran end-to-end on the hosted build-and-test job's "Acceptance install lifecycle" step (E2E_INCLUDE_LIFECYCLE=1) and passed all of its steps, giving this change stronger CI verification than its description suggests.
• Documentation (README.md, docs/engine-plugins.md, docs/manual-testing.md, docs/testing.md) was updated consistently with the behavior change, including the new CI step and the uninstall dry-run output.

Deployment notes

• Users who have ever created ~/.config/fish (even without using fish day to day) will, from this version on, have that file appended to and a rocm.fish completions file written on their next rocm install/reinstall. This is intended per the PR but is a new side effect worth a release note if this project publishes one (none found in this repo).

What this covered

Read all 10 changed files completely — install.sh, README.md, docs/engine-plugins.md, docs/manual-testing.md, docs/testing.md, tests/install-sh/run.sh, tests/e2e-cucumber/features/install_lifecycle.feature, tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs, xtask/src/workflow_contract.rs, .github/workflows/ci.yml — plus the surrounding install.sh functions the new code calls into (profile_path_for_shell, append_path_snippet, profile_has_own_block, record_path_edit, ensure_installer_process_path) and the pre-existing consumer apps/rocm/src/uninstall.rs, to verify cross-file consistency. Single-pass review (no fan-out): the change was small enough — 584 insertions across 10 files, mostly test code — for one context to read completely. Commits reviewed: 3dec0eb2ad9b1b6169cdc9fdd437203582b13379…8ca724d49574220c84894282a349f0796537644a (this branch's actual fork point; GitHub's reported base.sha cc56179 is main's later tip, not this branch's fork point).

Checked CI via gh pr view --json statusCheckRollup rather than re-deriving coverage from workflow definitions; all required checks are green except the commit-signature gate above. At review time several self-hosted E2E self-hosted jobs (MI300X, Strix Halo Ubuntu release/nightly) were still queued/in-progress; this is unrelated to the @lifecycle scenario this PR adds, which already ran and passed on the hosted job as noted above.

Did not run: "history" pass found nothing beyond the single clean commit; "prior changes" pass — no earlier reviews exist on this PR to reconcile against (confirmed via the review list). No ticket is named in the description, so ticket retrieval does not apply.
Not reconciled against any discussion here.