joshtronic/igor

Watch

chore: fix(logwatch): suppress ticket filing when a Claude health backoff overlapped the reviewed hour #341

Merged

joshtronic merged 2 commits from agent/334-logwatch-suppress-tickets-when-a-claude-health into master

2026-07-04 14:09:10 +00:00

igor commented

2026-07-04 06:07:43 +00:00

Collaborator

What this PR does

fix(logwatch): suppress ticket filing when a Claude health backoff overlapped the reviewed hour
Add lib/logwatch.sh with logwatch_health_backoff_in_window (window-overlap check against .health.kind/first_failure/cooldown_until)
Wire the guard into do_logwatch_tick in bin/tick.sh, before the per-repo discovery loop, so the whole pass (not just filing) is skipped and logged
Add bin/test-logwatch.sh covering: no state file, healthy state, backoff fully inside the window, backoff straddling either edge, backoff entirely outside the window, and a non-auth/limit kind

Test plan

bash bin/check-sync.sh passes (runs every bin/test-*.sh, including the new test-logwatch.sh)
/security-review run on the diff: no findings
No manual verification needed beyond the above; this is a pure guard function with no UI/runtime surface

Closes #334

## What this PR does - [x] fix(logwatch): suppress ticket filing when a Claude health backoff overlapped the reviewed hour - [x] Add `lib/logwatch.sh` with `logwatch_health_backoff_in_window` (window-overlap check against `.health.kind`/`first_failure`/`cooldown_until`) - [x] Wire the guard into `do_logwatch_tick` in `bin/tick.sh`, before the per-repo discovery loop, so the whole pass (not just filing) is skipped and logged - [x] Add `bin/test-logwatch.sh` covering: no state file, healthy state, backoff fully inside the window, backoff straddling either edge, backoff entirely outside the window, and a non-auth/limit `kind` ## Test plan - [x] `bash bin/check-sync.sh` passes (runs every `bin/test-*.sh`, including the new `test-logwatch.sh`) - [x] `/security-review` run on the diff: no findings - [x] No manual verification needed beyond the above; this is a pure guard function with no UI/runtime surface Closes #334

igor added 1 commit

2026-07-04 06:07:43 +00:00

chore: fix(logwatch): suppress ticket filing when a Claude health backoff overlapped the reviewed hour

Lint / check-sync (push) Successful in 4s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

b915d360cb

igor commented

2026-07-04 06:09:51 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for b915d360: success

Adds lib/logwatch.sh with a pure window-overlap guard and wires it into do_logwatch_tick to skip the hourly logwatch pass when a Claude auth/limit health backoff overlapped the reviewed hour. Clean, focused (~110 lines), tests added, CI green, checklist matches the diff. One thing I can't verify from the diff keeps me off APPROVE.

Overlap logic is correct. first_failure < win_end && cooldown_until > win_start is the standard half-open interval overlap for [first_failure, cooldown_until) vs [win_start, win_end). The test matrix (inside / straddle-start / straddle-end / before / after / kind=other / missing file / cleared) exercises each branch and the expected rc's line up. Nice coverage.

Window computation is right. date -d '1 hour ago' ... %H:00:00 (prev hour start) → date ... %H:00:00 (current hour start) yields the hour that just closed, matching the "reviewed hour" intent.

Checklist honesty: every checked box maps to real diff content — the lib helper, the tick.sh wiring before the discovery loop, and the six-case test file are all present.

Open question (why COMMENT, not APPROVE):

return 1 semantics on suppression (bin/tick.sh ~L2478). The suppression path logs and then return 1 after logwatch_mark_done already ran. I can't see the caller of do_logwatch_tick or the other early-return paths in this function from the diff, so I can't confirm whether a non-zero return is treated as a benign skip or as a failure (which could trigger an error log, retry, or alert — the opposite of the intended quieting). Please confirm the caller ignores/expects the return code here, or that other intentional-skip paths in do_logwatch_tick also return 1 (consistency). If skips elsewhere use return 0, this should probably match.

Minor / non-blocking:

logwatch_health_backoff_in_window assumes .health.first_failure / .cooldown_until are integers; a non-integer value would make [ "$x" -gt ... ] error under set -u-ish contexts. Given these are epoch seconds written by lib/claude.sh this is almost certainly fine, just noting the implicit contract.
The documented MVP gap (a post-cooldown successful call zeroes the fields and erases the overlap) is honestly called out in the header — reasonable to defer.

No security or scope concerns. Resolve the return-code question and this is APPROVE-ready.

_{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 — `COMMENT` _(automated)_ CI for `b915d360`: **success** Adds `lib/logwatch.sh` with a pure window-overlap guard and wires it into `do_logwatch_tick` to skip the hourly logwatch pass when a Claude auth/limit health backoff overlapped the reviewed hour. Clean, focused (~110 lines), tests added, CI green, checklist matches the diff. One thing I can't verify from the diff keeps me off APPROVE. **Overlap logic is correct.** `first_failure < win_end && cooldown_until > win_start` is the standard half-open interval overlap for `[first_failure, cooldown_until)` vs `[win_start, win_end)`. The test matrix (inside / straddle-start / straddle-end / before / after / kind=other / missing file / cleared) exercises each branch and the expected rc's line up. Nice coverage. **Window computation is right.** `date -d '1 hour ago' ... %H:00:00` (prev hour start) → `date ... %H:00:00` (current hour start) yields the hour that just closed, matching the "reviewed hour" intent. **Checklist honesty:** every checked box maps to real diff content — the lib helper, the tick.sh wiring before the discovery loop, and the six-case test file are all present. Open question (why COMMENT, not APPROVE): - **`return 1` semantics on suppression (`bin/tick.sh` ~L2478).** The suppression path logs and then `return 1` after `logwatch_mark_done` already ran. I can't see the caller of `do_logwatch_tick` or the other early-return paths in this function from the diff, so I can't confirm whether a non-zero return is treated as a benign skip or as a *failure* (which could trigger an error log, retry, or alert — the opposite of the intended quieting). Please confirm the caller ignores/expects the return code here, or that other intentional-skip paths in `do_logwatch_tick` also `return 1` (consistency). If skips elsewhere use `return 0`, this should probably match. Minor / non-blocking: - `logwatch_health_backoff_in_window` assumes `.health.first_failure` / `.cooldown_until` are integers; a non-integer value would make `[ "$x" -gt ... ]` error under `set -u`-ish contexts. Given these are epoch seconds written by `lib/claude.sh` this is almost certainly fine, just noting the implicit contract. - The documented MVP gap (a post-cooldown successful call zeroes the fields and erases the overlap) is honestly called out in the header — reasonable to defer. No security or scope concerns. Resolve the return-code question and this is APPROVE-ready. --- <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>

igor added spent time

2026-07-04 06:09:51 +00:00

46 seconds

igor requested review from joshtronic

2026-07-04 06:09:52 +00:00

igor commented

2026-07-04 06:17:23 +00:00

Author

Collaborator

CoS binding review: 🟢 GREEN — merge-ready.

Verified the correctness-critical points:

Window-overlap, not claude_health_blocked ✓ — reads raw .health and tests first_failure < win_end && cooldown_until > win_start; the code comment correctly explains why (a backoff may have lifted by the time logwatch reviews the closed hour). Exactly the trap the spec flagged.
Guard placement + signal ✓ — after logwatch_mark_done (hour won't re-run), return 1 = suppress-and-continue-cascade, matching the function convention (if do_logwatch_tick; then exit 0). discretionary_state_file() resolves.
Window math ✓ — [1h-ago:00, this-hour:00) = the closed hour logwatch reviews.
Tests ✓ — 9 cases incl. both edge-straddles + kind≠auth/limit; CI green (test-logwatch.sh runs in check-sync).

Honest caveat (documented in-code, not a blocker): the post-recovery reset gap — once a successful call clears .health, the overlap history is erased, so a later pass reviewing a backoff-touched window won't suppress. This still covers the real #332/#333 incident (filed during the backoff); #340 (refinement) closes it via durable backoff-day tracking.

Merge-ready. (Shadow verdict COMMENT, non-blocking.) Split saga: #334 capped twice at full scope → re-scoped to this MVP, which fit in one attempt.

**CoS binding review: 🟢 GREEN — merge-ready.** Verified the correctness-critical points: - **Window-overlap, not `claude_health_blocked`** ✓ — reads raw `.health` and tests `first_failure < win_end && cooldown_until > win_start`; the code comment correctly explains why (a backoff may have lifted by the time logwatch reviews the *closed* hour). Exactly the trap the spec flagged. - **Guard placement + signal** ✓ — after `logwatch_mark_done` (hour won't re-run), `return 1` = suppress-and-continue-cascade, matching the function convention (`if do_logwatch_tick; then exit 0`). `discretionary_state_file()` resolves. - **Window math** ✓ — `[1h-ago:00, this-hour:00)` = the closed hour logwatch reviews. - **Tests** ✓ — 9 cases incl. both edge-straddles + kind≠auth/limit; CI green (`test-logwatch.sh` runs in check-sync). **Honest caveat (documented in-code, not a blocker):** the post-recovery reset gap — once a successful call clears `.health`, the overlap history is erased, so a *later* pass reviewing a backoff-touched window won't suppress. This still covers the real #332/#333 incident (filed *during* the backoff); **#340 (refinement) closes it via durable backoff-day tracking.** Merge-ready. (Shadow verdict COMMENT, non-blocking.) Split saga: #334 capped twice at full scope → re-scoped to this MVP, which fit in one attempt.

igor referenced this pull request

2026-07-04 06:41:15 +00:00

bump issue-work --max-turns to 100 (hardcoded constant — strong opinion, not a knob) #343

igor referenced this pull request

2026-07-04 06:59:13 +00:00

fix: bump issue-work --max-turns 50 -> 100 (hardcoded constant) #344

joshtronic added 1 commit

2026-07-04 14:08:44 +00:00