refactor(inject): format script and check for errors

[skevetter]

• refactor(inject): format script and check for errors
• fix: apply feedback comments
• fix: quote variables

Summary by CodeRabbit

• New Features

  • Installer refactored into clear handshake → privilege setup → install → finalize flow with robust retries and atomic staging.
  • Safer binary staging via temporary-file writes and atomic moves; installer exports the install URL and signals completion.
  • Automatic fallback between download and injection install methods.

• Bug Fixes

  • Improved privilege-elevation detection and error handling.
  • Expanded error messages and better cleanup of temporary artifacts on failure.

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactors pkg/inject/inject.sh into a modular installer flow with handshake, inject_binary, download_binary, setup_sudo, install_agent, execute_command, and main, reorganizing handshake, privilege handling, binary delivery with retries/fallbacks, and centralized final execution.

Changes

Cohort / File(s)	Summary
Injector script (refactor & new flow) pkg/inject/inject.sh	Replaces monolithic installer with modular functions: handshake(), inject_binary(), download_binary(), setup_sudo(), install_agent(), execute_command(), and main(). Adds temp-file write/move patterns, download retries using curl/wget, sudo/su detection, fallback between inject/download, expanded error reporting, and centralized final execution flow.

Sequence Diagram(s)

sequenceDiagram
participant Client
participant Injector as "inject.sh"
participant Remote as "remote server/URL"
participant Priv as "sudo/su"
participant Parent as "parent process"

Client->>Injector: invoke main()
Injector->>Injector: handshake() - send ping, expect pong
Injector->>Priv: setup_sudo() - determine sudo or su
alt download path
    Injector->>Remote: download_binary() (curl/wget, retries)
    Remote-->>Injector: binary data
    Injector->>Injector: write to temp file -> move to target
else inject path
    Client->>Injector: inject_binary() (receive binary stream)
    Injector->>Injector: write to temp file -> move to target
end
Injector->>Priv: install_agent() (apply chmod via sudo/su if required)
Injector->>Parent: execute_command() (export URL, run configured command)
Parent-->>Injector: command exit/status
Injector->>Client: final status / done

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'refactor(inject): format script and check for errors' only partially captures the main changes—the PR is primarily about modularizing an installer script into reusable functions, but the title emphasizes formatting and error-checking without mentioning the refactoring structure.

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

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch refactor-inject-script-2

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

pkg/inject/inject.sh (1)

68-106: EXIT trap set inside loop is never cleared after successful mv.

The trap 'rm -f "$temp_file"' EXIT on line 70 is overwritten each iteration and persists after the loop. After line 100 successfully moves the temp file to INSTALL_PATH, the trap remains armed. On script exit it will attempt rm -f on the (now non-existent) temp path — harmless due to -f, but it would be cleaner to disarm it after the move succeeds.

Suggested improvement

     if ! $sh_c "mv \"$temp_file\" \"$INSTALL_PATH\""; then
         >&2 echo "Error: Failed to move downloaded binary to $INSTALL_PATH"
         $sh_c "rm -f \"$temp_file\"" 2>/dev/null || true
         return 1
     fi
+    trap - EXIT

     return 0

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@pkg/inject/inject.sh`:
- Around line 78-91: The code captures cmd_status after an if compound which
yields 0, so replace the "if $sh_c \"curl ...\"; then break; fi; cmd_status=$?"
and the analogous wget block with a short-circuit form that preserves the actual
tool exit code, e.g. invoke $sh_c "curl -fsSL \"$DOWNLOAD_URL\" -o
\"$temp_file\"" && break || cmd_status=$? (and do the same for wget),
referencing the existing symbols DOWNLOAD_URL, temp_file, cmd_status, sh_c,
command_exists, and the curl/wget branches to locate where to change.

🧹 Nitpick comments (2)

pkg/inject/inject.sh (2)

42-42: Minor: mktemp fallback doesn't create the file and reuses the same name across retries.

When mktemp fails, the fallback echo "$INSTALL_PATH.$$" just produces a filename string without creating it — the subsequent cat > or curl -o will create it, so it works. However, $$ is constant for the process, so in download_binary's retry loop every iteration collides on the same fallback name. This is benign (the file is cleaned up between attempts) but worth noting.

Also applies to: 76-76

---

142-166: Duplicate chmod +x when using inject_binary path.

inject_binary() already runs chmod +x on $INSTALL_PATH (lines 56–61), and then install_agent() does it again (lines 161–166). For the download_binary path the outer chmod is needed, but for inject_binary it's redundant.

You could remove the chmod from inject_binary() and let install_agent() be the single place that handles it, since it applies after both paths.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@pkg/inject/inject.sh`:
- Around line 77-80: The temp file created with mktemp (temp_file) is leaked
when neither curl nor wget is found because the function returns 127 without
removing it; modify the logic around the download-tool check so that before
executing `return 127` the temp_file is removed (rm -f "$temp_file") or, better,
ensure a cleanup trap is set earlier (trap 'rm -f "$temp_file"' EXIT) so
temp_file is always deleted regardless of the early return in the download
branch that currently returns 127.

🧹 Nitpick comments (2)

pkg/inject/inject.sh (2)

82-82: Cleanup rm under set -e could abort the retry loop unexpectedly.

If $sh_c "rm -f ..." fails (e.g., directory permissions changed), set -e terminates the script immediately, bypassing the retry logic and error messages below. The same pattern appears in inject_binary (lines 46, 52) and later on line 101. Adding || true makes cleanup best-effort and keeps the intended error flow intact.

Proposed fix (apply similarly to lines 46, 52, 101)

-        $sh_c "rm -f \"$temp_file\""
+        $sh_c "rm -f \"$temp_file\"" 2>/dev/null || true

---

158-162: Quote the template value on line 160 to guard against shell metacharacters.

The ShellCheck warnings (SC1083/SC1054) on lines 160–161 and 167 are false positives — they fire because ShellCheck cannot parse Go template {{ }} braces. These will be replaced before the script executes.

However, the rendered URL on line 160 should be quoted to prevent word-splitting or glob expansion if the value ever contains special characters:

Proposed fix

-    export DEVPOD_AGENT_URL={{ .DownloadBase }}
+    export DEVPOD_AGENT_URL="{{ .DownloadBase }}"

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@pkg/inject/inject.sh`:
- Line 161: The export of DEVPOD_AGENT_URL in inject.sh uses an unquoted
template expansion which can cause word splitting or globbing if {{
.DownloadBase }} contains spaces or shell-special chars; update the export so
the template expansion is quoted (i.e., assign the value in quotes) to ensure
the entire string is preserved and safe for shell export when setting
DEVPOD_AGENT_URL.