autoinstall_script/README.md (3)

188-195: Missing language identifiers for fenced code blocks.

Add languages to satisfy MD040 and improve readability.

Example:

-```
+```bash
 cd IvorySQL/IvorySQL-AutoInstaller

-```
+```bash
 chmod +x AutoInstall.sh

---

67-74: Markdown tables need surrounding blank lines and one row has 4 cells.

• Add blank lines before/after tables (MD058).
• Fix the “Common Error Handling” table row that yields 4 cells (MD056).

Also applies to: 239-246, 255-262

---

145-151: Code fences lack language spec.

Tag as bash/conf/text where appropriate to satisfy MD040.

Also applies to: 157-169, 218-236

autoinstall_script/README_CN.md (2)

174-181: Add code fence languages (bash/conf).

Satisfy MD040 and improve readability.

Also applies to: 186-193, 209-212, 261-267, 287-291

---

16-22: Tables need blank lines around and verify column counts.

Add blank lines before/after tables (MD058) and fix any misaligned rows (MD056).

Also applies to: 30-37, 66-73, 236-246, 252-259

autoinstall_script/AutoInstall.sh (6)

866-868: Quote command substitution for robustness.

Minor: quote $(nproc) to avoid word splitting edge cases and quiet SC2046.

-    make -j$(nproc) || STEP_FAIL  "Build failed."
+    make -j"$(nproc)" || STEP_FAIL  "Build failed."

---

1004-1028: Non-systemd helper: ensure sed-safe substitution and POSIX shell.

If paths include & or #, sed replacement may misbehave. Also, the shebang uses bash; consider sh if bash isn’t guaranteed. At minimum, escape & in values.

Example escaping:

-        sed -i "s#__PGDATA_PLACEHOLDER__#$DATA_DIR#g" "$INSTALL_DIR/ivorysql-ctl"
+        sed -i "s#__PGDATA_PLACEHOLDER__#$(printf '%s' "$DATA_DIR" | sed 's/[&]/\\&/g')#g" "$INSTALL_DIR/ivorysql-ctl"

Repeat for INSTALL_DIR and LOG_DIR.

---

942-954: INIT_CMD quoting is brittle.

The long double-quoted assignment with mixed quotes triggers ShellCheck warnings. Build an array or use a here-doc for clarity.

Example:

INIT_CMD=(
  env PGDATA="$DATA_DIR"
  LD_LIBRARY_PATH="$INSTALL_DIR/lib:$INSTALL_DIR/lib/postgresql:${LD_LIBRARY_PATH:-}"
  PATH="$INSTALL_DIR/bin:${PATH:-/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin}"
  "$INSTALL_DIR/bin/initdb" -D "$DATA_DIR" --locale=C --encoding=UTF8 -m "$INIT_MODE" --debug
)
runuser -u "$SERVICE_USER" -- bash -c "${INIT_CMD[*]}" > "$INIT_LOG" 2>&1 || { ... }

---

1035-1043: Service start failure path: include initdb tail automatically when start fails right after init.

Helps diagnose early failures without manual steps.

Append tail -n 100 "$INIT_LOG" || true to the failure branch.

---

1046-1062: db_is_ready: prefer discovered port and unix socket directory if available.

You parse port (get_pgport) but don’t use it; also hardcoding /tmp may miss distros using /run/postgresql.

Use the port from postmaster.pid and probe both /tmp and /run/postgresql.

---

915-933: .bash_profile overwrite risk.

Overwrites any existing service user profile. Append or write a dedicated file and source it instead.

Write $user_home/.ivorysql_env and append a source line if not present.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

autoinstall_script/AutoInstall.sh (3)

97-108: Stop sourcing ivorysql.conf; parse it safely instead.

Using . "$cfg" still executes arbitrary code via command substitutions despite the regex check. Please switch to a safe KEY=VALUE parser that trims whitespace, rejects invalid keys, and assigns via declare -g without evaluation.

Apply this diff:

-  STEP "Sourcing config"
-  if grep -Evq '^\s*([A-Z_][A-Z0-9_]*\s*=\s*.*|#|$)' "$cfg"; then
-    FAIL "Invalid lines detected in config (only KEY=VALUE, # comments, blanks)."
-  fi
-  . "$cfg"
+  STEP "Loading config"
+  if grep -Evq '^\s*([A-Z_][A-Z0-9_]*\s*=\s*[^#]*|#|$)' "$cfg"; then
+    FAIL "Invalid lines detected in config (only KEY=VALUE, # comments, blanks)."
+  fi
+  local raw_key raw_val key val
+  declare -g INSTALL_DIR DATA_DIR SERVICE_USER SERVICE_GROUP LOG_DIR REPO_URL TAG
+  while IFS='=' read -r raw_key raw_val; do
+    raw_key="${raw_key%%#*}"
+    raw_val="${raw_val%%#*}"
+    key="${raw_key#"${raw_key%%[![:space:]]*}"}"
+    key="${key%"${key##*[![:space:]]}"}"
+    [ -z "$key" ] && continue
+    val="${raw_val#"${raw_val%%[![:space:]]*}"}"
+    val="${val%"${val##*[![:space:]]}"}"
+    [[ "$val" =~ ^\".*\"$ ]] && val="${val:1:${#val}-2}"
+    [[ "$val" =~ ^\'.*\'$ ]] && val="${val:1:${#val}-2}"
+    case "$key" in
+      INSTALL_DIR|DATA_DIR|SERVICE_USER|SERVICE_GROUP|LOG_DIR|REPO_URL|TAG)
+        declare -g "$key=$val"
+        ;;
+      *)
+        WARN "Unknown config key ignored: $key"
+        ;;
+    esac
+  done < "$cfg"
   OK "Config loaded"

---

142-161: Handle Fedora explicitly during OS detection.

case "$OS_ID" still has no fedora branch, so Fedora hosts abort even though later logic assumes Fedora-style packaging. Please add fedora) selecting dnf (with version guard if needed) before the fallback.

Apply this diff:

   case "$OS_ID" in
     rhel|rocky|almalinux|oracle|ol|oraclelinux|centos)
       EL_MAJOR="$(echo "$OS_VER" | cut -d. -f1)"
       [[ "$EL_MAJOR" =~ ^(8|9|10)$ ]] || FAIL "Unsupported EL major: $EL_MAJOR"
       PKG="$(command -v dnf >/dev/null 2>&1 && echo dnf || echo yum)"
       ;;
+    fedora)
+      PKG="dnf"
+      ;;
     ubuntu)

---

368-369: Guard against recursive chown of system directories.

chown -R "$SERVICE_USER:$SERVICE_GROUP" "$INSTALL_DIR" will happily seize ownership of /usr, /opt etc. if misconfigured, breaking the OS. Fail fast when INSTALL_DIR is a sensitive path (/, /usr, /var, /bin, …) before running chown.

Apply this diff:

   STEP "Install"
   (cd "$src_root" && make install) || FAIL "Install failed"
-  chown -R "$SERVICE_USER:$SERVICE_GROUP" "$INSTALL_DIR" || true
+  case "$INSTALL_DIR" in
+    /|/usr|/usr/*|/bin|/sbin|/lib|/lib64|/etc|/var|/opt|/opt/*)
+      FAIL "Unsafe INSTALL_DIR for chown: $INSTALL_DIR"
+      ;;
+  esac
+  chown -R "$SERVICE_USER:$SERVICE_GROUP" "$INSTALL_DIR" || true
   OK "Installed to $INSTALL_DIR"

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

autoinstall_script/README.md (1)

79-92: Use a heading instead of bold text for the “Examples” section.

Switching **Examples** to a proper heading (e.g., ### Examples) matches the surrounding structure and clears the MD036 lint warning about emphasis masquerading as a heading.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

autoinstall_script/AutoInstall.sh (2)

97-102: Critical: Replace unsafe source with a safe config parser.

Line 101 uses source "$cfg", which executes arbitrary shell code including command substitutions. This is a code execution vulnerability if the config file is compromised. Parse the file line-by-line and assign validated key-value pairs instead.

Apply this diff:

   STEP "Sourcing config"
   if grep -Evq '^\s*([A-Z_][A-Z0-9_]*\s*=\s*.*|#|$)' "$cfg"; then
     FAIL "Invalid lines detected in config (only KEY=VALUE, # comments, blanks)."
   fi
-  . "$cfg"
-  OK "Config loaded"
+  # Safe parse: read line-by-line, no command substitution evaluation
+  while IFS='=' read -r key value || [ -n "$key" ]; do
+    # Strip leading/trailing whitespace
+    key="$(echo "$key" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')"
+    value="$(echo "$value" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')"
+    # Skip blank lines and comments
+    [[ -z "$key" || "$key" =~ ^# ]] && continue
+    # Whitelist known keys
+    case "$key" in
+      INSTALL_DIR|DATA_DIR|SERVICE_USER|SERVICE_GROUP|LOG_DIR)
+        declare -g "$key=$value"
+        ;;
+      *)
+        WARN "Unknown config key: $key"
+        ;;
+    esac
+  done < "$cfg"
+  OK "Config loaded (safe parse)"

---

366-369: Add safety guard before recursive chown.

Line 368 performs chown -R on INSTALL_DIR without verifying it's not a sensitive system path. Validate before recursing to prevent accidental permission changes on /, /usr, etc.

Apply this diff:

   STEP "Install"
   (cd "$src_root" && make install) || FAIL "Install failed"
+  # Safety: prevent chown on system directories
+  case "$INSTALL_DIR" in
+    ""|"/"|"/usr"|"/usr/bin"|"/usr/lib"|"/var"|"/etc"|"/bin"|"/sbin")
+      FAIL "Refusing to chown system directory: $INSTALL_DIR"
+      ;;
+  esac
   chown -R "$SERVICE_USER:$SERVICE_GROUP" "$INSTALL_DIR" || true

autoinstall_script/flake.nix (1)

10-11: Consider adding aarch64-linux and darwin support.

The flake currently hardcodes x86_64-linux. To improve portability for developers on ARM64 Linux or macOS, extend outputs to iterate over multiple systems.

Apply this diff to support additional platforms:

-      system = "x86_64-linux";
-      pkgs = nixpkgs.legacyPackages.${system};
+      systems = [ "x86_64-linux" "aarch64-linux" "x86_64-darwin" "aarch64-darwin" ];
+      forAllSystems = f: nixpkgs.lib.genAttrs systems (system: f system);
     in
     {
-      devShells.${system}.default = pkgs.mkShell {
+      devShells = forAllSystems (system:
+        let pkgs = nixpkgs.legacyPackages.${system};
+        in {
+          default = pkgs.mkShell {
         buildInputs = with pkgs; [
           # Core compiler and build tools
           gcc
@@ -40,5 +43,6 @@
           curl
         ];
       };
+        });
     };
 }

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

autoinstall_script/AutoInstall.sh (1)

158-177: Fedora is not explicitly supported.

The OS detection handles EL derivatives, Ubuntu, and Debian, but Fedora (which uses dnf) is not explicitly listed. Fedora installations will fail at line 175 with "Unsupported or unrecognized OS."

Add explicit Fedora support:

   case "$OS_ID" in
     rhel|rocky|almalinux|oracle|ol|oraclelinux|centos)
       EL_MAJOR="$(echo "$OS_VER" | cut -d. -f1)"
       [[ "$EL_MAJOR" =~ ^(8|9|10)$ ]] || FAIL "Unsupported EL major: $EL_MAJOR"
       PKG="$(command -v dnf >/dev/null 2>&1 && echo dnf || echo yum)"
       ;;
+    fedora)
+      PKG="dnf"
+      ;;
     ubuntu)

autoinstall_script/AutoInstall.sh (3)

256-263: Consider warning users before using --allowerasing.

The --allowerasing flag can remove conflicting packages without warning. While necessary for handling conflicts (e.g., curl vs. curl-minimal), users might appreciate a heads-up.

Add an informational message:

+     STEP "Installing packages (may replace conflicting packages)"
      if ! dnf install -y --allowerasing "${EL_PKGS[@]}"; then

---

435-455: Consider adding TimeoutStartSec to the systemd unit.

The systemd unit currently relies on the default TimeoutStartSec (90s), but you're configuring pg_ctl with a custom ${READY_TIMEOUT}. Aligning the systemd timeout prevents systemd from killing the service if startup takes longer than 90s but less than your configured timeout.

 ExecStart=$INSTALL_DIR/bin/pg_ctl start  -D \${PGDATA} -s -w -t ${READY_TIMEOUT}
 ExecStop=$INSTALL_DIR/bin/pg_ctl stop   -D \${PGDATA} -s -m fast
 ExecReload=$INSTALL_DIR/bin/pg_ctl reload -D \${PGDATA}
+TimeoutStartSec=$((${READY_TIMEOUT} + 10))
 Restart=on-failure

---

500-506: Readiness check could be more efficient.

The current implementation spawns three su processes per iteration (one for each socket location). While correct, this adds overhead during startup verification.

Consider checking all locations in a single subshell:

   for _ in $(seq 1 "$READY_TIMEOUT"); do
-    su -s /bin/bash - "$SERVICE_USER" -c "env PATH='$INSTALL_DIR/bin:\$PATH' $INSTALL_DIR/bin/pg_isready -q -h /tmp -p $port -t 1" \
-      || su -s /bin/bash - "$SERVICE_USER" -c "env PATH='$INSTALL_DIR/bin:\$PATH' $INSTALL_DIR/bin/pg_isready -q -h /var/run/postgresql -p $port -t 1" \
-      || su -s /bin/bash - "$SERVICE_USER" -c "env PATH='$INSTALL_DIR/bin:\$PATH' $INSTALL_DIR/bin/pg_isready -q -h 127.0.0.1 -p $port -t 1" \
-      && { ok=1; break; }
+    su -s /bin/bash - "$SERVICE_USER" -c "env PATH='$INSTALL_DIR/bin:\$PATH' $INSTALL_DIR/bin/pg_isready -q -h /tmp -p $port -t 1 || $INSTALL_DIR/bin/pg_isready -q -h /var/run/postgresql -p $port -t 1 || $INSTALL_DIR/bin/pg_isready -q -h 127.0.0.1 -p $port -t 1" \
+      && { ok=1; break; }
     sleep 1

autoinstall_script/flake.nix (1)

5-5: Consider documenting the nixos-unstable choice.

Using nixos-unstable provides the latest packages but can introduce breaking changes. For a more stable development environment, consider mentioning in the README that users can override to a stable channel if needed.

Alternatively, consider using a recent stable release:

   inputs = {
-    nixpkgs.url = "github:NixOS/nixpkgs/nixos-unstable";
+    nixpkgs.url = "github:NixOS/nixpkgs/nixos-24.05";  # or nixos-unstable
   };

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

autoinstall_script/AutoInstall.sh (1)

158-177: Major: Fedora is not explicitly supported in OS detection.

Fedora (a mainstream, actively maintained distribution) is not listed in the OS_ID case statement. Users running on Fedora will see "Unsupported OS" and the script will exit. Since Fedora uses dnf like modern RHEL/Rocky, adding explicit support is straightforward.

Add Fedora to the OS detection case:

  case "$OS_ID" in
-   rhel|rocky|almalinux|oracle|ol|oraclelinux|centos)
+   fedora|rhel|rocky|almalinux|oracle|ol|oraclelinux|centos)
       EL_MAJOR="$(echo "$OS_VER" | cut -d. -f1)"
-      [[ "$EL_MAJOR" =~ ^(8|9|10)$ ]] || FAIL "Unsupported EL major: $EL_MAJOR"
+      [[ "$EL_MAJOR" =~ ^(8|9|10|39|40)$ ]] || FAIL "Unsupported EL/Fedora major: $EL_MAJOR"
       PKG="$(command -v dnf >/dev/null 2>&1 && echo dnf || echo yum)"
       ;;

And in enable_el_repos(), include Fedora in the case statement so it skips EL-specific repo commands (Fedora doesn't use CRB/EPEL):

  case "$OS_ID" in
-   rocky|almalinux|centos)
+   fedora)
+     : # Fedora uses standard repos; skip EPEL/CRB/PowerTools setup
+     ;;
+   rocky|almalinux|centos)
       if cmd_exists dnf; then

autoinstall_script/AutoInstall.sh (2)

108-124: Optional: Clarify unused configuration keys.

The config validation whitelist (line 109–116) only includes INSTALL_DIR, DATA_DIR, SERVICE_USER, SERVICE_GROUP, and LOG_DIR. However, earlier review discussions mention REPO_URL and TAG as configuration keys. If these are meant to be supported, they should be added to the whitelist and used in the script (e.g., to override the source repository). If not, document that they are intentionally unsupported or remove references to them.

This is a minor clarity issue; if the config file is simple and doesn't include these keys, no action is needed.

---

515-516: Optional: Escape special characters in sed substitutions for robustness.

The sed command on line 515 substitutes unquoted variables into patterns. While path validation reduces the risk (paths must start with / and contain no spaces), paths containing & or \ could break sed semantics. For defense-in-depth, either pre-escape the variables or use -- to prevent interpretation:

+   # Escape special sed characters: & \ and /
+   DATA_DIR_ESC="$(printf '%s\n' "$DATA_DIR" | sed -e 's/[\/&]/\\&/g')"
+   INSTALL_DIR_ESC="$(printf '%s\n' "$INSTALL_DIR" | sed -e 's/[\/&]/\\&/g')"
+   LOG_DIR_ESC="$(printf '%s\n' "$LOG_DIR" | sed -e 's/[\/&]/\\&/g')"
-    sed -i "s#__PGDATA__#$DATA_DIR#g; s#__INSTALL__#$INSTALL_DIR#g; s#__LOG__#$LOG_DIR#g; s#__READY_TIMEOUT__#${READY_TIMEOUT}#g" "$INSTALL_DIR/ivorysql-ctl"
+    sed -i "s#__PGDATA__#${DATA_DIR_ESC}#g; s#__INSTALL__#${INSTALL_DIR_ESC}#g; s#__LOG__#${LOG_DIR_ESC}#g; s#__READY_TIMEOUT__#${READY_TIMEOUT}#g" "$INSTALL_DIR/ivorysql-ctl"

This is a low-risk defensive improvement; given your path validation, it's not urgent.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

autoinstall_script/AutoInstall.sh (4)

98-98: Regex pattern uses \s which may not be portable across all grep implementations.

Line 98 uses \s for whitespace in the ERE pattern. While supported by GNU grep, POSIX ERE does not guarantee \s support on all systems. For maximum portability, use [[:space:]] instead.

Apply this diff:

-if grep -Evq '^\s*([A-Z_][A-Z0-9_]*\s*=\s*.*|#|$)' "$cfg"; then
+if grep -Evq '^[[:space:]]*([A-Z_][A-Z0-9_]*[[:space:]]*=[[:space:]]*.*|#|$)' "$cfg"; then

---

334-335: Hardcoded apt-get command should use $PKG variable for consistency.

Line 334 calls apt-get install directly instead of using the $PKG variable established at line 167. While this works within the apt-get) case branch, it is inconsistent with the goal of centralizing package manager logic. For maintainability, use $PKG like the dnf/yum branches do.

Apply this diff:

-    apt-get install -y --no-install-recommends "${APT_PKGS[@]}" \
+    $PKG install -y --no-install-recommends "${APT_PKGS[@]}" \
       || FAIL "Package installation failed on Debian/Ubuntu"

---

356-365: Perl module availability check is thorough, but consider logging the fallback path.

Lines 359–365 attempt to install Perl with fallbacks (dnf install / yum install / apt-get install), then re-check. While robust, there is no log message indicating which fallback was used or why. Consider adding a WARN or STEP message to make troubleshooting clearer if the fallback is needed.

---

544-555: Readiness polling loop is robust but could be simplified.

Lines 547–553 poll pg_isready three times (with /tmp, /var/run/postgresql, and 127.0.0.1) to handle different socket/network configurations. While thorough, consider documenting why all three are needed or consolidating to the most common path. The loop respects READY_TIMEOUT and includes a final status check if the timeout is exceeded.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

autoinstall_script/README.md (1)

58-58: Use hyphenated "command-line" for the compound adjective.

Per grammar conventions, compound adjectives should be hyphenated when they precede the noun.

Apply:

-The AutoInstall.sh script supports the following command line options:
+The AutoInstall.sh script supports the following command-line options:

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro