Skip to content

t1404: Harden ShellCheck fallback path — process group kill + missing warning #2923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
marcusquinn merged 1 commit into from
Mar 5, 2026
Merged

t1404: Harden ShellCheck fallback path — process group kill + missing warning #2923

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions .agents/scripts/linters-local.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,10 @@ run_shellcheck() {
# exponential expansion. Each file gets max 30s and 1GB virtual memory.
local sc_timeout=30
local file_result
# t1404: warn once when no timeout utility is available (degraded protection)
if [[ -z "$timeout_cmd" ]]; then
print_warning "ShellCheck: no timeout/gtimeout utility found; using portable fallback (less reliable process cleanup)"
fi
for file in "${ALL_SH_FILES[@]}"; do
[[ -f "$file" ]] || continue
file_result=""
Expand Down Expand Up @@ -371,12 +375,16 @@ run_shellcheck() {
continue
}
# t1398.2: no -x in fallback path (no timeout utility = higher risk)
# t1404: use process group kill to clean up ShellCheck child processes
(
ulimit -v 1048576 2>/dev/null || true
shellcheck -P SCRIPTDIR --severity=warning --format=gcc "$file"
) >"$sc_tmpfile" 2>&1 &
local sc_bg_pid=$!
(sleep "$sc_timeout" && kill "$sc_bg_pid" 2>/dev/null) &
# Watcher: kill the process group (- prefix) to catch child processes,
# falling back to single-process kill if group kill fails (e.g., not a
# process group leader on some shells).
(sleep "$sc_timeout" && kill -- -"$sc_bg_pid" 2>/dev/null || kill "$sc_bg_pid" 2>/dev/null) &
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While this change correctly attempts to kill the process group with a fallback, suppressing stderr for both kill commands with 2>/dev/null can hide important errors, such as "Operation not permitted". This goes against the project's general rules which advise against blanket error suppression. While removing the suppression may result in "No such process" messages in the log if the process has already completed, it ensures that actual failures in the timeout mechanism are visible for debugging.

Suggested change
(sleep "$sc_timeout" && kill -- -"$sc_bg_pid" 2>/dev/null || kill "$sc_bg_pid" 2>/dev/null) &
(sleep "$sc_timeout" && kill -- -"$sc_bg_pid" || kill "$sc_bg_pid") &
References
  1. Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.

local sc_watcher_pid=$!
local sc_exit_code=0
wait "$sc_bg_pid" 2>/dev/null || sc_exit_code=$?
Expand All @@ -388,7 +396,7 @@ run_shellcheck() {
# Exit codes >128 indicate signal kill (timeout fired)
if [[ $sc_exit_code -gt 128 ]]; then
timed_out=$((timed_out + 1))
print_warning "ShellCheck: $file killed after ${sc_timeout}s (no timeout utility; portable fallback)"
print_warning "ShellCheck: $file killed after ${sc_timeout}s (portable fallback)"
file_result=""
continue
fi
Expand Down
Loading