Skip to content
Merged
Changes from 1 commit
Commits
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
8 changes: 8 additions & 0 deletions .agents/scripts/stats-wrapper.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

While adding comments to explain the rationale is excellent, these comments are tied very specifically to the development history (GitHub issues, PRs, and review tools). For better long-term maintainability, it's preferable for code comments to explain the why of the logic itself, rather than the history of how it came to be. The git history is the best place for that context.

I suggest rephrasing these comments to be more evergreen and focused on the technical reasons for the implementation choices.

Suggested change
#
# 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.
References
  1. Ensure comments provide a single, clear justification for a design choice, avoiding the presentation of multiple, potentially conflicting rationales.

#######################################
_stats_process_elapsed_seconds() {
local pid="$1"
Expand Down
Loading