quality-debt: .agents/scripts/circuit-breaker-helper.sh — PR #2294 review feedback (high)

[marcusquinn]

Unactioned Review Feedback

Source PR: #2294
File: .agents/scripts/circuit-breaker-helper.sh
Reviewers: gemini, human
Findings: 8
Max severity: high

---

HIGH: gemini (gemini-code-assist[bot])

File: .agents/scripts/circuit-breaker-helper.sh:190

In this function, printf and mv commands have their stderr redirected to /dev/null. This practice conceals the root cause of failures, such as 'disk full' or permission errors, making troubleshooting difficult. The function logs a generic warning, but the specific error from the command is lost.

if ! printf '%s\n' "$state_json" >"$tmp_file"; then
		_cb_log_warn "failed to write temp state file: $tmp_file"
		rm -f "$tmp_file" 2>/dev/null || true
		return 1
	fi
	if ! mv -f "$tmp_file" "$state_file"; then
		_cb_log_warn "failed to move temp state to: $state_file"
		rm -f "$tmp_file" 2>/dev/null || true
		return 1
	fi

References

1. When reporting errors for failed file operations in shell scripts, such as 'jq' writes, include the file path in the error message. Avoid suppressing stderr with '2>/dev/null' to ensure that diagnostic information about malformed files or write failures is visible.

View comment

HIGH: gemini (gemini-code-assist[bot])

File: .agents/scripts/circuit-breaker-helper.sh:560

The gh issue create command has its stderr suppressed. This prevents useful error messages from gh (e.g., authentication failure, invalid repository, permission denied) from being displayed, making it difficult to diagnose why issue creation failed. The script only logs a generic "failed to create GitHub issue" message. This applies to other gh calls in this script as well.

issue_url=$(gh issue create \
			--repo "$repo_slug" \
			--title "Supervisor circuit breaker tripped — ${failure_count} consecutive failures" \
			--body "$body" \
			--label "circuit-breaker") || {
			_cb_log_warn "failed to create GitHub issue"
			return 1
		}

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.

View comment

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/circuit-breaker-helper.sh:62

Suppressing stderr with 2>/dev/null here hides potentially important errors, such as the script lacking permissions to create the state directory. While || true prevents the script from exiting, the failure is silent. To improve debuggability, it's better to allow the error message from mkdir to be displayed.

mkdir -p "$dir" || true

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.

View comment

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/circuit-breaker-helper.sh:161

The 2>/dev/null suppresses error messages from cat. Although the file's existence is checked on line 159, cat could still fail due to permission issues. Hiding this error makes debugging more difficult. If cat fails, the script currently proceeds as if the state file is corrupt, which could be misleading.

content=$(cat "$state_file") || content=""

References

1. Avoid using 2>/dev/null to suppress errors on file operations if the file's existence has already been verified by a preceding check (e.g., [[ -f "$file" ]] or an early return). This practice is redundant for 'file not found' errors and can mask other important issues like permissions problems.

View comment

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/circuit-breaker-helper.sh:463

Suppressing stderr for the gh command hides important diagnostic information. If the command fails due to authentication issues, network problems, or incorrect repository configuration, the error is silenced. The script then proceeds with an empty repo_slug, leading to a less specific error message later.

repo_slug=$(gh repo view --json nameWithOwner -q '.nameWithOwner') || repo_slug=""

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.

View comment

HIGH: human (augmentcode[bot])

File: .agents/scripts/circuit-breaker-helper.sh:90
trap "rmdir ..." RETURN in _cb_with_state_lock sets a global RETURN trap that persists after the function returns, so later function returns can remove the lock directory while another process holds it (breaking mutual exclusion). Also, cleanup only runs on RETURN (not EXIT), so an early exit/set -e abort can leave a stale lock and block dispatch.

Severity: high

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

View comment

MEDIUM: human (augmentcode[bot])

File: .agents/scripts/circuit-breaker-helper.sh:35
After stripping non-digits, SUPERVISOR_CIRCUIT_BREAKER_THRESHOLD=0 remains “valid” and will cause the breaker to trip on the first failure (new_count >= 0). That seems like an invalid/misleading configuration value to accept silently.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

View comment

MEDIUM: human (augmentcode[bot])

File: .agents/scripts/circuit-breaker-helper.sh:328
In cmd_check, the auto-reset path calls cmd_reset "auto_cooldown" without handling failures; if reset errors (lock timeout/write failure), check may abort under set -e and still report dispatch as blocked unexpectedly. If this runs in the pulse loop, it could turn a transient reset failure into a longer dispatch pause.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

View comment

---

Auto-generated by quality-feedback-helper.sh scan-merged. Review each finding and either fix the code or dismiss with a reason.

[marcusquinn]

Additional Review Feedback (PR #2389)

Reviewers: gemini
Findings: 2
Max severity: medium

---

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/circuit-breaker-helper.sh:112

The date command used to convert an ISO timestamp to an epoch second suppresses stderr. If the timestamp format is invalid, date will fail silently, and the epoch variable will be empty, which could lead to unexpected behavior in _cb_elapsed_since. This violates the rule: 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.

	epoch=$(date -u -j -f '%Y-%m-%dT%H:%M:%SZ' "$ts" '+%s') || \
		# GNU date
		epoch=$(date -u -d "$ts" '+%s') || \
		epoch=""

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.

View comment

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/circuit-breaker-helper.sh:463

Multiple gh CLI calls throughout this script suppress stderr using 2>/dev/null. This violates the project's general rule to avoid error suppression, as it hides potentially useful error messages related to authentication, network issues, or invalid arguments. For example, in _cb_resolve_repo_slug, if gh repo view fails, the reason is hidden. This violates the rule: 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.

	repo_slug=$(gh repo view --json nameWithOwner -q '.nameWithOwner') || repo_slug=""

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.

View comment

---

Appended by quality-feedback-helper.sh scan-merged (cross-PR file dedup, t1411).

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[marcusquinn]

Additional Review Feedback (PR #2389)

Reviewers: gemini
Findings: 2
Max severity: medium

---

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/circuit-breaker-helper.sh:112

The date command used to convert an ISO timestamp to an epoch second suppresses stderr. If the timestamp format is invalid, date will fail silently, and the epoch variable will be empty, which could lead to unexpected behavior in _cb_elapsed_since. This violates the rule: 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.

	epoch=$(date -u -j -f '%Y-%m-%dT%H:%M:%SZ' "$ts" '+%s') || \
		# GNU date
		epoch=$(date -u -d "$ts" '+%s') || \
		epoch=""

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.

View comment

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/circuit-breaker-helper.sh:463

Multiple gh CLI calls throughout this script suppress stderr using 2>/dev/null. This violates the project's general rule to avoid error suppression, as it hides potentially useful error messages related to authentication, network issues, or invalid arguments. For example, in _cb_resolve_repo_slug, if gh repo view fails, the reason is hidden. This violates the rule: 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.

	repo_slug=$(gh repo view --json nameWithOwner -q '.nameWithOwner') || repo_slug=""

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.

View comment

---

Appended by quality-feedback-helper.sh scan-merged (cross-PR file dedup, t1411).

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[johnwaldo]

Fixed in PR #3916 (commit e34ea8c) — circuit-breaker-helper.sh: removed 2>/dev/null from printf and mv in cb_write_state(). Errors like 'disk full' or permission issues are now visible in the warning message. The rm -f cleanup lines retain 2>/dev/null || true since those are best-effort cleanup.

[marcusquinn]

Dispatching worker.

• Model: sonnet (anthropic/claude-sonnet-4-6)
• Branch: bugfix/t3239-circuit-breaker-helper-high
• Scope: Address high-priority review findings on .agents/scripts/circuit-breaker-helper.sh from PR t1331: Supervisor circuit breaker — pause on consecutive failures #2294
• Attempt: 1 of 1

[github-actions]

Completed via PR #4668. Task t3239 merged to main.