feat: chronic-day escalation + narration-line precision for the logwatch backoff suppressor #349

Merged

joshtronic merged 2 commits from agent/340-logwatch-de-noise-refinement-chronic-2-day into master

2026-07-04 23:28:13 +00:00

igor commented

2026-07-04 20:07:26 +00:00

Collaborator

What this PR does

feat: chronic-day escalation + narration-line precision for the logwatch backoff suppressor
lib/logwatch.sh: add logwatch_record_backoff_day / logwatch_chronic_backoff (distinct-day tracking, .logwatch.backoff_days state) and logwatch_strip_backoff_noise (removes only the backoff's own narration lines from a journal)
bin/tick.sh do_logwatch_tick/logwatch_review_unit: replace whole-pass suppression with line-level noise stripping for a transient (1-day) backoff, and skip the stripping once the backoff is chronic (>=2 distinct days) so the recurring pattern stays visible to the reviewer
bin/tick.sh logwatch_mark_done: fix a merge bug where the hourly stamp was replacing .logwatch wholesale, which would have wiped backoff_days every hour and made chronic detection unreachable
Tests covering the full matrix: transient-suppressed, no-backoff-normal, chronic->=2-day-files, unrelated-failure-during-backoff-still-files

Test plan

make test passes (full suite, incl. expanded bin/test-logwatch.sh)
make lint shellcheck: no new findings introduced (pre-existing lib/cost.sh SC2148 unrelated to this change, confirmed present on master before this branch)
/security-review run on the diff: no findings (state-only jq writes via --arg/--argjson, hardcoded grep patterns applied to local journal data, no new external input)

Closes #340

## What this PR does - [x] feat: chronic-day escalation + narration-line precision for the logwatch backoff suppressor - [x] `lib/logwatch.sh`: add `logwatch_record_backoff_day` / `logwatch_chronic_backoff` (distinct-day tracking, `.logwatch.backoff_days` state) and `logwatch_strip_backoff_noise` (removes only the backoff's own narration lines from a journal) - [x] `bin/tick.sh` `do_logwatch_tick`/`logwatch_review_unit`: replace whole-pass suppression with line-level noise stripping for a transient (1-day) backoff, and skip the stripping once the backoff is chronic (>=2 distinct days) so the recurring pattern stays visible to the reviewer - [x] `bin/tick.sh` `logwatch_mark_done`: fix a merge bug where the hourly stamp was replacing `.logwatch` wholesale, which would have wiped `backoff_days` every hour and made chronic detection unreachable - [x] Tests covering the full matrix: transient-suppressed, no-backoff-normal, chronic->=2-day-files, unrelated-failure-during-backoff-still-files ## Test plan - [x] `make test` passes (full suite, incl. expanded `bin/test-logwatch.sh`) - [x] `make lint` shellcheck: no new findings introduced (pre-existing `lib/cost.sh` SC2148 unrelated to this change, confirmed present on master before this branch) - [x] `/security-review` run on the diff: no findings (state-only jq writes via `--arg`/`--argjson`, hardcoded grep patterns applied to local journal data, no new external input) Closes #340

igor added 1 commit

2026-07-04 20:07:26 +00:00

feat: chronic-day escalation + narration-line precision for the logwatch backoff suppressor

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

Details

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

Details

bdb46c4e67

igor commented

2026-07-04 20:10:32 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for bdb46c4e: success

Adds chronic-day escalation + line-level noise stripping to the logwatch backoff suppressor (lib/logwatch.sh helpers, tick.sh wiring, a mark_done merge fix, and expanded pure-function tests). Well-scoped, honestly described, CI green — but two things I want a human to eyeball before merge.

Checklist honesty: every checked item maps to a real change in the diff (three new lib functions, tick.sh transient/chronic branch, the .logwatch = ((.logwatch // {}) + {hour}) merge fix, expanded tests). The test description is candid that the "full matrix" is exercised against the pure functions with the do_logwatch_tick decision narrated in comments, not end-to-end — acceptable for bash, and it says so rather than overclaiming.

Concern 1 — possible abort when the stripped journal is empty (can't verify from the diff). logwatch_strip_backoff_noise ends in grep -vE ..., which exits 1 when it filters out every line (the pure-noise case — precisely the transient-suppression happy path). In do_logwatch_tick this feeds journal=$(logwatch_strip_backoff_noise "$journal"). If bin/tick.sh runs under set -e, that assignment inherits rc=1 and aborts the tick before the intended if [ -z "$journal" ] skip check ever runs. I can't see tick.sh's set flags in this diff, so I can't confirm. If -e is in effect, add a || true (or return 0) so the empty-strip case degrades to the existing skip path. Worth confirming — the tests only call this function inside $(...) in a non--e test harness, so they wouldn't catch it.

Concern 2 — distinct-day set is never aged out, only capped. logwatch_record_backoff_day dedups and keeps the last 30 distinct days (unique | .[-cap:]), but nothing prunes by age. Two isolated blips months apart (e.g. one in January, one in July) will still register as 2 distinct days and trip "chronic," since neither ages out until 30 other distinct days accumulate. The design comment justifies day-granularity vs. timestamp-granularity but doesn't address stale entries surviving long gaps. Probably fine for the intended "recurring" signal, but it means "chronic" can fire on genuinely sparse, unrelated incidents. Flagging for a human call on whether that matters.

Nits (non-blocking):

logwatch_record_backoff_day's && mv ... || rm -f "$tmp" correctly leaves state untouched on jq failure — good, no corruption risk.
The grep pattern logwatch: [^:]+: (review call failed|no parseable review) matches any unit name, which is the intended downstream-noise scope; low risk of matching a genuine unrelated failure.

Not blocking on the count/scope/security fronts — jq writes use --arg/--argjson, patterns are hardcoded against local journal data. Just want the set -e/empty-strip interaction confirmed and the stale-day behavior acknowledged before a human merges.

_{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 `bdb46c4e`: **success** Adds chronic-day escalation + line-level noise stripping to the logwatch backoff suppressor (lib/logwatch.sh helpers, tick.sh wiring, a mark_done merge fix, and expanded pure-function tests). Well-scoped, honestly described, CI green — but two things I want a human to eyeball before merge. **Checklist honesty:** every checked item maps to a real change in the diff (three new lib functions, tick.sh transient/chronic branch, the `.logwatch = ((.logwatch // {}) + {hour})` merge fix, expanded tests). The test description is candid that the "full matrix" is exercised against the pure functions with the do_logwatch_tick decision narrated in comments, not end-to-end — acceptable for bash, and it says so rather than overclaiming. **Concern 1 — possible abort when the stripped journal is empty (can't verify from the diff).** `logwatch_strip_backoff_noise` ends in `grep -vE ...`, which exits **1** when it filters out every line (the pure-noise case — precisely the transient-suppression happy path). In `do_logwatch_tick` this feeds `journal=$(logwatch_strip_backoff_noise "$journal")`. If `bin/tick.sh` runs under `set -e`, that assignment inherits rc=1 and aborts the tick *before* the intended `if [ -z "$journal" ]` skip check ever runs. I can't see tick.sh's `set` flags in this diff, so I can't confirm. If `-e` is in effect, add a `|| true` (or `return 0`) so the empty-strip case degrades to the existing skip path. Worth confirming — the tests only call this function inside `$(...)` in a non-`-e` test harness, so they wouldn't catch it. **Concern 2 — distinct-day set is never aged out, only capped.** `logwatch_record_backoff_day` dedups and keeps the last 30 distinct days (`unique | .[-cap:]`), but nothing prunes by age. Two isolated blips months apart (e.g. one in January, one in July) will still register as 2 distinct days and trip "chronic," since neither ages out until 30 *other* distinct days accumulate. The design comment justifies day-granularity vs. timestamp-granularity but doesn't address stale entries surviving long gaps. Probably fine for the intended "recurring" signal, but it means "chronic" can fire on genuinely sparse, unrelated incidents. Flagging for a human call on whether that matters. **Nits (non-blocking):** - `logwatch_record_backoff_day`'s `&& mv ... || rm -f "$tmp"` correctly leaves state untouched on jq failure — good, no corruption risk. - The grep pattern `logwatch: [^:]+: (review call failed|no parseable review)` matches any unit name, which is the intended downstream-noise scope; low risk of matching a genuine unrelated failure. Not blocking on the count/scope/security fronts — jq writes use `--arg`/`--argjson`, patterns are hardcoded against local journal data. Just want the `set -e`/empty-strip interaction confirmed and the stale-day behavior acknowledged before a human merges. --- <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 20:10:33 +00:00

1 minute 15 seconds

igor requested review from joshtronic

2026-07-04 20:10:33 +00:00

igor commented

2026-07-04 20:24:58 +00:00

Author

Collaborator

CoS binding review: 🟢 GREEN — merge-ready.

Verified the refinement does exactly what #340 specced and closes the MVP's reset-gap:

Durable distinct-day tracking (.logwatch.backoff_days) survives the .health reset the MVP's live-state read couldn't — so a chronic backoff is caught even after recovery.
Integration in do_logwatch_tick: backoff-in-window → record the day; CHRONIC (≥2 distinct days) → narration stays visible → the reviewer files (chronic ≠ noise — your boundary restored); TRANSIENT (first day) → strips only the backoff's own narration lines → a genuine unrelated failure in the same window still files (better than the MVP's suppress-whole-pass).
Threshold LOGWATCH_CHRONIC_BACKOFF_DAYS=2 is a hardcoded constant, not an env knob. ✓
logwatch_strip_backoff_noise removes only backoff / auth-alert / review-call-failed lines; record_backoff_day preserves sibling .logwatch keys (.hour survives).

Comprehensive test matrix (chronic / transient / dedup / sibling-preservation / strip), CI green, 219 lines. Merge-ready. (Shadow COMMENT, non-blocking.)

**CoS binding review: 🟢 GREEN — merge-ready.** Verified the refinement does exactly what #340 specced and closes the MVP's reset-gap: - **Durable distinct-day tracking** (`.logwatch.backoff_days`) survives the `.health` reset the MVP's live-state read couldn't — so a chronic backoff is caught even after recovery. - **Integration** in `do_logwatch_tick`: backoff-in-window → record the day; **CHRONIC (≥2 distinct days)** → narration stays visible → the reviewer files (chronic ≠ noise — your boundary restored); **TRANSIENT (first day)** → strips only the backoff's own narration lines → a genuine unrelated failure in the same window **still files** (better than the MVP's suppress-whole-pass). - Threshold `LOGWATCH_CHRONIC_BACKOFF_DAYS=2` is a **hardcoded constant, not an env knob**. ✓ - `logwatch_strip_backoff_noise` removes only backoff / auth-alert / review-call-failed lines; `record_backoff_day` preserves sibling `.logwatch` keys (`.hour` survives). Comprehensive test matrix (chronic / transient / dedup / sibling-preservation / strip), CI green, 219 lines. Merge-ready. (Shadow COMMENT, non-blocking.)

joshtronic added 1 commit

2026-07-04 22:56:01 +00:00