From 661b713e2dbf061cee89e7a7f52d7ff4470f758f Mon Sep 17 00:00:00 2001 From: Alexey <1556417+alex-solovyev@users.noreply.github.com> Date: Thu, 12 Mar 2026 23:53:08 +0100 Subject: [PATCH 1/2] fix: document GH#4271 quality-debt findings as verified in stats-wrapper.sh All 3 PR #4232 review findings are confirmed addressed: - CRITICAL (coderabbit): ps probes guarded with || true so set -euo pipefail cannot abort before etime fallback runs (commit 80ee81f) - HIGH (coderabbit): pidfile preserved for live PIDs via kill -0 check; only removed when process is confirmed gone (commit 80ee81f) - MEDIUM (gemini): awk uses exit 1 for invalid input; || true on command substitution is intentional to prevent set -e abort on parse failure (commit 268ad841) Adds inline comment explaining the || true design rationale so future reviewers understand it is not a suppression but a deliberate guard. --- .agents/scripts/stats-wrapper.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.agents/scripts/stats-wrapper.sh b/.agents/scripts/stats-wrapper.sh index 2202207b5d..2744d58b2a 100755 --- a/.agents/scripts/stats-wrapper.sh +++ b/.agents/scripts/stats-wrapper.sh @@ -35,6 +35,14 @@ mkdir -p "$(dirname "$STATS_PIDFILE")" ####################################### # Portable elapsed-seconds lookup for a running PID +# +# Robustness notes (GH#4271 — PR #4232 review findings): +# - Both ps probes use `|| true` so set -euo pipefail cannot abort the +# function before the etime fallback runs (critical finding, coderabbit). +# - awk uses `exit 1` for all invalid-input branches (medium finding, gemini). +# The `|| true` on the awk command substitution is intentional — it prevents +# set -euo pipefail from aborting on awk parse failure; the ^[0-9]+$ guard +# below handles the empty-result case cleanly. ####################################### _stats_process_elapsed_seconds() { local pid="$1" From c29034ab928711e491dd947cdf50f53170c0b6eb Mon Sep 17 00:00:00 2001 From: Alexey <1556417+alex-solovyev@users.noreply.github.com> Date: Thu, 12 Mar 2026 23:57:15 +0100 Subject: [PATCH 2/2] refactor: replace history-referencing comments with logic-focused comments in stats-wrapper.sh Replace GH issue/PR references in robustness notes with evergreen comments that explain the technical why (set -euo pipefail guard behaviour, awk exit semantics, empty-output handling) rather than the development history. --- .agents/scripts/stats-wrapper.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/.agents/scripts/stats-wrapper.sh b/.agents/scripts/stats-wrapper.sh index 2744d58b2a..f1b474702b 100755 --- a/.agents/scripts/stats-wrapper.sh +++ b/.agents/scripts/stats-wrapper.sh @@ -36,13 +36,13 @@ mkdir -p "$(dirname "$STATS_PIDFILE")" ####################################### # Portable elapsed-seconds lookup for a running PID # -# Robustness notes (GH#4271 — PR #4232 review findings): -# - Both ps probes use `|| true` so set -euo pipefail cannot abort the -# function before the etime fallback runs (critical finding, coderabbit). -# - awk uses `exit 1` for all invalid-input branches (medium finding, gemini). -# The `|| true` on the awk command substitution is intentional — it prevents -# set -euo pipefail from aborting on awk parse failure; the ^[0-9]+$ guard -# below handles the empty-result case cleanly. +# Robustness notes: +# - The `ps` commands use `|| true` to prevent `set -euo pipefail` from +# aborting the script if the process disappears. This allows the `etime` +# fallback logic to execute. +# - The `awk` command substitution also uses `|| true`. `awk` is scripted to +# `exit 1` on invalid input, and this guard prevents script termination. +# The subsequent `^[0-9]+$` check handles the empty output case. ####################################### _stats_process_elapsed_seconds() { local pid="$1"