Uh oh!
There was an error while loading. Please reload this page.
feat: Add systemd-managed pg-node service with secure API and core version flag - #11
Conversation
WalkthroughAdds a new TLS HTTPS REST-like server script ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenSSL as OpenSSL s_server
participant Script as pg-node-service.sh
participant Host
rect rgb(235,245,255)
Note over Client,OpenSSL: TLS handshake & HTTPS request
end
Client->>OpenSSL: HTTPS request
OpenSSL->>Script: Request delivered via stdin
rect rgb(255,250,235)
Note over Script: Parse request line, headers, optional body
Script->>Script: Validate x-api-key header
alt API key invalid
Script->>OpenSSL: 401 Unauthorized JSON
else API key valid
alt GET /
Script->>OpenSSL: 200 {"status":"ok"}
else POST /node/update
Script->>Host: Exec APP_NAME update
Script->>OpenSSL: 200 JSON result
else POST /node/core_update
Script->>Script: Parse JSON body (uses jq if present)
Script->>Host: Exec APP_NAME core-update [--version]
Script->>OpenSSL: 200/500 JSON result
else POST /node/geofiles
Script->>Script: Parse region from JSON
Script->>Host: Exec APP_NAME geofiles --region ...
Script->>OpenSSL: 200/500 JSON result
end
end
end
OpenSSL->>Client: HTTPS response
Script->>OpenSSL: Restart coprocess for next connection
sequenceDiagram
participant User
participant CLI as pg-node.sh
participant systemd
participant Firewall
participant Service as pg-node-service.sh
participant Host
User->>CLI: install_service_command
CLI->>CLI: set_service_paths & require_systemd
CLI->>CLI: choose API_PORT / read ENV
CLI->>Firewall: configure_firewall_for_port(API_PORT)
CLI->>CLI: install_node_service_script (write unit & script)
CLI->>systemd: systemctl daemon-reload / enable / start unit
systemd->>Service: start pg-node-service.sh
Service->>Service: load ENV_FILE, validate cert/key, start s_server
User->>CLI: service-status
CLI->>systemd: systemctl status <unit>
systemd-->>User: status output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
pg-node-service.sh (3)
36-42: Verify environment file fallback behavior is intentional.Lines 32-34 prioritize a local
.envover the default one if it exists. This is useful for development but could be a security concern if an attacker can write a local.env. Ensure this is intentional or document the precedence clearly in a comment.Add a clarifying comment:
if [[ ! -f "$ENV_FILE" && -f "$LOCAL_ENV_FILE" ]]; then + # Local .env takes precedence (useful for development; ensure directory is protected) ENV_FILE="$LOCAL_ENV_FILE" fi
148-162: HTTP header parsing is case-sensitive forx-api-keybut not forContent-Length.Line 152 correctly matches
Content-Lengthcase-insensitively, but line 159 checksx-api-keyin lowercase only. While HTTP headers are case-insensitive per RFC 7230, this inconsistency could lead to bugs. Line 157 lowercases the header name, so line 159 is correct, but the mixed approach is fragile.Ensure consistent case-insensitive header matching:
header_name=${header_line%%:*} header_value=${header_line#*:} + # Normalize header name to lowercase for consistent matching header_name=${header_name,,} header_value=${header_value# } if [[ "$header_name" == "x-api-key" ]]; then x_api_key="$header_value" fiThis is already implemented correctly; consider adding a comment for clarity.
71-88: json_escape and status_text are minimal but functional.The
json_escapefunction correctly escapes common JSON special characters. However, it doesn't escape control characters (0x00-0x1F except those handled). For untrusted input, ensure the function is sufficient or add additional escaping.Consider using
jqfor robust JSON escaping if it's already a dependency, or add control character escaping:json_escape() { local s=$1 s=${s//\\/\\\\} s=${s//\"/\\\"} s=${s//$'\n'/\\n} s=${s//$'\r'/\\r} + # Optionally escape other control characters+ # s=${s//$'\t'/\\t} # tab echo -n "$s" }pg-node.sh (3)
1439-1463: core-update with --version flag relies on global variable mutation.The
update_core_commandfunction callsget_xray_core "$core_version_arg"(line 1463), which updates a globalselected_versionvariable internally (visible in the function at line 1491). This implicit state mutation is fragile and makes the code harder to reason about.Consider returning the selected version explicitly or storing it in a local variable. Alternatively, document the global mutation clearly:
update_core_command() { check_running_as_root local core_version_arg="" + local selected_version="" # Declared locally; will be set by get_xray_core while [[ $# -gt 0 ]]; do case "$1" inOr modify
get_xray_coreto output the version instead of relying on a global:- get_xray_core "$core_version_arg"+ selected_version=$(get_xray_core "$core_version_arg")This requires refactoring
get_xray_coreto output the version, but improves clarity.
1537-1537: Usage help references service commands but doesn't document required flags.The usage documentation (lines 1537, 1586-1589, 1599-1600) correctly lists the new service-related commands and the
--versionflag for core-update. However, it lacks:
- Explanation of systemd vs. Docker-only setups
- Clarification that service commands require root
- Examples of service-status or service-restart usage
Enhance the help text with examples or prerequisites:
colorized_echo yellow " service-install $(tput sgr0)– Install and start pg-node-service (systemd)" colorized_echo yellow " service-uninstall $(tput sgr0)– Remove pg-node-service (systemd)" colorized_echo yellow " service-restart $(tput sgr0)– Restart pg-node-service (systemd)" colorized_echo yellow " service-status $(tput sgr0)– Show pg-node-service status" + echo " (Note: service-* commands require systemd and root privileges)"Also applies to: 1586-1589, 1599-1600
1269-1277: Version validation in get_xray_core uses API calls but lacks retry or fallback.Lines 1269-1277 handle the
--versionflag, but if the API call to validate the version fails (network issue, rate limit), the script exits without fallback. Thevalidate_versionfunction (lines 1231-1240) uses a single curl call with no retries.Add basic retry logic or cache mechanism:
validate_version() { local version="$1" + local attempts=0+ while (( attempts < 2 )); do+ local response=$(curl -s --max-time 5 "https://api.github.com/repos/XTLS/Xray-core/releases/tags/$version")+ if [ $? -eq 0 ]; then+ if echo "$response" | grep -q '"message": "Not Found"'; then+ echo "invalid"+ else+ echo "valid"+ fi+ return+ fi+ (( attempts++ ))+ sleep 1+ done+ echo "unknown" # Fallback on network failure }Then handle the "unknown" case to allow user to proceed or retry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pg-node-service.sh(1 hunks)pg-node.sh(18 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
pg-node-service.sh
[warning] 145-145: version appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 207-207: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 207-207: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 208-208: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 208-208: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
🔇 Additional comments (4)
pg-node.sh (3)
814-816: Service uninstall properly integrated into uninstall flow.The uninstall command checks if the service is installed and calls
uninstall_service_commandbefore cleaning up other components. This order is correct and prevents orphaned systemd units.
941-1015: I'll begin verification of the review comment regarding the API port selection logic in pg-node.sh. Let me start by examining the actual code to verify the concerns raised.
<function_calls>
shell
#!/bin/bashFirst, let's locate and examine the pg-node.sh file
find . -name "pg-node.sh" -type f 2>/dev/null | head -5
</function_calls>
<function_calls>
shell
#!/bin/bashGet the line count and examine the relevant section
wc -l pg-node.sh
echo "---"Display lines 950-990 to see the port selection logic
sed -n '950,990p' pg-node.sh
</function_calls>
119-174: The suggested fallback tosystemctl is-enabledis semantically incorrect and would break functionality.The
service_installed()function checks whether a service exists/is installed, not whether it's enabled. The suggested fallback tosystemctl is-enabledonly returns success if the service is in an enabled state—it would fail to detect disabled services that are still installed. This changes the function's purpose from detecting installation to detecting enablement.The current implementation is already robust: it uses a filesystem check (
[ -f "$SERVICE_UNIT" ]) as the primary method, which is compatible across all systemd versions, withsystemctl list-unit-filesas a fallback for edge cases. The filesystem check is the most reliable and version-agnostic approach and should remain the primary detection method.pg-node-service.sh (1)
175-185: I'll help you verify this review comment about request body size validation and potential hanging in the bash script. Let me start by examining the file and understanding the actual code behavior.
<function_calls>First, find and examine the pg-node-service.sh file
find . -name "pg-node-service.sh" -type f
</function_calls>
<function_calls>Get line count to understand file size
wc -l pg-node-service.sh
</function_calls>
<function_calls>Examine the specific section (lines 175-185) and surrounding context
sed -n '165,195p' pg-node-service.sh | cat -n
</function_calls>
<function_calls>Search for all instances of 'read' command in the file to understand patterns
rg -n "read\s+" pg-node-service.sh | head -20
</function_calls>
<function_calls>Check for timeout usage or any other timeout mechanisms
rg -n "timeout|TIMEOUT" pg-node-service.sh
</function_calls>
<function_calls>Look at the full function/context where this code is located
sed -n '150,200p' pg-node-service.sh | cat -n
</function_calls>Please provide the results of these verification queries so I can assess whether the review comment's concerns are valid and whether the proposed fix is appropriate.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pg-node.sh (1)
993-1014: Systemd unit lacks process limits and restart constraints.The unit file (lines 994–1014) includes good start/stop timeouts and restart rate limits (added from previous feedback), but lacks resource limits. Without memory/CPU bounds, a service loop or resource leak could exhaust system resources.
Consider adding:
[Service] Type=simple ExecStart=$SERVICE_SCRIPT_PATH WorkingDirectory=$APP_DIR Restart=on-failure RestartSec=5 StartLimitInterval=600 StartLimitBurst=3 TimeoutStartSec=30 TimeoutStopSec=10 +MemoryLimit=512M+CPUQuota=50% Environment="ENV_FILE=$ENV_FILE"Adjust limits based on expected workload. This prevents cascading failures and runaway processes.
pg-node-service.sh (1)
138-138: Incorrect HTTP status code forcore-updatefailure: use 500, not 404.HTTP 404 (Not Found) is semantically incorrect for execution failures. Line 116 correctly uses HTTP 500 for server errors; apply the same pattern here.
- respond 404 "{\"detail\":\"core-update failed for version $(json_escape "$core_version")\"}"+ respond 500 "{\"detail\":\"core-update failed for version $(json_escape "$core_version")\"}"
🧹 Nitpick comments (1)
pg-node-service.sh (1)
207-217: OpenSSL coproc spawned per request: consider persistent server architecture.Spawning a new OpenSSL s_server coproc per connection (line 209) incurs repeated TLS handshakes, increases resource overhead, and creates restart churn. While the cleanup improvements (lines 214–216) reduce zombie risk, this architecture doesn't scale well.
For production, consider:
- A single long-lived TLS listener (persistent coproc outside the loop)
- Or migrate to a lightweight HTTP server library (e.g., Python's http.server over TLS)
- Or document this as a development/testing design and add observability to detect resource leaks
If keeping the per-request model, ensure your trap handlers (EXIT, INT, TERM) are installed to guarantee cleanup on script termination.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pg-node-service.sh(1 hunks)pg-node.sh(18 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
pg-node-service.sh
[warning] 149-149: version appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 211-211: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 211-211: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 212-212: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 212-212: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
🔇 Additional comments (7)
pg-node-service.sh (1)
149-149: Unused variableversionfrom parsed request line.Line 149 reads
versionfrom the HTTP request line but it's never used. If HTTP/2 or chunked encoding support is planned, capture it; otherwise, remove it to reduce confusion.pg-node.sh (6)
1273-1281: Requested version handling inget_xray_corelooks solid.The logic correctly prioritizes an explicit
--versionarg, validates it against GitHub releases, and falls back to interactive menu on user input. The "latest" shortcut (line 1274–1275) is clean.
1441-1467:update_core_commandcorrectly parses and forwards--versionflag.The --version option handling (lines 1445–1465) is well-structured, with proper error messages and help text. Forwarding to
get_xray_coreat line 1467 is correct.
771-781: Service installation prompt placement ininstall_commandis good UX.Post-install prompt (lines 771–781) follows natural flow: node installed → ask to install service. AUTO_CONFIRM behavior (default "y") is sensible. User opt-out is respected.
119-174: Service helper functions are well-organized and defensive.Functions like
require_systemd()(line 125),service_installed()(line 132), andrestart_service_if_installed()(line 143) handle missing systemd gracefully and check permissions. The code is readable and maintainable.
960-970: API_PORT selection logic is sound.Extracting existing port from env (lines 960–962), validating range (lines 964–965), checking occupancy (lines 965–970), and generating random fallback (lines 973–981) covers edge cases well. The user-facing messages are clear.
1541-1541: Completion commands list updated correctly.Shell completions (line 1541) now include service-install, service-uninstall, service-restart, service-status. No drift detected.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pg-node.sh (2)
961-968: Improve API_PORT extraction robustness.The sed extraction at line 962 could fail on edge cases (e.g., tabs instead of spaces around
=). Consider using a more robust parser.- if existing_api_port=$(grep -E '^API_PORT[[:space:]]*=' "$ENV_FILE" | head -n1 | sed 's/^API_PORT[[:space:]]*=[[:space:]]*//'); then- existing_api_port=$(echo "$existing_api_port" | tr -d '"'\')+ if existing_api_port=$(grep -E '^API_PORT[[:space:]]*=' "$ENV_FILE" | head -n1 | cut -d= -f2- | xargs); then+ existing_api_port=$(echo "$existing_api_port" | tr -d '"'\''') fiThe
cutandxargscombination is simpler and more forgiving of whitespace variations.
995-1006: Consider consolidating API_PORT write logic.Lines 995–1006 handle two cases (update vs. append) for API_PORT. This could be simplified by always appending and letting the service use the last occurrence, or by using a dedicated configuration function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pg-node.sh(18 hunks)
🔇 Additional comments (8)
pg-node.sh (8)
119-174: Service helper functions look solid.The defensive approach to checking service installation (both file and systemctl) and the idempotent restart/update patterns are good. Note that
service_installedis called multiple times in some flows; if performance becomes an issue, consider caching the result within a single command execution.
1013-1033: Systemd unit configuration includes timeout protections.Excellent—the prior concern about missing
TimeoutStartSec,TimeoutStopSec,StartLimitInterval, andStartLimitBursthas been addressed. This ensures the service won't hang indefinitely and won't enter rapid restart loops. The configuration is solid.
1764-1775: Service command dispatch is clean and consistent.Follows the established pattern in the codebase.
1249-1332: Core version selection refactoring is solid.The optional parameter support for
requested_versionmaintains backward compatibility while enabling programmatic version selection. Validation via GitHub API is consistent with the codebase.
1460-1486: Core-update command-line interface is clean.The
--versionflag with support forlatestand specific versions improves UX significantly. Help text is clear.
771-781: Install service prompt is well-designed.The post-install service installation prompt respects
AUTO_CONFIRMand provides sensible defaults. Good UX.
1560-1560: Completion commands updated correctly.Added service commands to bash completion list. Ensure the list matches the case statement in the main dispatcher (it does).
1609-1612: Help text and core-update documentation enhanced.The new service commands and
--versionoption for core-update are properly documented in the help output.Also applies to: 1623-1623
| install_node_service_script() { | ||
| set_service_paths | ||
| colorized_echo blue "Installing node service script" | ||
| curl -sSL $SERVICE_SCRIPT_URL -o "$SERVICE_SCRIPT_PATH" | ||
| sed -i "s/^APP_NAME=.*/APP_NAME=\"$APP_NAME\"/" "$SERVICE_SCRIPT_PATH" | ||
| chmod 755 "$SERVICE_SCRIPT_PATH" | ||
| colorized_echo green "node service script installed successfully at $SERVICE_SCRIPT_PATH" | ||
| } |
There was a problem hiding this comment.
Add error handling to service script download.
Line 273 downloads the service script but doesn't check if the curl command succeeds. A failed download would result in an empty or partial file, causing cryptic failures later when systemd tries to start it.
Apply this diff to add error checking:
install_node_service_script() {
set_service_paths
colorized_echo blue "Installing node service script"
- curl -sSL $SERVICE_SCRIPT_URL -o "$SERVICE_SCRIPT_PATH"+ if ! curl -sSL "$SERVICE_SCRIPT_URL" -o "$SERVICE_SCRIPT_PATH"; then+ colorized_echo red "Failed to download service script from $SERVICE_SCRIPT_URL"+ exit 1+ fi
sed -i "s/^APP_NAME=.*/APP_NAME=\"$APP_NAME\"/" "$SERVICE_SCRIPT_PATH"
chmod 755 "$SERVICE_SCRIPT_PATH"
colorized_echo green "node service script installed successfully at $SERVICE_SCRIPT_PATH"
}📝 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.
| install_node_service_script() { | |
| set_service_paths | |
| colorized_echo blue "Installing node service script" | |
| curl -sSL $SERVICE_SCRIPT_URL -o "$SERVICE_SCRIPT_PATH" | |
| sed -i "s/^APP_NAME=.*/APP_NAME=\"$APP_NAME\"/""$SERVICE_SCRIPT_PATH" | |
| chmod 755 "$SERVICE_SCRIPT_PATH" | |
| colorized_echo green "node service script installed successfully at $SERVICE_SCRIPT_PATH" | |
| } | |
| install_node_service_script() { | |
| set_service_paths | |
| colorized_echo blue "Installing node service script" | |
| if! curl -sSL "$SERVICE_SCRIPT_URL" -o "$SERVICE_SCRIPT_PATH";then | |
| colorized_echo red "Failed to download service script from $SERVICE_SCRIPT_URL" | |
| exit 1 | |
| fi | |
| sed -i "s/^APP_NAME=.*/APP_NAME=\"$APP_NAME\"/""$SERVICE_SCRIPT_PATH" | |
| chmod 755 "$SERVICE_SCRIPT_PATH" | |
| colorized_echo green "node service script installed successfully at $SERVICE_SCRIPT_PATH" | |
| } |
🤖 Prompt for AI Agents
In pg-node.sh around lines 270 to 277, the curl download at line 273 needs
robust error handling: invoke curl with --fail (or equivalent) so it returns
non-zero on HTTP errors, check its exit status immediately, and only proceed to
sed/chmod if the download succeeded; on failure remove any partial file at
$SERVICE_SCRIPT_PATH, emit a clear error message and exit non-zero. Ensure the
sed replacement and chmod are conditional on the successful download.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pg-node-service.sh (1)
114-141: ** HTTP 404 semantically incorrect for execution failure—use 500.**Line 138 responds with HTTP 404 (Not Found) when
core-updatefails. This is semantically incorrect; 404 means a resource doesn't exist, not that a server-side command execution failed. Other error responses in this file use HTTP 500 for execution failures (line 120, line 182). Align the response code for consistency and correctness.if $APP_NAME core-update --version "$core_version"; then respond 200 "{\"detail\":\"node core updated successfully\"}" else log "core-update failed for version: $core_version" - respond 404 "{\"detail\":\"core-update failed for version $(json_escape "$core_version")\"}"+ respond 500 "{\"detail\":\"core-update failed for version $(json_escape "$core_version")\"}" fi
🧹 Nitpick comments (1)
pg-node-service.sh (1)
186-210: Unused HTTP version variable; remove or document purpose.Line 192 parses the HTTP version from the request line (
read -r method path version) but the variable is never referenced. The version is not checked, logged, or used in routing. Either remove it or document if it's reserved for future version validation.local request_line method path version if ! IFS= read -r request_line; then return 0 fi request_line=${request_line%$'\r'} - read -r method path version <<<"$request_line"+ read -r method path <<<"$request_line" log "Request line: $request_line"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pg-node-service.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
pg-node-service.sh
[warning] 192-192: version appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 257-257: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 257-257: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 258-258: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 258-258: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
🔇 Additional comments (2)
pg-node-service.sh (2)
143-184: Region handler logic is robust.The geofiles update handler properly validates region input, maps to CLI flags, executes the command with error checking, and responds with appropriate HTTP status codes. The JSON parsing is safe and the error messages are helpful.
253-263: Main loop resource management is adequate; ShellCheck warnings are false positives.The loop properly manages the OpenSSL coproc lifecycle: spawns per request (consistent with
-naccept 1), closes file descriptors, waits for process termination, and kills as a fallback. Thesleep 0.1prevents respawn storms. The implementation trades TLS handshake overhead per request for security isolation, which is acceptable for an API server.ShellCheck warnings SC1083 at lines 257–258 are false positives;
exec {FD}>&-is valid Bash syntax for closing file descriptors. No changes needed.
Uh oh!
There was an error while loading. Please reload this page.
| local body="" | ||
| if (( content_length > 0 )); then | ||
| if (( content_length > MAX_BODY )); then | ||
| respond 400 '{"error":"Payload too large"}' | ||
| log "Body rejected: $content_length bytes (too large)" | ||
| return 0 | ||
| fi | ||
| IFS= read -r -N "$content_length" body || true | ||
| log "Body received: ${#body} bytes" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent JSON response field naming—standardize on "detail".
Line 225 uses {"error":"Payload too large"} while all other error responses use {"detail":"..."} (lines 120, 123, 127, 156, 162, 172, 182, 212, 218, 247). This inconsistency may confuse clients parsing responses. Standardize on "detail" for all error responses.
if (( content_length > MAX_BODY )); then
- respond 400 '{"error":"Payload too large"}'+ respond 400 '{"detail":"Payload too large"}'
log "Body rejected: $content_length bytes (too large)"
return 0
fi🤖 Prompt for AI Agents
In pg-node-service.sh around lines 222 to 231, the error response uses
{"error":"Payload too large"} while other errors use {"detail":"..."}; change
the response payload to use the "detail" field (e.g. {"detail":"Payload too
large"}) and update any surrounding log/message text if necessary so all error
JSON responses consistently use "detail".
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
pg-node-service.sh (3)
225-225: Inconsistent JSON response field—"default" should be "detail".Line 225 uses
{"default":"Payload too large"}while all other error responses use{"detail":"..."}(lines 120, 127, 156, 162, 172, 182, 212, 218, 247). This inconsistency will confuse API clients.- respond 400 '{"default":"Payload too large"}'+ respond 400 '{"detail":"Payload too large"}'
138-138: HTTP 404 is semantically incorrect for a server execution failure.Line 138 responds with HTTP 404 ("Not Found") when
core-updatefails. HTTP 404 indicates a resource does not exist (e.g., an endpoint), not an internal server error. Execution failures should use HTTP 500 ("Internal Server Error"), consistent with line 120, 156, 150, and 182.- respond 404 "{\"detail\":\"core-update failed for version $(json_escape "$core_version")\"}"+ respond 500 "{\"detail\":\"core-update failed for version $(json_escape "$core_version")\"}"
107-107: $? is stale—captured after log() executes internal commands.The
log()function (lines 4–6) internally callsdateandprintf, which overwrite$?. By the time line 107 evaluates$?, it reflects the status of those commands, not the original$APP_NAME updatefailure. Additionally, the stderr redirect2>&1on line 106 is lost.Capture the exit code in a variable immediately after the failed command, before calling
log():handle_node_update(){ log "Executing $APP_NAME update" - if ! $APP_NAME update 2>&1; then - log "update failed with exit code: $?" + if ! $APP_NAME update; then+ local rc=$?+ log "update failed" respond 500 '{"detail":"update failed on server"}' return fi respond 200 "{\"detail\":\"node updated successfully\"}" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pg-node-service.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
pg-node-service.sh
[warning] 192-192: version appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 257-257: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 257-257: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 258-258: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 258-258: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
| return 0 | ||
| fi | ||
| request_line=${request_line%$'\r'} | ||
| read -r method path version <<<"$request_line" |
There was a problem hiding this comment.
Unused variable version captured from request line.
Line 192 reads version from the HTTP request line but never uses it. Either remove it or add parsing/validation logic if it's intended for future use.
- read -r method path version <<<"$request_line"+ read -r method path _ <<<"$request_line"📝 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.
| read -r method path version<<<"$request_line" | |
| read -r method path _<<<"$request_line" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 192-192: version appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
In pg-node-service.sh around line 192, the read command currently captures an
unused variable `version` from the HTTP request line; remove the unused variable
or consume it safely. Replace the three-field read with a two-field read (method
and path) or use a throwaway placeholder (e.g., `_`) for the third field, or if
version was intended to be used, add parsing/validation logic and subsequent
usage (e.g., check HTTP version format) so the variable is actually consumed.
| handle_connection <&"${OPENSSL[0]}" >&"${OPENSSL[1]}" || true | ||
| exec {OPENSSL[0]}>&- | ||
| exec {OPENSSL[1]}>&- | ||
| wait "$OPENSSL_PID" 2>/dev/null || true |
There was a problem hiding this comment.
Variable OPENSSL_PID is undefined—coproc array indices are 0 and 1, not PID.
Line 259 references $OPENSSL_PID, but the coproc syntax only populates OPENSSL[0] (read fd) and OPENSSL[1] (write fd). The process PID is stored in $OPENSSL_PID only if explicitly captured via local ... <(...) pattern, which is not used here. The wait and kill commands on lines 259 and 261 fail silently but ineffectively.
Capture the process PID from $! immediately after the coproc declaration:
while true; do
coproc OPENSSL { openssl s_server -quiet -accept "$API_PORT" -cert "$SSL_CERT_FILE" -key "$SSL_KEY_FILE" -naccept 1; }
+ local OPENSSL_PID=$!
handle_connection <&"${OPENSSL[0]}" >&"${OPENSSL[1]}" || true
exec {OPENSSL[0]}>&-
exec {OPENSSL[1]}>&-
wait "$OPENSSL_PID" 2>/dev/null || true
# Ensure cleanup even if wait fails kill "$OPENSSL_PID" 2>/dev/null || true sleep 0.1 # Brief pause to avoid rapid respawn storms
done🤖 Prompt for AI Agents
In pg-node-service.sh around line 259, the script calls wait on $OPENSSL_PID but
the coproc creates file descriptors OPENSSL[0] and OPENSSL[1] not a PID; capture
the child PID immediately after the coproc by assigning OPENSSL_PID=$! (or
similar) right after the coproc declaration so subsequent wait and kill use the
real PID, then replace any uses of $OPENSSL_PID elsewhere (e.g., lines 259 and
261) to rely on that captured PID; ensure the variable is in scope where
wait/kill are invoked and handle absence safely (e.g., check non-empty) to avoid
silent failures.
pg-node-service.shwith API key enforcement and core update endpoint (supportscore_version)pg-node.sh, including install/update/restart/uninstall commands and post-install promptcore-updatewith--versionhandling (includinglatest), validation, completions, and usage helpSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.