Repository navigation
fix(health-check): make --fix restart bridges that are configured but not running - #1898
Conversation
… not running A dead telegram/discord bridge reports status "warn" (optional channels don't page), which excludes it from `issues` — so main()'s fix loop never reaches the bridge-restart branch and `--fix` leaves the bridge down while owner DMs queue channel-side. Observed 2026-07-02: discord-bridge died at boot with nothing logged; health-check --fix reported the memory issue but didn't touch the bridge, and 8 owner DMs sat undelivered until a manual relaunch. Add fix_down_bridges(), dispatched off the full `checks` list under --fix, mirroring the existing screen-capture warn-fix pattern. Exact-detail match on "configured but not running" keeps the other bridge warns (multiple PIDs, token invalid, stale log) out of the restart path — LoginFailure in particular would just crash-loop. Tests: tests/health-check-fix-down-bridges.test.py (4 cases). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9E832S1QZiwRdujtdKPs7
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Verified on a live install (Mac mini):
- The unreachability claim is correct:
issuesfilters outwarn, and "configured but not running" is warn-only, so the existing bridge branch at ~2188 can never fire for a dead bridge. Dispatching off the fullcheckslist mirrors the screen-capture precedent exactly as described. - The spawn mechanism (
Popen([sys.executable, ...], start_new_session=True), append tologs/<name>.log) is byte-for-byte consistent with the existing stale-restart branch (main:~2220) — including the sys.executable rationale. No new pattern introduced. - Ran the PR's test suite against the PR head with a real src/ tree: 4/4 pass (restart on exact detail, other warns untouched, non-covered check names untouched, ok/fail untouched).
- The exact-detail match doing the safety work is the right call: "configured but not running" implies pgrep found nothing, so there's no duplicate at spawn time; multiple-PIDs/token-invalid/stale-log all stay excluded.
One non-blocking policy observation from live operation today: on a multi-node fleet, a bridge can be configured but intentionally stopped on one host (we run Telegram on the peer node only — two pollers on one bot token 409-war each other). This fix would faithfully resurrect such a bridge on every --fix. That's not a defect in this PR — "configured" is the only signal it has — but it makes a config-level disable flag (e.g. TELEGRAM_BRIDGE_DISABLED=1 honored by both the check and the fixer) the natural follow-up so operators and --fix don't fight. Happy to file that as a separate issue.
LGTM.
|
Cold review — LGTM, two non-blocking nits. Ran the suite on the PR head: 4/4 pass (restart on exact match; other bridge-warns, non-bridge checks, other statuses all untouched). The exact-detail match ( Nits:
Also worth a comment breadcrumb on the check that produces the detail string, pointing at this matcher — the string is now a contract (the test locks it, which helps). |
…asing, detail-string contract breadcrumb Per qingyun's cold review on sonichi#1898: the parent's log file object now closes right after Popen (child keeps its dup), the --fix output says 'restart attempted' so the next health run stays the liveness source of truth, and the producer of 'configured but not running' notes that the string is matched verbatim by fix_down_bridges. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9E832S1QZiwRdujtdKPs7
|
Both nits taken in c01f58f: the parent's log file object now closes right after |
|
Re-reviewed c01f58f — delta is exactly the three items, all correctly done: |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approving — both review nits landed correctly: the with-block closes the parent's log fd right after Popen (child keeps its own dup, so the log stays writable — right semantics, and the comment states exactly the non-obvious constraint), and the 'restart attempted' phrasing now matches what the code can actually promise. The added contract comment tying the 'configured but not running' detail string to fix_down_bridges + the test is a nice touch — that string being load-bearing was the subtle trap here. CI green. Fittingly, this host had two bridge incidents yesterday where this fix would have been the first responder. Ready for owner merge.
|
Automated review:
This restart path still only covers Telegram and Discord, but the repo also has |
| restarted = [] | ||
| for c in checks: | ||
| if ( | ||
| c["name"] in ("telegram-bridge", "discord-bridge") |
There was a problem hiding this comment.
Automated review: fix_down_bridges() only matches telegram-bridge and discord-bridge here, but run_all_checks() also emits the same "configured but not running" warning for slack-bridge, and startup.sh already treats Slack as the same optional channel class. As written, health-check --fix still leaves a configured Slack bridge down indefinitely, so owner Slack DMs can keep queueing with no recovery path. Please include slack-bridge in the restart set (or explain why Slack should stay excluded while the other optional bridges auto-restart).
| # `with` closes the parent's handle after Popen; the child holds | ||
| # its own dup of the fd, so the log stays writable. | ||
| with open(str(log_path), "a") as log_f: | ||
| subprocess.Popen([sys.executable, str(REPO_DIR / "src" / f"{c['name']}.py")], |
There was a problem hiding this comment.
Automated review: launching discord-bridge with sys.executable is not equivalent to the normal startup path. src/startup.sh deliberately probes for an interpreter that can import discord before spawning this service; many installs run health-check.py under a Python that does not have discord.py, so this restart path will immediately crash while still printing restart attempted. The helper needs to reuse the same interpreter-selection logic (and Slack has the analogous slack_bolt constraint) instead of assuming the health-check interpreter can run every bridge.
|
Automated review:
|
|
Automated review:
|
| restarted = [] | ||
| for c in checks: | ||
| if ( | ||
| c["name"] in ("telegram-bridge", "discord-bridge") |
There was a problem hiding this comment.
Automated review: health-check already supports Slack as a first-class bridge, but this restart helper only handles Telegram and Discord. That leaves slack-bridge in the same broken state as before: a configured-but-dead Slack bridge still shows warn-only and --fix never restarts it. If the goal is to make --fix restart down bridges, this needs to include Slack or the regression remains for one supported channel.
… set fix_down_bridges() only restarted telegram-bridge and discord-bridge; startup.sh supervises slack-bridge the same way, so a dead Slack bridge stayed down after --fix. Add slack-bridge to the allowed-name guard and update the test to cover all three bridges. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EL2XBcV3RsWhevzTPtW4jw
|
Fixed in |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 50%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
…2349-2354 The coverage gate failed because fix_down_bridges() was only tested by calling it directly; the main() --fix code path that calls it and prints the restart messages (lines 2349-2354) was never exercised. Add case_e that mocks run_all_checks and fix_down_bridges, runs main() with --fix, and asserts the expected 'restart attempted' output lines appear. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EL2XBcV3RsWhevzTPtW4jw
|
Automated review:
|
|
Automated review:
|
|
Automated review:
|
|
Automated mergeability review (2026-07-09) Signal: NOT MERGEABLE Current blocker(s):
This is a conservative backlog triage signal. After the blockers are cleared, please rerun checks and request a focused review. |
…ke startup.sh Address qingyun-wu review on PR sonichi#1898: 1. run_all_checks() only emitted the "configured but not running" warn for telegram + discord, never slack — so fix_down_bridges()'s slack branch was dead code and the test was a false positive. Add slack-bridge to the bridge check loop so the warn is genuinely emitted (all Check 1-6 logic already accommodates slack). New case_f exercises the real run_all_checks() path and fails if the slack check is not emitted. 2. Restart used a bare `sys.executable src/<bridge>.py`, skipping startup.sh's bootstrapping — discord needs a discord.py-capable interpreter and slack needs its channel .env sourced (SLACK_BOT_TOKEN/APP_TOKEN), so --fix could crash-loop. Mirror startup.sh: probe the same interpreter candidates for the bridge's import (discord / slack_bolt) and inject channels/slack/.env into the child env. Fail-safe: skip the bridge if no capable interpreter or the slack tokens are missing, instead of spawning a guaranteed crash-loop. New case_g (parity) and case_h (fail-safe skip) cover it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DFp2p6XUSAQ1eAKKZeiZPt
…pers The diff-coverage gate flagged the two new module-level helpers added in 9cba55b as uncovered: the existing fix_down_bridges tests mock both helpers out, so their internals (interpreter probe branches, .env parse) never ran. Add hermetic cases calling the helpers directly: i) _bridge_interpreter → sys.executable when no import gate (telegram) j) _bridge_interpreter → probes candidates, skips missing, picks first import k) _bridge_interpreter → None when no candidate can import (rc!=0 + timeout) l) _load_channel_env → parses KEY=VALUE, honors `export `, strips quotes m) _load_channel_env → absent .env yields {} n) _load_channel_env → unreadable .env (OSError) yields {} Changed-line diff coverage for src/health-check.py goes 47.5% → 100%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DFp2p6XUSAQ1eAKKZeiZPt
liususan091219
added
bug
john-the-dev
added
the
ag2product
liususan091219
removed
the
ag2product
|
Confirmed live today: this fully fixes the down-bridge path — nice, One sibling gap this doesn't reach: the stale-code restart path (~ Since this PR already introduces |
…esolve trivial bridge-list wrap conflict (same content both sides; kept main's two-line form)
|
Maintainer rebase: merged latest main; the only conflict was a line-wrap difference in the bridges list in src/health-check.py (identical content both sides — kept main's two-line form). No changes to this PR's own logic. |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Thanks — the diagnosis is right (warn-status bridges never reach the --fix loop) and the fail-safe interpreter/env handling is careful. Requesting changes on evidence, not on the code.
This is a --fix behavior claim ("restart bridges that are configured but not running"), and the only proof shipped is a unit test of fix_down_bridges() plus the 2026-07-02 narrative. Per the owner's standing rule, a unit test of the sub-component is not a substitute for a significant before/after of the actual claimed behavior.
Please add a real before/after on a host that has the bridges configured:
- Before (main): stop discord-bridge, run
health-check.py --fix, show it reports the warn and leaves the bridge down (pgrep -f discord-bridgeempty after). - After (this branch): same setup, run
--fix, show the bridge is actually running afterward (pgrepfinds it,logs/discord-bridge.logshows a fresh boot).
I can only self-verify the safety side here (exact-detail match excludes other warns; skip when no capable interpreter / missing slack tokens). The spawn-actually-recovers-the-bridge path needs a host with the bridges + the right interpreters, which the author has and I don't — so I can't sign off on the core claim without the before/after.
|
Daily reminder — this PR has an open change-request from 2026-07-15 (1d) with no commits since. Push fixes or reply to the review to reset the clock; ~2 weeks of silence routes to final-notice and then close-as-abandoned per the triage policy.
|
|
Maintainer-run live verification — the evidence the CR asked for. Real invocation on the production install (no mocks): killed the running
Confirmed the bridge stayed up (pid survived 9s+) and restored the service after testing. This is the live before/after the CR requested — clearing my ask on the evidence front. (One aside I noticed doing this: |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
APPROVE - the CR asked for evidence not code changes, and I've now provided live maintainer-run before/after: killed the real telegram-bridge into warn status, ran health-check --fix from both main (leaves it down) and this PR's head (restarts it), real Terminal screenshot posted above. The fix_down_bridges() logic works against the real production bridge. Evidence bar met.
|
@cla-assistant check |
…ge-gate (Chi) Chi 2026-07-25 (#dev): "add in the reviewer's guide to ask: What is the worst case disruption to the Sutando users by this pr and how do we mitigate that. And the maintainer can check that question before merging too." Crystallizes lesson 5 (disruption-to-existing-users) into the explicit question every reviewer AND the maintainer-before-merge must answer, and folds in the #1898 auto-action angle (what code/state does it act on). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro
Each grounded lesson now cites its actual incident: #2 ← the #1600-M-fix muted-bot (#1663) + the #1403 migration stand-identity loss (#1540); #4 ← #1428's default-on watchdog loop (Chi 'nearly unusable', Rui's host bitten unnoticed); #5 ← #1898's latent bug in the un-asked worst-case. Corpus: sutando-pr-triage kb/breakage/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… + CI gate (#2281) * feat(review): repo-scoped REVIEW_GUIDE drives a generic checks runner + CI gate Splits generic review MECHANICS from repo-specific CRITERIA (design v3): - .github/REVIEW_GUIDE.md — single source of truth: prose lessons (fed to the codex reviewer) + a machine-readable `checks:` block (hardcoded-path flag/allow patterns). Add a lesson/check by editing this file, not the tooling. - scripts/review-checks.sh (+ review-checks.py) — standalone, codex-free runner: reads the guide checks: block and scans a diff. Generic-default fallback when a repo has no guide. Skips docs/tests/self (path literals are data there). - .github/workflows/ci.yml — runs the runner on every PR diff, so the guide gates ALL PRs unconditionally (not just codex-tool reviews). The big win. - review-pr.sh — calls the shared runner (supersedes the baked-in scanner), so the codex path surfaces the same mechanical findings. - tests/review-checks.test.sh — folds the #2229 hardcoded-path fixtures + guide patterns, fallback, per-token allow anchoring, file-skip (15 checks). Closes #2229 (its hardcoded-path scan now lives in the shared, guide-driven runner). Robust on macOS bash 3.2 (avoids heredoc-in-$() and O(pathological) ${var//…/} on large diffs). Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN * test(review-checks): Python unit tests for the scanner (diff-coverage) review-checks.py is exercised via the shell runner, so the python diff-coverage gate saw its new lines as uncovered. Add tests/review-checks-py.test.py: imports the module and pins token_at / allowed / main across all diff-parsing branches (+++/@@/-/+ , skip, comment, flag-found + allow anchoring, empty). pragma the __main__ guard. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN * fix(review-checks): advance line numbers across context * refactor(review): relocate REVIEW_GUIDE.md -> root REVIEW.md so Claude Code's built-in reviewers consume it Claude Code's managed GitHub-App PR reviews read a root REVIEW.md (injected into every review agent's prompt); the in-session /code-review reads CLAUDE.md but NOT REVIEW.md. Neither built-in reads .github/REVIEW_GUIDE.md. So: - git mv .github/REVIEW_GUIDE.md -> REVIEW.md (CC-native path; GitHub App + humans + codex + the checks: block now all read the one file). - Mirror the 7 canonical lessons into CLAUDE.md "Reviewing a PR" so the in-session /code-review applies them too (it cannot see REVIEW.md). - Repoint review-checks.sh default path, ci.yml, tests, and the review-pr.sh comment. - Keep the deterministic hardcoded-path scan + CI gate unchanged (tool-independent). Tests: review-checks.test.sh 15/15, review-checks-py.test.py 17/17; the gate parses checks: from REVIEW.md; the diff itself passes the gate (docs are .md-skipped). Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN * chore(review): sync AGENTS.md + merge main — clears qingyun's CR Regenerated AGENTS.md via scripts/agents-md-sync.sh so the new CLAUDE.md review criteria are mirrored (the 3 failing required CI checks were all this one omission). Merged origin/main to clear the behind-main state. Stand: Echo Act IV Pro * docs(review): make REVIEW.md the single source for review criteria Drop the duplicated 7-item review-criteria list from CLAUDE.md and replace it with a short pointer to REVIEW.md. The only consumer that required the criteria inlined in CLAUDE.md was the local /code-review command, which the owner is retiring (disable-model-invocation / user-triggered-only), so the mirror is no longer needed. Enforcement now flows through REVIEW.md alone: review-preflight.py reads and prints the criteria inline on every preflight run, scripts/review-checks.sh runs the machine checks: block in CI, and the managed GitHub-App reviewer reads REVIEW.md directly. Regenerated AGENTS.md from CLAUDE.md. Stand: Echo Act IV Pro * docs(review): update REVIEW.md note to single-source (drop stale CLAUDE.md-mirror text) The CLAUDE.md prose copy is gone; REVIEW.md is now the one source. Rewrites the note to reflect how the criteria reach reviewers (preflight print + review-checks.sh CI + GitHub-App direct read) instead of the removed sync. Stand: Echo Act IV Pro * fix(review-checks): stream diff via stdin + fail closed on scanner launch failure The runner passed the entire PR diff to review-checks.py through the RC_DIFF env var. On a large diff (~8MB, > macOS ARG_MAX of 1MB) the python3 exec hit 'Argument list too long', the scanner never launched, yet the shell discarded the exit code and still printed 'review-checks: PASS (hardcoded-paths clean)' and exited 0 — a silent false pass that lets a big PR bypass the scan while CI reports PASS (#2281, reported by qingyun's Codex). Fix: - review-checks.sh now STREAMS the diff to the scanner on stdin (printf, a bash builtin — no exec-size limit) instead of the RC_DIFF env var. Pattern lists (RC_FLAGS/RC_ALLOWS) stay in env; they're small. - Fail closed: capture the scanner's exit code. Non-zero (exec failure / crash) now errors out with a clear message and exit 2 — never PASS. Empty stdout is no longer misread as "clean". Exit contract preserved: 0=clean, 1=flagged, 2=usage error OR scanner-launch failure. - review-checks.py reads the diff from stdin (documented) instead of RC_DIFF. - Regression tests (shell + python) feed an oversized (>ARG_MAX) diff carrying a hardcoded /Users/alice/secret path and assert it's still flagged (exit 1, never a silent PASS). Both fail on the old argv/env code, pass on the new. Stand: Echo Act IV Pro * fix(review-checks): don't flag hardcoded paths inside docstring prose The scanner skipped `#`/`//` comment lines but not triple-quoted docstring prose, so a PR that ADDS a docstring *describing* a legacy path it removes got false-flagged (found live: #2313's `claude_home_path` fix documents the old `~/.claude/skills/` it replaces, and the runner flagged the doc line). Track triple-quote (`"""`/`'''`) state per hunk (reset at each @@ to avoid carrying stale state across a gap) and skip the flag-check for any line that STARTS inside a docstring — the opening `"""` line is still checked, and real code after the docstring closes is still scanned. Approximate by design (a path on the SAME line as a one-line docstring is still checked), but it clears the common multi-line case. Verified: the real #2313 diff now PASSES. 3 new cases in review-checks-py.test.py (docstring-prose skipped / code-after still flags / even-count triple-quote doesn't corrupt parity); 21/21 py + 16/16 shell green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * docs(review): add the worst-case-disruption question + maintainer merge-gate (Chi) Chi 2026-07-25 (#dev): "add in the reviewer's guide to ask: What is the worst case disruption to the Sutando users by this pr and how do we mitigate that. And the maintainer can check that question before merging too." Crystallizes lesson 5 (disruption-to-existing-users) into the explicit question every reviewer AND the maintainer-before-merge must answer, and folds in the #1898 auto-action angle (what code/state does it act on). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * fix(review-checks): fail closed in CI + scope docstring exemption to real docstrings (Qingyun review, #2281) Two blocking findings from Qingyun's review on head 4a6b1aa: 1. CI review-checks step fails open. `gh pr diff N | bash review-checks.sh` reports only the pipeline's last exit status, so a failed `gh pr diff` (auth/rate-limit/network) fed an empty diff to the scanner, which printed "empty diff — nothing to check" and exited 0 — the gate PASSED without ever scanning the PR. Now: `set -euo pipefail`, materialize the diff to a file, and scan it via `--diff` so a `gh pr diff` failure fails the step closed. 2. Triple-quote suppression was too broad. The #2313 docstring-skip toggled on ANY line with an odd triple-quote count, so an assignment like `COMMAND = <triple-quote> ... rsync /Users/alice/data ... <triple-quote>` exempted the hardcoded path inside it — a scanner bypass that could merge host-specific paths under a false-green gate. Replaced the count-only toggle with `_doc_transition()`, which opens the exempt span only when the stripped line STARTS with a triple-quote (a bare string-expression = real docstring); an assigned/called triple-quoted literal stays executable code and is still scanned. Added regressions: multi-line assigned string IS flagged; prefixed (r) docstring prose is NOT. All existing docstring tests still pass (23 py-internal + 16 shell checks green). AGENTS.md already mirrors the CLAUDE.md change on this head (sync check passes); the RC_DIFF-via-env finding was already resolved by the stdin-streaming path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * docs(REVIEW.md): ground lessons 2/4/5 in the real reported breakages Each grounded lesson now cites its actual incident: #2 ← the #1600-M-fix muted-bot (#1663) + the #1403 migration stand-identity loss (#1540); #4 ← #1428's default-on watchdog loop (Chi 'nearly unusable', Rui's host bitten unnoticed); #5 ← #1898's latent bug in the un-asked worst-case. Corpus: sutando-pr-triage kb/breakage/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: john-the-dev <johnv.the.dev@gmail.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com> Co-authored-by: Susan Xueqing Liu <xliu127@stevens.edu>
…wiring Diff-coverage gate was RED (<95%): the original 4 tests missed the except-OSError degrade-to-warn branch and the checks.append() call site inside run_all_checks(). Add: - test_stat_failure_warns_not_crashes — a stamp that exists() but whose stat() raises must warn, never propagate OSError and crash the health check. - test_run_all_checks_emits_comm_sweep — reachability guard (mirrors PR #1898): the probe is useless if defined but never wired in. Diff coverage now 100% (15/15 lines). 6/6 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro
…ing loud (P1) (#2321) * feat(health-check): comm-sweep staleness probe — make dead comm-handling loud (P1) P1 of the comm-handling overhaul. Comm handling ran as a *discipline* ("sweep every quiet pass"), so when its driver died (inbox-score loop, 2026-07-21) the owner-comm sweeps lapsed for days and nothing alerted — the recurring "you dropped comm handling" failure. check_comm_sweep_freshness() converts it to a *mechanism*: the comm-sweep driver stamps state/last-comm-sweep.json each run, and this probe pages when that stamp goes stale — warn >2h, down >6h, warn (not down) when absent (a host that hasn't wired the driver isn't broken, just un-adopted). Unlike quota-telemetry (absence-only), this IS age-checked: comm handling runs on a fixed cadence, so a lengthening age is the signal. Wired into run_all_checks after quota-telemetry. 4 regression tests (missing/fresh/lagging/stalled). Follow-ons (build order in the comm-sweep skill): the sweep script that writes the stamp, then the owner-task-gated cron that drives it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * test(health-check): cover comm-sweep OSError branch + run_all_checks wiring Diff-coverage gate was RED (<95%): the original 4 tests missed the except-OSError degrade-to-warn branch and the checks.append() call site inside run_all_checks(). Add: - test_stat_failure_warns_not_crashes — a stamp that exists() but whose stat() raises must warn, never propagate OSError and crash the health check. - test_run_all_checks_emits_comm_sweep — reachability guard (mirrors PR #1898): the probe is useless if defined but never wired in. Diff coverage now 100% (15/15 lines). 6/6 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…, not sys.executable (#2905) * fix(health-check): stale-bridge restart uses the shared launch policy, not sys.executable The --fix stale path spawned bridges with sys.executable, bypassing the interpreter probe + channel-env injection the down path (fix_down_bridges, PR #1898) already has. On a host where the health-check interpreter lacks slack_bolt, 'restarted (stale code)' killed a working slack-bridge and replaced it with an import crash-loop (live incident 2026-08-15, issue #2904). Extract the launch policy into _bridge_launch_plan (side-effect-free resolve) + _launch_bridge (spawn); both restart paths now share it. The stale path resolves the plan BEFORE killing the old process, so an unlaunchable bridge is left running (warning) instead of killed (outage). Closes #2904 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDMwd7PPV6CQhkMMKMUrpe * fix(health-check): require both Slack tokens in launch plan; trim comments to repo rule Per qingyun-wu review on #2905: the plan now validates SLACK_BOT_TOKEN AND SLACK_APP_TOKEN in the merged child env (the bridge exits without both), with stale-path regression case_r proving missing app token => no kill, no spawn. Comments/docstrings trimmed to <=2 lines; incident references moved out of code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDMwd7PPV6CQhkMMKMUrpe * fix(health-check): treat empty Slack tokens as missing in the launch plan Per qingyun-wu round-2 on #2905: gate on truthiness of both merged token values (empty string = missing, matching slack-bridge.py); case_q/case_r now cover present-but-empty app tokens on both the down and stale paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDMwd7PPV6CQhkMMKMUrpe * fix(health-check): bridge launch plan honors vault tokens and never execs bare python3 Slack launchability now resolves through the canonical tiering (process env -> channel .env -> Keychain vault via channel_token.token_from_vault), so a vault-only install is no longer skipped by --fix; vault values gate the plan as a boolean only and are never embedded in the child env. The interpreter probe substitutes _resolved_bare_python3 (startup.sh's resolve_python policy: $SUTANDO_PY -> bundled runtime -> non-stub PATH python3) for the bare candidate, so probing can never execute the Xcode-CLT stub and raise the developer-tools install modal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(health-check): cover resolver fallback branches; trim prose to comment cap Covers the three uncovered _resolved_bare_python3 branches (bundled-runtime return, no PATH python3, non-darwin PATH accept) so the 95% python diff-coverage gate passes, and trims added comment/docstring prose to the two-line cap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Chi Wang <4250911+sonichi@users.noreply.github.com>
You said you were not sure about this. Reading it back as a diff rather than as prose I was writing, two things were wrong with the shape: - lesson 5 already ends with a `*Grounded by:*` for #1898, so mine put two of them back to back inside one lesson — that reads as a mistake; - nine lines is long for an entry whose value is being scanned, and the rest of lesson 5 is semicolon-separated clauses in a single sentence. Now one clause in that existing list, keeping the content and the grounding reference, dropping the second grounding paragraph. 9 added lines -> 4. Stand: Echo Act IV Mini
…in the repo (#3047) * docs(review): removing a path — the callers that break are often not in the repo Lesson 5 asks the worst-case-disruption question and enumerates where to look: opt-in vs always-on, on-disk state-format migration, new hard-required config, process-global blast radius, and what an auto-action acts on. None of those names the case that broke #3005. The implementation moved to a skill and the PR deleted `scripts/pr_flag.py`. The in-repo grep was clean, and I treated that as the answer. It was an answer about the wrong population: a registered cron holds its own copy of the invocation, so no diff reaches it, and the host keeps calling the deleted path until someone re-runs /schedule-crons. The failure is silent — the hourly digest simply stops. The headline question would have caught it. The enumeration under it would not, and the enumeration is what a reviewer reads down. Adds the case plus the mitigation that closed it (a one-release forwarding shim), grounded on #3005 in the same form the existing entries use. Stand: Echo Act IV Mini * docs(review): shrink to one clause in lesson 5's existing list You said you were not sure about this. Reading it back as a diff rather than as prose I was writing, two things were wrong with the shape: - lesson 5 already ends with a `*Grounded by:*` for #1898, so mine put two of them back to back inside one lesson — that reads as a mistake; - nine lines is long for an entry whose value is being scanned, and the rest of lesson 5 is semicolon-separated clauses in a single sentence. Now one clause in that existing list, keeping the content and the grounding reference, dropping the second grounding paragraph. 9 added lines -> 4. Stand: Echo Act IV Mini --------- Co-authored-by: Chi Wang <sonichi@users.noreply.github.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
Problem
A dead telegram/discord bridge reports status
warn("configured but not running") because channel bridges are optional components. Butissues = [c for c in checks if c["status"] not in ("ok", "warn")]excludes warns, and the--fixloop only iteratesissues— so the bridge-restart branch at ~line 2188 is unreachable for the most common bridge failure mode.Observed 2026-07-02: discord-bridge died silently at boot (log got only the workspace legacy-state warning, no traceback — likely the machine's critical memory pressure).
health-check.py --fixran, reported the memory issue, and left the bridge down. 8 owner DMs queued channel-side undelivered until a manual relaunch, including an actionable owner request.Fix
Add
fix_down_bridges(checks), dispatched off the fullcheckslist under--fix— mirroring the existing screen-capture pattern (which has the exact same warn-only shape and already dispatches outside theissuesloop, see the comment at ~line 2299)."configured but not running"keeps every other bridge warn out of the restart path: multiple PIDs (needs a kill first), token invalid (restart would crash-loop on LoginFailure), stale log (handled by the main loop'sstalepath).sys.executable(launchd's minimal PATH would resolvepython3to the site-packages-less system python), append tologs/<name>.log,start_new_session=True.Tests
tests/health-check-fix-down-bridges.test.py— 4 cases: down bridges restarted; other bridge warns untouched; non-covered check names untouched; ok/down statuses untouched. Follows the module-load idiom of the existing health-check tests. All othertests/health-check-*.test.pypass excepthealth-check-notify-slack.test.pycase i, which fails identically on cleanorigin/main(the known channel-.env precedence thread, #1844/#1817) — unrelated.🤖 Generated with Claude Code
https://claude.ai/code/session_01F9E832S1QZiwRdujtdKPs7