fix: false positive on verify_install script being external shell

[elibosley]

• Introduced a new test script for shell detection logic in verify_install.sh.
• Updated the package.json to include a new test command for shell detection.
• Enhanced the verify_install.sh script to accurately check the current shell interpreter using /proc and fallback methods.

This improves the testing framework and ensures better validation of shell detection functionality.

Summary by CodeRabbit

• Bug Fixes
  • Installer now detects the actual interpreter more robustly and strictly requires Bash; non-Bash environments produce clear error messages and abort installation.
• Tests
  • Added automated shell-detection tests to validate behavior across environments.
  • Test suite expanded to run the new shell-detection checks as part of the standard test command.

[coderabbitai]

Walkthrough

Replaces shell detection in verify_install.sh to read the interpreter via /proc with a ps fallback and enforces Bash-only execution; adds a new shell-detection test script and wires it into plugin/package.json test workflow.

Changes

Cohort / File(s)	Change Summary
NPM Scripts plugin/package.json	Appended && pnpm run test:shell-detection to test. Added new script test:shell-detection running bash ./tests/test-shell-detection.sh.
Shell detection logic plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh	Replaced ps -o comm= -p $$ detection with readlink "/proc/$$/exe" and basename to obtain interpreter; fallback to ps when /proc unavailable; exit with error if interpreter is not bash. Comments updated.
Shell detection tests plugin/tests/test-shell-detection.sh	Added a test harness that validates the new shell-detection function across scenarios (bash success, non-bash failure, /proc availability, script-name independence) and reports pass/fail for CI integration.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant CI as User/CI
  participant Script as verify_install.sh
  participant Proc as /proc (readlink)
  participant PS as ps(1)

  CI->>Script: Invoke verify_install.sh
  activate Script
  Script->>Proc: readlink /proc/$$/exe
  alt /proc available
    Proc-->>Script: interpreter path
  else /proc unavailable
    Script->>PS: ps -o comm= -p $PPID
    PS-->>Script: parent command
  end
  Script->>Script: basename(interpreter)
  alt interpreter == "bash"
    Script-->>CI: continue (success)
  else
    Script-->>CI: print errors & exit 1
  end
  deactivate Script

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I hop through code with curious nose,
/proc reveals where each shell goes.
If not bash, I thump and cry “no pass!”
Tests march in line, each little task—
CI cheers softly, carrots at last. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title concisely describes the primary fix—resolving a false-positive detection that treated verify_install.sh as an external (non-bash) shell—so it accurately reflects the main change to verify_install.sh while remaining short and team-readable; it is acceptable that it does not enumerate the added test and package.json updates because titles need not cover every detail.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/verify_install

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a5fef31 and 967e553.

📒 Files selected for processing (2)

• plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh (1 hunks)
• plugin/tests/test-shell-detection.sh (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh
• plugin/tests/test-shell-detection.sh

---

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

[claude]

Claude finished @elibosley's task —— View job

---

Code Review - DIFF Analysis

Analyzing the DIFF for critical issues in changed lines only...

Todo List:

• Read and analyze the changed files
• Review DIFF for critical bugs, security issues, breaking changes, data loss risks
• Provide feedback on findings

Analysis Results

⚠️ CRITICAL BUG FOUND - Line 19 in verify_install.sh

The fallback shell detection logic has a critical flaw:

# Line 19 - PROBLEMATIC:
current_shell=$(ps -o comm= -p $$)

Issue: Using $$ (current process PID) in the fallback will return the script name or bash, not detect if the script was invoked from a non-bash parent shell. This defeats the purpose of the shell detection.

Expected Behavior: Should detect when script is called like zsh verify_install.sh

Fix Required: Change $$ to $PPID to check the parent process that invoked this script:

current_shell=$(ps -o comm= -p $PPID)

Other Changes Reviewed:

• ✅ package.json: Added test command - no issues
• ✅ test-shell-detection.sh: New test file - no critical issues in new code
• ✅ /proc detection logic (lines 14-16): Correctly implemented

Recommendation: Fix the fallback PID detection before merging to prevent false negatives in shell detection.

---

[codecov]

Codecov Report

❌ Patch coverage is 91.71975% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.60%. Comparing base (c4ca761) to head (967e553).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
...eb/src/components/UserProfile/CallbackFeedback.vue	0.00%	6 Missing ⚠️
web/src/components/HeaderOsVersion.standalone.vue	66.66%	5 Missing ⚠️
unraid-ui/src/composables/useTeleport.ts	87.50%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1704      +/-   ##
==========================================
- Coverage   58.45%   52.60%   -5.86%     
==========================================
  Files         636      854     +218     
  Lines       31189    47864   +16675     
  Branches     3659     4749    +1090     
==========================================
+ Hits        18232    25177    +6945     
- Misses      12920    22618    +9698     
- Partials       37       69      +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (4)

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh (1)

24-33: Early non-bash exit is appropriate; message can be safer.

The guidance snippet could cause loops if users paste it when $current_shell later resolves to bash. Consider printing a concrete example instead, e.g., [[ $- == *i* ]] && exec zsh.

plugin/package.json (1)

36-39: Wire-up looks good; consider CI-friendly vitest invocation.

vitest typically detects CI, but to avoid interactive mode edge cases, you may prefer vitest run. Optional tweak:

-    "test": "vitest && pnpm run test:extractor && pnpm run test:shell-detection",
+    "test": "vitest run && pnpm run test:extractor && pnpm run test:shell-detection",

plugin/tests/test-shell-detection.sh (2)

109-109: Quote command substitutions to avoid word splitting (SC2046).

Both $(command -v sh) and $(command -v bash) should be quoted.

-if command -v sh >/dev/null 2>&1 && [ "$(readlink -f $(command -v sh))" != "$(readlink -f $(command -v bash))" ]; then
+if command -v sh >/dev/null 2>&1 && [ "$(readlink -f "$(command -v sh)")" != "$(readlink -f "$(command -v bash)")" ]; then

---

95-105: Extraction of check section relies on a lone check_shell line.

sed -n '1,/^check_shell$/p' depends on a call line exactly equal to check_shell. If that changes (e.g., args added), this test will miss the call. Match the call more flexibly.

-    sed -n '1,/^check_shell$/p' "$VERIFY_SCRIPT" > "$TEMP_VERIFY"
+    sed -n '1,/^check_shell\(\s\|$\)/p' "$VERIFY_SCRIPT" > "$TEMP_VERIFY"

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b153189 and a5fef31.

📒 Files selected for processing (3)

• plugin/package.json (1 hunks)
• plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh (1 hunks)
• plugin/tests/test-shell-detection.sh (1 hunks)

🧰 Additional context used

🧠 Learnings (3)

📚 Learning: 2025-08-11T15:10:28.150Z

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Run terminal commands with pnpm within the web directory (e.g., pnpm test)

Applied to files:

• plugin/package.json

📚 Learning: 2025-08-11T15:10:28.150Z

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: This Nuxt app is tested with vitest outside the Nuxt runtime; structure tests accordingly

Applied to files:

• plugin/package.json

📚 Learning: 2025-08-11T15:07:39.222Z

Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-08-11T15:07:39.222Z
Learning: Applies to api/**/*.{test,spec}.{js,jsx,ts,tsx} : Use Vitest for tests in the api; do not use Jest

Applied to files:

• plugin/package.json

🪛 Shellcheck (0.10.0)

plugin/tests/test-shell-detection.sh

[warning] 109-109: Quote this to prevent word splitting.

(SC2046)

---

[warning] 109-109: Quote this to prevent word splitting.

(SC2046)

🔇 Additional comments (2)

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh (1)

36-38: Good: fail-fast shell guard before any other checks.

Running check_shell first prevents subtle breakage later in the script under non-bash.

plugin/tests/test-shell-detection.sh (1)

85-92: Nice: hermetic temp script for happy-path under bash.

Self-contained extraction plus cleanup is solid; toggling set -e around test runs is handled correctly.

[coderabbitai]

⚠️ Potential issue

Fallback uses PPID (parent) instead of this script’s PID — can mis-detect shell.

If /proc is unavailable, ps -o comm= -p $PPID will report the invoking shell (e.g., zsh) rather than the interpreter executing this script. Use $$ so we inspect the current process. This prevents false “non-bash” exits when running bash verify_install.sh from another shell.

Apply:

-  if [ -e "/proc/$$/exe" ]; then
-    current_shell=$(readlink "/proc/$$/exe")
-  else
-    # Fallback to checking the parent process if /proc isn't available
-    current_shell=$(ps -o comm= -p $PPID)
-  fi
+  if [ -e "/proc/$$/exe" ]; then
+    current_shell=$(readlink "/proc/$$/exe")
+  else
+    # Fallback: inspect THIS process, not the parent
+    if current_shell=$(ps -o comm= -p $$ 2>/dev/null); then
+      :
+    elif current_shell=$(ps -p $$ -o comm= 2>/dev/null); then
+      :
+    else
+      current_shell="$0"
+    fi
+  fi

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if [ -e "/proc/$$/exe" ]; then
	current_shell=$(readlink "/proc/$$/exe")
	else
	# Fallback to checking the parent process if /proc isn't available
	current_shell=$(ps -o comm= -p $PPID)
	fi
	if [ -e "/proc/$$/exe" ]; then
	current_shell=$(readlink "/proc/$$/exe")
	else
	# Fallback: inspect THIS process, not the parent
	if current_shell=$(ps -o comm= -p $$ 2>/dev/null); then
	:
	elif current_shell=$(ps -p $$ -o comm= 2>/dev/null); then
	:
	else
	current_shell="$0"
	fi
	fi

🤖 Prompt for AI Agents

In
plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh
around lines 14-19, the fallback branch uses ps -o comm= -p $PPID which inspects
the parent process and can mis-detect the interpreter running this script;
change the fallback to inspect the current process by using ps -o comm= -p $$
(the script's PID) so we detect the actual interpreter executing this script,
and ensure any variables are properly quoted and trimmed as needed.

[github-actions]

This plugin has been deployed to Cloudflare R2 and is available for testing.
Download it at this URL:

https://preview.dl.unraid.net/unraid-api/tag/PR1704/dynamix.unraid.net.plg