joshtronic/igor
joshtronic/igor
Watch
2
Code Issues 3 Pull requests 2 Activity Actions

feat: add healthcheck ping wiring (lib/healthcheck.sh, opt-in) #348

Merged
joshtronic merged 2 commits from agent/339-feat-healthcheck-ping-wiring-lib-healthcheck-sh into master 2026-07-05 00:50:38 +00:00
Conversation 3 Commits 2 Files changed 4 +168
igor commented 2026-07-04 19:52:11 +00:00
Collaborator
Copy link

What this PR does

  • feat: add healthcheck ping wiring (lib/healthcheck.sh, opt-in)
  • Add hc_ping <which> [start|success|fail] -- best-effort curl, --max-time 10, always returns 0
  • Wire HEALTHCHECK_HEARTBEAT_URL -- plain ping at the top of every tick (dead-man's switch, check A)
  • Wire HEALTHCHECK_TASK_URL -- start ping once the tick clears the health/deploy gates, paired success/fail ping in the existing cleanup() EXIT trap so every exit path (including the cascade's many early exit 0s, and any crash) reports honestly (check B)
  • Document both new vars in .env.example, both default empty -> clean no-op like the SMTP2GO/GSC gates
  • Unit tests in bin/test-healthcheck.sh (unset URL -> no curl call, URL set -> correct request built for base//start//fail, curl failure -> rc still 0)

Both vars are new -- no existing var was renamed or reused. Neither agent-settings.json, systemd/, nor .forgejo/workflows/ were touched. Merges safe now; activation (standing up the healthchecks server + creating the two checks + dropping the URLs into .env) is the operator-side #297.

Test plan

  • bash bin/check-sync.sh passes (outcome-sentinel sync check + every bin/test-*.sh, including the new test-healthcheck.sh)
  • bash -n on bin/tick.sh, lib/healthcheck.sh, bin/test-healthcheck.sh -- no syntax errors
  • /security-review run on the diff -- no findings (URL args are always trusted .env-sourced values or hardcoded literals, never attacker-influenced; output is fully discarded)
  • No manual verification needed beyond the above -- this PR is inert until #297 supplies real check URLs

Closes #339

## What this PR does - [x] feat: add healthcheck ping wiring (`lib/healthcheck.sh`, opt-in) - [x] Add `hc_ping <which> [start|success|fail]` -- best-effort curl, `--max-time 10`, always returns 0 - [x] Wire `HEALTHCHECK_HEARTBEAT_URL` -- plain ping at the top of every tick (dead-man's switch, check A) - [x] Wire `HEALTHCHECK_TASK_URL` -- start ping once the tick clears the health/deploy gates, paired success/fail ping in the existing `cleanup()` EXIT trap so every exit path (including the cascade's many early `exit 0`s, and any crash) reports honestly (check B) - [x] Document both new vars in `.env.example`, both default empty -> clean no-op like the SMTP2GO/GSC gates - [x] Unit tests in `bin/test-healthcheck.sh` (unset URL -> no curl call, URL set -> correct request built for base/`/start`/`/fail`, curl failure -> rc still 0) Both vars are new -- no existing var was renamed or reused. Neither `agent-settings.json`, `systemd/`, nor `.forgejo/workflows/` were touched. Merges safe now; activation (standing up the healthchecks server + creating the two checks + dropping the URLs into `.env`) is the operator-side #297. ## Test plan - [x] `bash bin/check-sync.sh` passes (outcome-sentinel sync check + every `bin/test-*.sh`, including the new `test-healthcheck.sh`) - [x] `bash -n` on `bin/tick.sh`, `lib/healthcheck.sh`, `bin/test-healthcheck.sh` -- no syntax errors - [x] `/security-review` run on the diff -- no findings (URL args are always trusted `.env`-sourced values or hardcoded literals, never attacker-influenced; output is fully discarded) - [x] No manual verification needed beyond the above -- this PR is inert until #297 supplies real check URLs Closes #339
feat: add healthcheck ping wiring (lib/healthcheck.sh, opt-in)
All checks were successful
Lint / check-sync (pull_request) Successful in 5s
Details
Lint / check-sync (push) Successful in 4s
Details
73acc5eee7
igor commented 2026-07-04 19:54:40 +00:00
Author
Collaborator
Copy link

🤖 Review — APPROVE (automated)

CI for 73acc5ee: success

Opt-in healthcheck ping wiring: a new lib/healthcheck.sh (hc_ping), sourced and wired into bin/tick.sh (heartbeat + task start/success/fail), documented in .env.example, with unit tests. CI green. Approving.

Verification against the checklist — all items hold up:

  • hc_ping <which> [start|success|fail] exists in lib/healthcheck.sh, uses curl -fsS --max-time 10 -o /dev/null, and always return 0 (both || true and the trailing return 0). ✅
  • Heartbeat ping wired unconditionally after lock/log setup (hc_ping heartbeat). ✅
  • Task start ping placed after claude_health_blocked gate with HC_TASK_STARTED=1; paired success/fail lives in cleanup() guarded on HC_TASK_STARTED. ✅
  • Both vars documented in .env.example, default empty, exported with :- defaults so set -u is safe. ✅
  • Tests cover unset/empty URL (no curl), correct URL construction for base//start//fail, state-omitted default, and curl-failure-still-rc0. ✅

Correctness spot-checks:

  • cleanup() captures local rc=$? before any hc_ping call, so the ping's curl exit status can't clobber the real exit code. Ordering is right.
  • The HC_TASK_STARTED guard correctly prevents false success/fail reports for ticks that bail at the health/deploy/cooldown gates (start never fired), while covering the cascade's early exit 0s and crashes via the EXIT trap. Sound design.
  • Test mock curl() { CURL_URL="${*: -1}"; } correctly grabs the last arg (the URL); ${*: -1} slicing is valid bash. Assertions match the real call shape.
  • State defaults to success → base URL (healthchecks.io convention); consistent with the test task (state omitted) case.

Security: URLs are trusted .env-sourced or unset; output discarded to /dev/null, stderr suppressed, --max-time 10 bounds the network call. No injection surface. No CI/workflow files touched. Inert until #297 supplies real URLs.

Scope: ~130 lines across 4 files, tightly focused on the one issue. No drive-by changes.

Nothing blocking. Clean, well-tested, no-op-by-default change.

Independent review by the harness on claude-opus-4-8 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.

### 🤖 Review — `APPROVE` _(automated)_ CI for `73acc5ee`: **success** Opt-in healthcheck ping wiring: a new `lib/healthcheck.sh` (`hc_ping`), sourced and wired into `bin/tick.sh` (heartbeat + task start/success/fail), documented in `.env.example`, with unit tests. CI green. Approving. **Verification against the checklist — all items hold up:** - `hc_ping <which> [start|success|fail]` exists in `lib/healthcheck.sh`, uses `curl -fsS --max-time 10 -o /dev/null`, and always `return 0` (both `|| true` and the trailing `return 0`). ✅ - Heartbeat ping wired unconditionally after lock/log setup (`hc_ping heartbeat`). ✅ - Task start ping placed after `claude_health_blocked` gate with `HC_TASK_STARTED=1`; paired success/fail lives in `cleanup()` guarded on `HC_TASK_STARTED`. ✅ - Both vars documented in `.env.example`, default empty, exported with `:-` defaults so `set -u` is safe. ✅ - Tests cover unset/empty URL (no curl), correct URL construction for base/`/start`/`/fail`, state-omitted default, and curl-failure-still-rc0. ✅ **Correctness spot-checks:** - `cleanup()` captures `local rc=$?` *before* any `hc_ping` call, so the ping's curl exit status can't clobber the real exit code. Ordering is right. - The `HC_TASK_STARTED` guard correctly prevents false success/fail reports for ticks that bail at the health/deploy/cooldown gates (start never fired), while covering the cascade's early `exit 0`s and crashes via the EXIT trap. Sound design. - Test mock `curl() { CURL_URL="${*: -1}"; }` correctly grabs the last arg (the URL); `${*: -1}` slicing is valid bash. Assertions match the real call shape. - State defaults to `success` → base URL (healthchecks.io convention); consistent with the test `task (state omitted)` case. **Security:** URLs are trusted `.env`-sourced or unset; output discarded to `/dev/null`, stderr suppressed, `--max-time 10` bounds the network call. No injection surface. No CI/workflow files touched. Inert until #297 supplies real URLs. **Scope:** ~130 lines across 4 files, tightly focused on the one issue. No drive-by changes. Nothing blocking. Clean, well-tested, no-op-by-default change. --- <sub>Independent review by the harness on `claude-opus-4-8` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub> <!-- review sha=73acc5eee7460f779ebd5cf1e9ed69272ec779a0 verdict=APPROVE ci=success -->
igor added spent time 2026-07-04 19:54:40 +00:00
35 seconds
igor commented 2026-07-04 20:05:50 +00:00
Author
Collaborator
Copy link

CoS binding review: 🟢 GREEN — merge-ready.

Live-verified hc_ping: no-op when the URL is unset (rc 0, no curl), survives set -e, always returns 0 — a ping can never affect tick outcome. Correct healthchecks.io semantics (/start, /fail, base URL for success), 10s timeout, non-model (plain curl, runs even during a Claude cooldown).

Checked the correctness-critical wiring (the false-alarm trap):

  • task start sits AFTER the health/deploy/cooldown early-exit gates and sets HC_TASK_STARTED.
  • success/fail fires in cleanup() (EXIT trap), guarded on HC_TASK_STARTED and conditional on rc (0→success, else→fail).

So every exit path reports honestly:

  • a tick that exits before the cascade (cooldown/deploy barrier) → never pings (no false alarm);
  • an idle tick that passed the gates → exits 0 → success (start+success pair correctly);
  • a crash/hang mid-cascade → no cleanup runs → "started, never finished" → the monitor alerts. Exactly the dead-man's switch #297 wants.

New env vars default empty (set -u safe), documented, activation deferred to #297. 168 lines, CI green, test-healthcheck.sh included. Merge-ready — inert until you set the URLs.

**CoS binding review: 🟢 GREEN — merge-ready.** Live-verified `hc_ping`: no-op when the URL is unset (rc 0, no curl), survives `set -e`, always returns 0 — a ping can never affect tick outcome. Correct healthchecks.io semantics (`/start`, `/fail`, base URL for success), 10s timeout, non-model (plain curl, runs even during a Claude cooldown). Checked the correctness-critical wiring (the false-alarm trap): - `task start` sits AFTER the health/deploy/cooldown early-exit gates and sets `HC_TASK_STARTED`. - success/fail fires in `cleanup()` (EXIT trap), **guarded on `HC_TASK_STARTED` and conditional on `rc`** (0→success, else→fail). So every exit path reports honestly: - a tick that exits before the cascade (cooldown/deploy barrier) → never pings (no false alarm); - an **idle** tick that passed the gates → exits 0 → success (start+success pair correctly); - a crash/hang mid-cascade → no cleanup runs → "started, never finished" → the monitor alerts. Exactly the dead-man's switch #297 wants. New env vars default empty (`set -u` safe), documented, activation deferred to #297. 168 lines, CI green, `test-healthcheck.sh` included. Merge-ready — inert until you set the URLs.
joshtronic approved these changes 2026-07-05 00:50:06 +00:00
Dismissed
Merge branch 'master' into agent/339-feat-healthcheck-ping-wiring-lib-healthcheck-sh
All checks were successful
Lint / check-sync (push) Successful in 5s
Details
Lint / check-sync (pull_request) Successful in 4s
Details
78165dbc1f
joshtronic dismissed joshtronic's review 2026-07-05 00:50:11 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

joshtronic deleted branch agent/339-feat-healthcheck-ping-wiring-lib-healthcheck-sh 2026-07-05 00:50:39 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
Agent

   
Compat/Breaking
Breaking change that won't be backward compatible

  
Kind/Bug
Something is not working

  
Kind/Documentation
Documentation changes

  
Kind/Enhancement
Improve existing functionality

  
Kind/Feature
New functionality

  
Kind/Security
This is security issue

  
Kind/Testing
Issue or pull request related to testing

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Reviewed
Confirmed
 Issue has been confirmed

  
Reviewed
Duplicate
 This issue or pull request already exists

  
Reviewed
Invalid
 Invalid issue

  
Reviewed
Won't Fix
 This issue won't be fixed

  
Status
Abandoned
 Somebody has started to work on this but abandoned work

  
Status
Blocked
 Something is blocking this issue or pull request

  
Status
Need More Info
 Feedback is required to reproduce issue or to continue work

No labels
Agent
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
2 participants
Notifications
Total time spent: 35 seconds
igor
35 seconds
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
joshtronic/igor!348
No description provided.