Surface CI hang-cancels in monitor-tick check (11)

[tomerweller]

Closes #3823

Summary

monitor-tick check (11) keyed exclusively on conclusion failure, so a CI job wall-clock-killed at its timeout-minutes cap — which resolves cancelled, not failure — was scored ci: all green. Per #3768 that is the worse of the two testnet-shard failure modes: because the job ends cancelled, if: failure() never fires, the #3289 watchdog diagnostics dump is destroyed, and --log-failed is empty. This PR teaches the monitor to detect it.

A pure, unit-tested classify_ci_cancel (scripts/lib/monitor-decisions.sh) maps a job outcome to not-cancel / supersede-cancel / hang-cancel / manual-cancel, distinguishing a reportable hang (no superseding run and duration ≥ the job's timeout-minutes cap) from a routine concurrency supersede — so widening the predicate does not recreate the #3653 alarm fatigue. Check (11a)/(11b)/REPORTING RULE and the ci: status line are rewired around it; (11b) now reads the immutable per-job conclusion, closing the run-level non-monotonicity a retry would otherwise cause (#3823 comment 6). Hang-cancels are explicitly excluded from auto-rerun (#3768: the rerun burns another full runner slot and lands identically).

Downstream blind spots in quickstart-retry.yml and the diagnostics-upload step (issue comments 4–6) are OUT of scope per triage — this PR fixes only the monitor's detection.

Plan reference

Converged Plan comment

Test plan

• scripts/test-monitor-skill-snippets.sh — 485/485 TAP (was 478; +7 new classify_ci_cancel cases + structural SKILL.md-references assertion)
• scripts/test-shell-lib-cross-shell.sh — 41/41 (new fn sources+runs clean under bash and zsh; no bashisms)
• bash -n on both modified shell files; direct zsh call returns hang-cancel
• n/a cargo fmt/clippy/test — no Rust touched (shell + markdown only); parity surface unaffected (internal monitor/admin tooling)

Regression test (kind: bug-fix)

• Test: scripts/test-monitor-skill-snippets.sh — classify_ci_cancel cases T63aa–T63ag
• Pre-fix: committed as ac93aad — verified FAILED: the function is absent on main, so the harness aborts with classify_ci_cancel: command not found (set -euo pipefail); (11b)'s select(.conclusion == "failure") returns empty for a cancelled job (scored green)
• Post-fix: verified PASSES after 7edf77f — the CI: the ~55-min quickstart testnet step-8 hang recurred on #3765 attempt 4 — #3289's step-level timeout-minutes: 25 did not fire, and the job cancel destroyed the watchdog dump it was built to capture #3768 fixture (cancelled no 3300 2700) returns hang-cancel; supersede/manual/failure paths unchanged

Deviations from plan

None.

🤖 Generated with Claude Code

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: classify_ci_cancel is a correct, well-tested pure function; SKILL.md wiring and the 7 new TAP cases are coherent. No blocking correctness defects. Two minor test-coverage gaps noted inline (non-blocking).

Full review

Cycle 1 (no prior ## 🔍 Reviewer: Correctness comment) — complete class-labeled change-list below.

Function logic (scripts/lib/monitor-decisions.sh) — correct:

• Branch order is right: not-cancel (conclusion≠cancelled) → supersede-cancel (has_successor==yes) → timeout comparison. Putting the successor check before the timeout check means a superseded run that also hit its cap is correctly treated as a routine concurrency cancel (green), matching the documented intent.
• ^[0-9]+$ guards reject empty/negative/non-integer inputs and route them to the fail-open hang-cancel branch — matches the documented "fail toward surfacing" rule.
• >= boundary (duration == timeout ⇒ hang-cancel) is covered by T63af.
• has_successor anything-other-than-yes ⇒ treated as no — matches the doc.
• echo-plus-global (CI_CANCEL_CLASS + stdout) mirrors the sibling classify_* contract.

Test-verification gate (kind: bug-fix) — satisfied:

• Regression test T63aa–T63ag added. Pre-fix RED is credible: the function is absent on main, so under set -euo pipefail any call aborts the harness (command not found), and (11b)'s old select(.conclusion == "failure") scores a cancelled job green. Post-fix GREEN confirmed by the passing "Monitor-Tick Skill Snippets" + "Script self-tests" CI checks.
• TAP plan bumped 478 → 485 (+7) consistent with the added cases.

test-coverage (minor, inline — non-blocking):

• No case exercises has_successor with an empty/garbage value (the "anything other than yes ⇒ no" path). The code handles it; a test would pin the contract.
• Fail-open is only tested with empty TIMEOUT_SEC (T63ae). The symmetric "unreadable DURATION_SEC, readable timeout, no successor ⇒ hang-cancel" path is untested.

Neither gap blocks: the covered paths include every returned literal plus the boundary and one fail-open case.

Readability / error handling: doc comment is thorough and accurately describes each literal and the fail-open rule. No error paths swallowed.

[tomerweller]

🔍 Reviewer: Risk

Verdict: APPROVE

Summary: Change is internal monitor/admin tooling (shell + markdown) with no parity/observable surface impact. Alarm-fatigue regression risk is explicitly mitigated by the supersede/manual classification; cross-shell portability was verified. No blocking risk.

Full review

Cycle 1 (no prior ## 🔍 Reviewer: Risk comment) — complete class-labeled change-list below.

parity-gap: none. Per docs/PARITY.md and CLAUDE.md, monitor/admin/debug tooling, metrics, and logging may deviate freely. This PR touches only .claude/skills/monitor-tick/SKILL.md, scripts/lib/monitor-decisions.sh, and scripts/test-monitor-skill-snippets.sh — no ledger/bucket hashes, tx result/meta XDR, SCP/overlay wire bytes, history archive format, or HTTP/RPC/CLI contracts. Nothing on the observable/interop surface. Reviewer-B-as-Risk (not Parity) is the correct lens; no parity concern.

regression-risk (alarm fatigue, #3653) — mitigated. Widening check (11) to scan cancelled could have resurfaced routine concurrency cancels as alarms. The supersede-cancel branch (newer run of same workflow + same head ref) and manual-cancel (under-cap early cancel) both stay green, so only a genuine no-successor at-cap hang-cancel is surfaced. Ref-scoping the successor check to main correctly prevents a concurrent PR run from masking a real main hang.

operational — sound. "Never auto-rerun a hang-cancel" (step 3) prevents burning a second full-length runner slot on a job that will land identically at the same wall (#3768). Reading the immutable per-job conclusion rather than the run-level one closes the retry-driven non-monotonicity (#3823 comment 6).

portability — verified. [[ =~ ]], [[ -ge ]], and printf are bash/zsh-safe; PR reports 41/41 cross-shell and bash -n clean. No external process/network I/O in the function.

Scope discipline: the quickstart-retry.yml / diagnostics-upload blind spots are correctly left OUT per triage; this PR fixes only the monitor's detection.

Fail-open trade-off (accepted): an unreadable timeout-minutes on a no-successor cancel surfaces as hang-cancel, i.e. biased toward a false alarm rather than a missed hang. Given the GitHub ~5m force-kill grace and the documented "never hide a hang" rule, this is the right direction and low-frequency. Not blocking.

[tomerweller]

Review: Bounce-Back Cycle 1

Reason: CI failed (unrelated, will rebase)

Reviewer A: APPROVE — classify_ci_cancel is correct and well-tested; only minor non-blocking test-coverage notes.
Reviewer B: APPROVE — internal monitor/admin tooling, no parity surface; alarm-fatigue and portability risks mitigated.
External reviewers: none
CI: red

Both agent reviewers APPROVE and there are no external change requests, so the only red signal is CI.

Failing check: test (testnet, core,horizon, ...) in the Quickstart workflow (run 32804041724), conclusion FAILURE. The job's testnet horizon-ingestion step did not complete and was killed during teardown:

Terminate orphan process: pid (13279) (go)
Terminate orphan process: pid (13348) (test_horizon_ingesting)

This is unrelated to the diff: the PR changes only .claude/skills/monitor-tick/SKILL.md, scripts/lib/monitor-decisions.sh, and scripts/test-monitor-skill-snippets.sh — shell + markdown, zero Rust and nothing that feeds a horizon-ingestion integration test. It is the flaky live-testnet-shard failure mode (ironically, the exact class this PR teaches the monitor to detect). Every other check — including "Monitor-Tick Skill Snippets" and "Script self-tests" that exercise the new code — is green.

Routing back to ready-for-doing for /do Mode B to rebase on origin/main and re-trigger CI. If the testnet shard is still red after a clean rebase, the next /review-pr will mark this blocked for operator attention.

[tomerweller]

Do: Mode B — CI re-trigger (no code change)

Both agent reviewers APPROVE and there are no inline review threads to address. The bounce-back was solely the flaky test (testnet, core,horizon, ...) shard in the Quickstart workflow (unrelated to this shell+markdown diff — it was a testnet horizon-ingestion teardown/orphan-process failure).

A rebase on origin/main is a no-op: the branch tip is already atop the current origin/main (9f534c2), so main has not moved since the branch was cut and there is nothing to replay. Re-triggered the failed Quickstart run instead (gh run rerun --failed, run 32804041724) to get a fresh result on the flaky shard.

Routing back to in-review for /review-pr to re-evaluate once CI settles.

[tomerweller]

Review: Bounce-Back Cycle 1

Reason: CI failed (unrelated, will rebase)

Reviewer A (Correctness): APPROVE — classify_ci_cancel is a clean, pure, fully-branch-covered decision function (4 stable literals, fail-open on unreadable duration/timeout); regression tests T63aa–T63ag (plan 478→485) plus the SKILL.md-references consistency assertion; SKILL.md check (11a)/(11b)/REPORTING-RULE rewiring is internally consistent and reads the immutable per-job conclusion. No blocking concerns.
Reviewer B (Risk): APPROVE — shell + markdown only; internal monitor/admin tooling, no observable/parity surface touched. Alarm-fatigue regression (#3653) is mitigated by the supersede-cancel branch; portability is fine (no bashisms, POSIX-ish). No blocking concerns.
External reviewers: none
CI: red

Both agent reviewers APPROVE and there are no external change requests, so the only red signal is CI.

Failing check: test (testnet, core,horizon, ...) in the Quickstart workflow (run 32804041724), conclusion CANCELLED — the testnet shard started 05:02:29Z and was wall-clock-killed at 05:57:30Z (~55 min). This is the flaky live-testnet hang-then-cancel mode (#3768) — ironically the exact class this PR teaches the monitor to detect. It is not diff-attributable: the PR changes only .claude/skills/monitor-tick/SKILL.md, scripts/lib/monitor-decisions.sh, and scripts/test-monitor-skill-snippets.sh (shell + markdown, zero Rust, nothing feeding a horizon-ingestion integration test). Every other check — including Monitor-Tick Skill Snippets, Script self-tests, Test, Clippy, Build — is green (43/44 green, 1 SKIPPED-adjacent cancelled shard).

origin/main has advanced to 9f534c2 since this PR's head (7edf77f) was built. Routing back to ready-for-doing for /do Mode B to rebase on origin/main and re-trigger CI so the flaky testnet shard gets a fresh run. If the testnet shard is still red after a clean rebase, the next /review-pr will mark this blocked for operator attention.

[tomerweller]

Do: Mode B — rebased on current main (resolved TAP-plan conflict)

Both agent reviewers APPROVE with no inline review threads to address. The prior bounce-back was the flaky test (testnet, core,horizon, ...) Quickstart shard (hang-then-cancel, unrelated to this shell+markdown diff).

origin/main had advanced to 854e23b (#3934, alarm re-keying) past the PR's old base 9f534c2, so this time the rebase was not a no-op:

• Replayed the 2 branch commits onto 854e23b.
• One conflict in scripts/test-monitor-skill-snippets.sh: both Re-key recovery-stalled alarm to the family sum of all reason series #3934 and this PR bumped TAP_PLAN from the same 478 base (main +1 → 479; this PR +7 → 485). Resolved to the combined 486 (478 + 1 + 7).
• Verified: bash scripts/test-monitor-skill-snippets.sh prints 1..486, all ok, exit 0 (the harness fails on any plan/count mismatch). bash -n clean on both scripts. Diff scope unchanged (3 files: SKILL.md, monitor-decisions.sh, test-monitor-skill-snippets.sh).

Force-pushed the rebased branch (7edf77f → c27b8926), which re-triggers CI on a fresh commit atop current main. Routing back to in-review.

[tomerweller]

Rebased on current main and resolved the TAP-plan conflict (now 486). Ready for re-review.

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Re-review of the rebased head (do/issue-3823, TAP plan now 486). classify_ci_cancel is a correct, well-tested pure function; the SKILL.md check-(11) wiring and the 7 new TAP cases are coherent. No blocking correctness defects. Prior cycle-1 concern classes were test-coverage (minor/inline, non-blocking) only — no blocking class was raised, and none is discovered now.

Full review

Cycle 2 (prior ## 🔍 Reviewer: Correctness APPROVE exists at 2026-08-25T04:33). The head was rebased on current origin/main at 07:03; the only delta vs. the cycle-1-reviewed tree is TAP_PLAN 485→486 (main added one structural test). Re-verified against the merged tree.

Independent verification on the rebased head (merged with origin/main):

• bash -n clean on both scripts/lib/monitor-decisions.sh and scripts/test-monitor-skill-snippets.sh.
• Full harness: 1..486 — 486/486 ok.
• Function spot-check reproduces every documented literal: cancelled/no/3300/2700→hang-cancel, cancelled/yes/600/2700→supersede-cancel, cancelled/no/120/2700→manual-cancel, failure/…→not-cancel, cancelled/no/3300/""→hang-cancel (fail-open), cancelled/no/2700/2700→hang-cancel (>= boundary).

Function logic — correct:

• Branch order (not-cancel → supersede-cancel → timeout comparison) is right: a superseded-and-at-cap run is correctly treated as a routine concurrency cancel (green).
• ^[0-9]+$ guards reject empty/negative/non-integer inputs and route to the fail-open hang-cancel branch, matching the documented "fail toward surfacing" rule.
• >= boundary (duration == timeout ⇒ hang-cancel) covered by T63af.
• echo-plus-global (CI_CANCEL_CLASS + stdout) mirrors the sibling classify_* contract.

Test-verification gate (kind: bug-fix) — satisfied:

• Regression tests T63aa–T63ag added. Pre-fix RED is credible: the function is absent on main, so under set -euo pipefail any call aborts the harness (command not found), and (11b)'s old select(.conclusion == "failure") scores a cancelled job green. Post-fix GREEN confirmed by CI ("Script self-tests" / monitor-snippets) and reproduced locally.

test-coverage (prior cycle-1 class, still minor/non-blocking): the "garbage has_successor" path and the symmetric "unreadable DURATION_SEC, readable timeout, no-successor" fail-open path remain untested. Every returned literal plus the boundary and one fail-open case ARE covered, so this does not block; suitable as an inline/follow-up nit only.

Readability / error handling: doc comment accurately describes each literal and the fail-open rule; no error paths swallowed.

[tomerweller]

🔍 Reviewer: Risk

Verdict: APPROVE

Summary: Re-review of the rebased head. Change is internal monitor/admin tooling (shell + markdown) with no parity/observable-surface impact. The #3653 alarm-fatigue regression is explicitly mitigated by the supersede/manual classification; cross-shell portability holds. No blocking risk; no new concern class discovered.

Full review

Cycle 2 (prior ## 🔍 Reviewer: Risk APPROVE exists at 2026-08-25T04:33). Re-evaluated on the head merged with current origin/main; only delta is the TAP-plan rebase bump.

parity-gap: none. Per docs/PARITY.md / CLAUDE.md, monitor/admin/debug tooling, metrics, and logging may deviate freely. Touches only .claude/skills/monitor-tick/SKILL.md, scripts/lib/monitor-decisions.sh, scripts/test-monitor-skill-snippets.sh — nothing on ledger/bucket hashes, tx result/meta XDR, SCP/overlay wire bytes, history archive format, or HTTP/RPC/CLI contracts. Risk (not Parity) is the correct lens.

regression-risk (alarm fatigue, #3653) — mitigated. Widening check (11) to scan cancelled is scoped: supersede-cancel (newer run, same workflow + same head ref main) and manual-cancel (under-cap) both stay green; only a no-successor at-cap hang-cancel surfaces. Ref-scoping the successor check to main prevents a concurrent PR run from masking a real main hang.

operational — sound. "Never auto-rerun a hang-cancel" prevents burning a second full-length runner slot on a job that lands identically at the same wall (#3768). Reading the immutable per-job conclusion rather than the run-level one closes the retry-driven non-monotonicity (#3823 comment 6).

portability — verified. [[ =~ ]], [[ -ge ]], printf are bash/zsh-safe; bash -n clean locally and CI reports cross-shell green.

Scope discipline: quickstart-retry.yml / diagnostics-upload blind spots correctly left OUT per triage; this PR fixes only detection.

Fail-open trade-off (accepted): unreadable timeout on a no-successor cancel biases toward a false hang-cancel alarm rather than a missed hang. Given GitHub's ~5m force-kill grace and the "never hide a hang" rule, this is the right direction and low-frequency. Not blocking.

[tomerweller]

✅ Merged

Commit: 129190c

Follow-up issues filed for unaddressed inline review comments: none (no unresolved inline threads on the PR)