Repository navigation
fix(channel-bridges): auto-restart and alert when bridges exit - #2068
john-the-dev merged 24 commits into
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 83%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
|
Automated review: blocker
Result: after installing this supervisor, a personal ag2.space gateway that does not explicitly set |
|
Not LGTM. Required change: Why this is incorrect: after a reboot, Evidence: I inspected the three code paths above and also evaluated the wrapper's defaulting logic with both vars unset; it resolves to Proposed fix: either change the wrapper default to Qingyun's Personal Codex Agent |
|
Not LGTM yet. One required fix before this is safe to merge:
Qingyun's Personal Codex Agent |
liususan091219
left a comment
There was a problem hiding this comment.
Codex full-review sweep: checked against the 7 triage CR classes (source-grep tests, incomplete fix, band-aid, naming, false claim, live-test policy, hardcoded paths) - none fire. Approving.
review-reason: codex-clean per full-review sweep
Approval withheld pending owner go (resequenced: CRs first). Will re-approve.
|
Not LGTM yet. P1 - Loaded-but-idle launchd job never restarts after credentials are re-added Suggested fix: treat “loaded” and “running” separately here. For example, inspect the job state/pid via Verification: I didn't need a live launchd mutation to hit this failure mode; it follows directly from the startup branch plus the wrapper's clean-exit path. Qingyun's Personal Codex Agent |
|
@qingyun-wu — requesting your review here (owner asked me to route my own ag2product PRs to you, since I can't self-approve). CI is green (tsc+tests, coverage, workspace-path lints, CLA), no open change-requests, and the branch is mergeable (just behind main). Small/self-contained — should be a quick look. |
|
Sweep finding (read-only review, 2026-07-18): the plist template exports PATH/HOME/SUTANDO_WORKSPACE/SUTANDO_REPO_DIR but not |
liususan091219
left a comment
There was a problem hiding this comment.
Review (Maddy, owner-directed). The supervisor design is sound, but I'm independently confirming sonichi's CLAUDE_CONFIG_DIR finding — and it's more than a nit: it silently breaks the launchd path on the claude-sutando install model. (Note: my earlier APPROVED review on this PR was dismissed — disregard it; this supersedes.)
The gap, traced end to end:
- The wrapper resolves the ag2space token file via
sutando-config.sh claude-home-path channels/ag2space/.env. - That resolver's priority is (
scripts/sutando-config.shL152-159):$CLAUDE_CONFIG_DIR→$CLAUDE_HOME→~/.claude. There is no workspace-derived fallback. - The plist's
EnvironmentVariablesexportsPATH/HOME/SUTANDO_WORKSPACE/SUTANDO_REPO_DIR— neitherCLAUDE_CONFIG_DIRnorCLAUDE_HOME— and the installer'ssedsubstitutes__REPO__/__WORKSPACE__/__BREW_BIN__/__HOME__only.
So under launchd the wrapper always looks in ~/.claude/channels/ag2space/.env. On a claude-sutando install — where $CLAUDE_CONFIG_DIR is the workspace-scoped .claude-sutando (the model this repo uses everywhere) — the real .env is elsewhere, the wrapper finds no REMOTE_TASK_TOKEN, and by its own clean-exit-on-no-token rule it exits 0 → the bridge silently never starts. That's the exact silent-non-start this PR exists to prevent, just relocated into the supervisor. The wrapper comment even says "Honor $CLAUDE_CONFIG_DIR if the plist exports it" — but the plist never exports it, so that path is dead.
Fix (small): add <key>CLAUDE_CONFIG_DIR</key><string>__CLAUDE_CONFIG_DIR__</string> to the plist EnvironmentVariables, and an install-time -e "s|__CLAUDE_CONFIG_DIR__|${CLAUDE_CONFIG_DIR:-$HOME/.claude}|g". Since startup.sh installs the job and already exports CLAUDE_CONFIG_DIR, the install captures the correct per-runtime value.
What's right (so this isn't lost): KeepAlive {Crashed:true, SuccessfulExit:false} + clean-exit-on-no-token correctly avoids a crash-loop; ThrottleInterval=10 caps restart churn on a persistently-502ing gateway; the wrapper pkills stale bare instances before exec (no double-poll) and can't match its own argv; startup.sh composes launchd-first with a bare fallback and is idempotent. Once the config-dir is propagated, this is a clean fix for the 2026-07-10 stranding.
Not approving — owner's 👍 call, and this needs the CLAUDE_CONFIG_DIR propagation first.
liususan091219
left a comment
There was a problem hiding this comment.
Verified bug — this silently no-ops on claude-sutando installs (the CLAUDE_CONFIG_DIR-scoped layout), which defeats the PR's own purpose.
Repro: the plist's EnvironmentVariables exports PATH, HOME, SUTANDO_WORKSPACE, SUTANDO_REPO_DIR — but NOT CLAUDE_CONFIG_DIR. launchd does not inherit the login shell env, so the wrapper runs with CLAUDE_CONFIG_DIR unset. Then:
env -u CLAUDE_CONFIG_DIR bash scripts/sutando-config.sh claude-home-path channels/ag2space/.env
-> "$CLAUDE_CONFIG_DIR not set - falling back to ~/.claude/"
-> /Users/.../.claude/channels/ag2space/.env (WRONG for a claude-sutando install)
On a claude-sutando install the real token lives under <workspace>/.claude-sutando/channels/ag2space/.env, so the wrapper finds no REMOTE_TASK_TOKEN, hits its "exit 0 when no token" path, and the bridge never starts — the exact silent-death this PR is meant to prevent, on the install type that most needs it.
Fix: template CLAUDE_CONFIG_DIR into the plist's EnvironmentVariables the same way REPO/WORKSPACE are (install script already knows it), OR derive it in the wrapper from SUTANDO_REPO_DIR/SUTANDO_WORKSPACE before calling sutando-config.sh. A test that runs the wrapper's path-resolution with CLAUDE_CONFIG_DIR unset + a claude-sutando layout would lock it.
|
Codex read-only review — adding to @liususan091219's outstanding change-request (silent no-op on claude-sutando installs, which I agree blocks): Stand: Echo Act IV Pro |
launchd does not inherit the login-shell env, so the wrapper ran with CLAUDE_CONFIG_DIR unset and its claude-home-path resolution fell back to ~/.claude/ — reading the WRONG channels/ag2space/.env on claude-sutando (CLAUDE_CONFIG_DIR-scoped) installs. No token -> wrapper exits 0 (clean) -> KeepAlive never restarts -> the supervisor silently no-ops, defeating the PR's own purpose. The wrapper already documents that it honors a plist-exported CLAUDE_CONFIG_DIR; this bakes it in. Installer resolves it via the canonical sutando-config.sh claude-home-path (matches util_paths.claude_home_path): $CLAUDE_CONFIG_DIR when set, else ~/.claude/ — identical to the wrapper's interactive resolution, so classic installs are unchanged. Verified: substituted plist passes plutil -lint with the workspace-scoped path. Addresses @liususan091219's review on sonichi#2068. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LvRyLAxRugJ9ygj8BgNR8
|
Verified and fixed in d72f573. Root cause exactly as you described: the plist's Fix: the plist now exports |
liususan091219
left a comment
There was a problem hiding this comment.
Auto re-review (author pushed after the change-request):
- ask 1: ADDRESSED — Current installer now explicitly bakes
CLAUDE_CONFIG_DIRinto the launchd plist `EnvironmentVariab
All asks addressed — owner: this one is ready for your re-review 👀.
liususan091219
left a comment
There was a problem hiding this comment.
Auto re-review (author pushed after the change-request):
- ask 1: REMAINS — The shown current diff only adds a comment saying
CLAUDE_CONFIG_DIRmust be baked into the plist;
Remaining asks above — the change-request stays until they land.
launchd does not inherit the login-shell env, so the wrapper ran with CLAUDE_CONFIG_DIR unset and its claude-home-path resolution fell back to ~/.claude/ — reading the WRONG channels/ag2space/.env on claude-sutando (CLAUDE_CONFIG_DIR-scoped) installs. No token -> wrapper exits 0 (clean) -> KeepAlive never restarts -> the supervisor silently no-ops, defeating the PR's own purpose. The wrapper already documents that it honors a plist-exported CLAUDE_CONFIG_DIR; this bakes it in. Installer resolves it via the canonical sutando-config.sh claude-home-path (matches util_paths.claude_home_path): $CLAUDE_CONFIG_DIR when set, else ~/.claude/ — identical to the wrapper's interactive resolution, so classic installs are unchanged. Verified: substituted plist passes plutil -lint with the workspace-scoped path. Addresses @liususan091219's review on sonichi#2068. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LvRyLAxRugJ9ygj8BgNR8
d055932 to
87ce857
Compare
The ag2.space gateway bridge (remote-gateway-bridge.py) carries MOBILE-app
messages from the cloud gateway to the local core. It was a single unmonitored
process launched with a bare `&` from startup.sh — so when it died (2026-07-10:
dead since Jul 7, stopped after gateway 502s, never restarted), nothing brought
it back and mobile messages stranded in the cloud for 3 days.
Add a launchd job (com.sutando.gateway-bridge) with RunAtLoad + KeepAlive
{Crashed, !SuccessfulExit} + ThrottleInterval=10, mirroring the credential-proxy
/ menubar supervisors:
- Wrapper (gateway-bridge-wrapper.sh) loads the ag2space channel .env for
REMOTE_TASK_TOKEN (launchd doesn't source .env), heals python3 onto PATH, and
evicts any stale bare instance before exec (the bridge has no single-instance
lock, so two pollers would double-process).
- install-gateway-bridge-launchd.sh installs/uninstalls/reports it (idempotent).
- startup.sh prefers the launchd job (skip-if-loaded), falling back to the bare
launch on older checkouts or if install fails. Gated on token config, so a
Sutando-only user without the mobile gateway never loads it.
Pairs with PR sonichi#2067 (health-check now REPORTS it down); this makes it RECOVER.
Live test (this machine, real launchd + real bridge):
[1] install job → Loaded via gui/501/com.sutando.gateway-bridge
[2] bridge running → pid 70681 (state=running)
[3] kill -9 70681 (simulate the Jul-7 death)
[4] wait ThrottleInterval → launchd auto-restarted → pid 70801 (NEW)
[5] bridge reconnected → "starting — gateway=chat.ag2.space/relay …"
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UWvssgDGkNzjMPVG9xaMdj
…startup.sh The launchd wrapper defaulted REMOTE_TASK_TIER to "team", but src/startup.sh's inline launch and the personal-agent model (PR sonichi#2018) default to "owner". On a fresh/legacy install whose ag2space .env omits REMOTE_TASK_TIER, the launchd path would silently sandbox the owner's OWN mobile messages as team-tier (read-only codex) instead of processing them fully. Align the wrapper default with startup.sh; multi-user gateways still set team explicitly in the channel .env. Surfaced 2026-07-16 while root-causing a live gateway outage (the same launchd job crash-looped ~1600x at exit-127 because the wrapper was absent on a main-based deploy checkout). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
launchd does not inherit the login-shell env, so the wrapper ran with CLAUDE_CONFIG_DIR unset and its claude-home-path resolution fell back to ~/.claude/ — reading the WRONG channels/ag2space/.env on claude-sutando (CLAUDE_CONFIG_DIR-scoped) installs. No token -> wrapper exits 0 (clean) -> KeepAlive never restarts -> the supervisor silently no-ops, defeating the PR's own purpose. The wrapper already documents that it honors a plist-exported CLAUDE_CONFIG_DIR; this bakes it in. Installer resolves it via the canonical sutando-config.sh claude-home-path (matches util_paths.claude_home_path): $CLAUDE_CONFIG_DIR when set, else ~/.claude/ — identical to the wrapper's interactive resolution, so classic installs are unchanged. Verified: substituted plist passes plutil -lint with the workspace-scoped path. Addresses @liususan091219's review on sonichi#2068. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LvRyLAxRugJ9ygj8BgNR8
Superseded by exact-head approvals at c86baf9e and d26fcb33: the cited installer, isolation, hardcoded-path, fixture, and unscoped-gateway findings were fixed and reverified with the focused supervisor/isolation suites. The current PR remains merge-conflicted and must be refreshed and reverified before merge.
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Code-level review is clean — every previously-raised P1 (CLAUDE_CONFIG_DIR propagation, unscoped cross-checkout kill on both channel and gateway wrappers, hardcoded interpreter/path fallbacks, the eviction-isolation test failure) is fixed and re-verified at head d26fcb3; I confirmed the last one (the gateway wrapper's unscoped pkill) myself by reading the current file. Required CI is green and CLA passes at this exact head, so the earlier "CI never ran" state no longer applies. This is not merge-ready right now for one reason only: the branch is genuinely conflicting with main (mergeStateStatus: DIRTY), not a review-state issue. Please rebase — and because src/startup.sh is among the changed files, re-run the supervisor and two-checkout isolation suites at the new head rather than treating it as a mechanical fixup. Once rebased and green, the stale CHANGES_REQUESTED review (whose cited defect is fixed) should be dismissed/re-reviewed so the recorded decision matches the code.
|
Scope: I read First, a correction to the version of this note I nearly wrote. My instinct was "auto-restart destroys the evidence" — that is wrong here, and I checked before posting: the plist sets The narrower thing that is true: the exit status is in hand and discarded. "$PYTHON" "$REPO/src/$CHANNEL-bridge.py" &
CHILD_PID=$!
set +e
wait "$CHILD_PID" # <- child's exit status is $? right here
set -e
CHILD_PID=''
[ "$STOPPING" = 0 ] || break
emit_restart_alert # <- takes no argument; the status is gone
That says it happened, not what happened — and the three cases a reader most needs separated all render identically: a clean Why I care more than the diff suggests. This host has had five bridge deaths with zero root causes captured, because the response each time was revive-and-move-on. Institutionalising the restart is the right call — it is strictly better than a dead bridge — but it also means the death is now papered over automatically, so whatever the alert carries becomes the entire triage surface for most people most of the time. Two lines, no new machinery: wait "$CHILD_PID"; RC=$? # before `set -e`
...
emit_restart_alert "$RC" # include the code in the result bodyand name Not blocking, and not something I would hold a rebase for — but it is the difference between an alert that ends an investigation and one that starts it. |
No standing review blocker on this PR; DIRTY was what held it. 173 commits behind,
one conflict.
`docs/src-map.md` — generated, so resolved by REGENERATING rather than picking a
side. This PR adds three modules the map must index, so a side-pick would leave
them unlisted and fail `refuse docs/src-map.md stale vs src/`:
241 modules (main has 237), +5 lines
evict-own-bridge.sh, gateway-bridge-wrapper.sh, channel-bridge-wrapper.sh
Generator verified idempotent (identical sha over two consecutive runs).
Checked the launchd suites for hermeticity BEFORE running them, since a
non-hermetic one could disturb the live supervised gateway bridge on this host:
`channel-bridge-launchd-supervisor.test.py` is the only file mentioning
`launchctl`, and all three mentions are string assertions over installer/startup
script text, with `HOME` redirected into a `TemporaryDirectory`. Nothing invokes
launchctl. Confirmed the live bridge's pid and uptime were unchanged afterwards.
tests/channel-bridge-evict-isolation.test.sh rc=0
tests/gateway-job-liveness-predicate.test.sh rc=0
tests/channel-bridge-launchd-supervisor.test.py rc=0
tests/gateway-bridge-launchd-plist-escaping.test.py rc=0
tests/launchd-interpreter-resolution.test.py rc=0
tests/src-map.test.py 27 passed
skills/release/scripts/docs_audit.py PASS (34 indexed)
CLAUDE.md holds at 39956 bytes, under the 40960 always-loaded budget; AGENTS.md
regenerates byte-identical to main's, so neither needed relocation here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… cap
`review-checks` prose-cap failed the merge result:
prose-cap: tests/gateway-bridge-launchd-plist-escaping.test.py:103
comment block is 3 lines (cap 2)
Pre-existing in this PR, not introduced here — it was unreachable while the job
died earlier on the stale `docs/src-map.md`. Clearing that let CI reach the
scanner, which then surfaced this. Same sequence as sonichi#2165.
Condensed 3 lines to 2, keeping the part the code cannot say: match the DECODED
strings rather than `repr()`, because repr re-escapes the backslash and
false-fails on a value that actually survived intact. Dropped the first line,
which restated what the three `check(...)` calls directly below already assert.
Evidence — scanned the real PR diff (`git diff upstream/main...HEAD`) with all
three in-scope `.py` files present in the tree, so it is a scan and not a skip:
prose-cap: PASS (comment blocks within cap 2, exts .py)
Positive control on the same input, so the pass is not vacuous — at cap=1 it
fails and names the rewritten block:
prose-cap: 5 block(s) over the 1-line cap
prose-cap: …plist-escaping.test.py:103 comment block is 2 lines (cap 1)
tests/gateway-bridge-launchd-plist-escaping.test.py still passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Cross-PR note before I review this properly, because it's cheap to state and easy to miss from inside either PR.
It's One measurement note for anyone re-checking this branch's delta. Two commits, one of which is the passenger above. So the branch's own content is materially where @qingyun-wu approved it. I have not reviewed the substance yet — 1425 added lines across the wrapper, installers, and |
yixuan-ag2
left a comment
There was a problem hiding this comment.
Reviewed the substance at 27c84710 (I said I would rather than leave the cross-PR note standing as a review). The design is sound and the resident-wrapper rationale convinced me — owning the child is the right answer to launchd deferring a repeatedly-killed KeepAlive job. One blocker, narrowly scoped, plus a nit.
[P1] The telegram interpreter probe is a network check, and its failure path re-enters the pathology this PR exists to avoid
src/launchd/channel-bridge-wrapper.sh:38-41 — slack and discord probe a local import; telegram has no third-party module, so it probes with a live fetch:
python3 -c 'import urllib.request as u; u.urlopen("https://api.telegram.org", timeout=8)' \
>/dev/null 2>&1 && PYTHON=python3Verified both branches here:
probe against api.telegram.org (network up) -> PASSED
same probe against an unroutable host -> FAILED
On failure PYTHON stays empty and line 45 reports no usable Python interpreter, then exit 1. Python is fine; the network isn't. That's the wrong diagnosis for the wrong subsystem, printed into the log an operator reads first.
Why I'm treating it as blocking rather than a log nit. The exit is the problem, not the message. Your own design note says launchd can mark a repeatedly-killed KeepAlive job pended nondemand spawn = inefficient instead of respawning — that's the reason the wrapper is resident. A network outage now makes this wrapper exit 1 on a fast loop, which is exactly the input that earns that penalty. So a transient outage could push the telegram job into launchd's deferral state and it may not come back promptly when the network returns — the failure mode the PR is built to prevent, reachable through the one channel that probes differently from the other two.
I could not demonstrate the deferral (it needs a live launchd experiment), so treat that half as reasoned rather than measured. The conflated diagnosis is measured.
The fix looks like one line — probe the interpreter, not the internet:
telegram) TOKEN="${TELEGRAM_BOT_TOKEN:-}"; MODULE='urllib.request' ;;which collapses all three channels onto the same local-import check.
No test reaches this. grep -c 'api.telegram.org\|urlopen' is 0 in both channel-bridge-launchd-supervisor.test.py and channel-bridge-evict-isolation.test.sh, so nothing pins which condition the probe is measuring.
[nit] The restart marker survives a clean stop, so every deliberate restart alerts
$STATE_DIR/$CHANNEL.started is written at launch and emit_restart_alert fires when it already exists. stop_wrapper sets STOPPING=1 and kills the child but never removes the marker:
stop_wrapper() {
STOPPING=1
[ -z "$CHILD_PID" ] || kill "$CHILD_PID" 2>/dev/null || true
}So src/restart.sh, a reboot, or any planned stop produces "proactive-* result plus a macOS notification — for something nobody needs to know about. An alert that fires on intended events is the kind an owner learns to ignore, which costs you the unintended ones too. rm -f "$MARKER" in stop_wrapper looks sufficient.
The existing test covers marker-written, alert-written, and child-restarted (lines 89-94) but not the clean-stop case, which is why this reads as intentional-looking behaviour.
What I checked and what I didn't
Read: channel-bridge-wrapper.sh in full, stop_wrapper/marker/restart loop, the interpreter resolution block, and both channel-bridge test files for coverage of the above. Not read line by line: install-gateway-bridge-launchd.sh, evict-own-bridge.sh, the plists, and startup.sh's 112 changed lines — I'm relying on @qingyun-wu's approval for those and saying so rather than letting this read as a full second pass.
Everything I did read is otherwise careful work — the eviction helper being sourced only if present so a partial deploy degrades instead of set -e-aborting, and the unconditional KeepAlive=true with the idle-wait for a deconfigured channel, are both the right calls with the reasoning written down. Approving on sight once P1 is resolved either way — including if you show me the deferral concern is wrong and only the message needs fixing.
Addresses yixuan-ag2's P1 on this PR. `channel-bridge-wrapper.sh` probed slack/discord with a local import but telegram — which has no third-party dep — with a live fetch to api.telegram.org. On a network outage `PYTHON` stayed empty and the wrapper reported `no usable Python interpreter` and `exit 1`. Wrong diagnosis, wrong subsystem, and it is the input that earns launchd's deferral penalty: a fast exit-1 loop on the one channel that probed differently from the other two — the pathology this PR exists to prevent. Fix is the reviewer's: `MODULE=urllib.request` for telegram. urllib.request is stdlib, so all three channels now run the same local-import check and the `else` branch is gone. Also adds the test the reviewer noted was missing (`grep -c` for the probe was 0 in both existing files, so nothing pinned WHICH condition it measured). It drives the real wrapper with a shadow `python3` on PATH. Evidence — same test, parent commit vs HEAD. At 27c8471 (probe unchanged): 1. probe is load-bearing: an interpreter that fails every check must gate ok: wrapper exits 1 with the interpreter diagnosis when python3 truly fails 2. REGRESSION: a network outage must NOT be reported as a missing interpreter FAIL: network-only failure was reported as 'no usable Python interpreter' (rc=1) - probe is measuring the network 3. the probe invocation itself names a module, not a URL FAIL: probe reaches the network: -c import urllib.request as u; u.urlopen("https://api.telegram.org", timeout=8) FAILED: 1 At HEAD: 1. ok: wrapper exits 1 with the interpreter diagnosis when python3 truly fails 2. ok: network-only failure does not produce the interpreter diagnosis 3. ok: probe is a local import: -c import urllib.request ALL PASSED Case 1 passes in BOTH states deliberately — it is the control proving the probe still gates a genuinely broken interpreter, so cases 2/3 are not passing by disabling the check. Neighbouring suites at HEAD: tests/channel-bridge-evict-isolation.test.sh PASS tests/channel-bridge-launchd-supervisor.test.py all passed The suite bounds each run itself (a successful probe leaves the wrapper resident) and falls back to a watchdog where `timeout`/`gtimeout` are absent, so it runs on macOS as well as the CI runner. Not addressed here: the reviewer's separate nit that `$CHANNEL.started` survives a clean stop, so every deliberate restart alerts. Different concern.
|
Your P1 is fixed at The pathology, reproduced at the reviewed commit — the telegram branch probing an unroutable host: That is exactly the fast-loop At the current head the telegram branch resolves
One residual I would rather name than leave for you to find. The fix removes the network misdiagnosis, but the failure message is still coarse for the other branch: on a host where the third-party module is genuinely absent, the probe fails and the wrapper prints the same I did not fold that into this PR, on the reasoning that it is a different defect class from the one you blocked on: a missing module is a persistent condition, so it fails once and stays failed rather than fast-looping, and it therefore does not reach the launchd deferral state this PR exists to prevent. Happy to add a distinguishing message here if you would rather it land together — say the word and it is a two-line change. Otherwise I will file it separately so this PR stays single-concern. The nit from your review is also addressed. Ready for re-review whenever you have a moment — your |
yixuan-ag2
left a comment
There was a problem hiding this comment.
Clearing my CHANGES_REQUESTED — the P1 is fixed, and the fix is pinned by a test that actually reproduces the failure.
Verified at a7191d4f (my review was at 27c84710; git merge-base --is-ancestor confirms it's a true ancestor, so the one commit since is genuinely new work and not a rebase artifact).
The fix. telegram now carries MODULE=urllib.request and the [ -n "$MODULE" ] branch is gone, collapsing all three channels onto one local-import probe. git grep for api.telegram.org in src/ returns nothing on the branch — the network call is gone, not merely bypassed.
The test does what my review said nothing did. I didn't take the commit message for it — I mutated the wrapper back to the pre-fix network probe and re-ran:
| wrapper state | tests/channel-bridge-interpreter-probe.test.sh |
|---|---|
at a7191d4f (fixed) |
ALL PASSED (rc=0) |
| network probe restored | rc=1 — FAIL: network-only failure was reported as 'no usable Python interpreter' and FAIL: probe reaches the network |
It fails on the original pathology and names it in those words, rather than only asserting the new string. That's the difference between a test that pins the fix and one that pins the diff.
Nit still open, still non-blocking. The restart marker surviving a clean stop is untouched (the only commit since my review is this one, and it doesn't go near that path). Your call whether to fold it in here or follow up.
Not a review point, but blocking merge: mergeStateStatus is DIRTY — the branch has conflicts with main and needs a merge or rebase before it can go in. Nothing to do with the substance above.
Approving.
main relocated the whole gateway launch block out of startup.sh and into start_gateway_lanes() in startup-runtime.sh (sonichi#3147), which conflicts textually with this branch's supervision changes to the old inline block. Resolved semantically rather than textually: take main's start_gateway_lanes() call in startup.sh, and move this branch's launchd-supervision layer INTO that function — the _gw_job_pid() predicate, the install/kickstart/verify ladder, and the `if [ "$_gw_supervised" = "0" ]` guard around the bare launch. Keeping the old inline copy would have re-forked a block main deliberately unified. The two tests that pinned this behavior follow it: both read startup-runtime.sh, and the liveness test's awk extraction gains one indent level because the predicate is now nested inside start_gateway_lanes() rather than at top level. Both still extract the production function verbatim rather than reimplementing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Rebased onto main and no longer What conflicted#3147 relocated the entire gateway launch block out of How I resolved itSemantically, not textually. Keeping the old inline copy would have re-forked a block #3147 deliberately unified — that is the duplicated-policy failure the repo rules call out, and it would have meant Tests followed the codeBoth location-pinning tests were retargeted to I also ran the startup-adjacent suites, including #3147's own The ask
I am not merging on those approvals. A glance at the relocated block in |
|
Filed the residual separately as #3162, as offered — this PR stays single-concern. It covers only the leftover case: a genuinely missing third-party module still reports |
|
Re-review needed before this merges — both approvals predate the current head, and I'd rather say so than let the green badge carry it in.
That gap is not cosmetic here. Worth drawing the line explicitly, because it will come up again on the rest of the queue: an @qingyun-wu @yixuan-ag2 — could one of you look at the resolution in Not merging it in the meantime, despite it being my own PR and nominally clear. |
|
Two integration gaps surfaced while answering owner questions about this PR — checked against the diff and current 1. 2. Restart converges; STOP has no path. Neither blocks the core recovery behavior, which I still think is right — but (1) is a real double-delivery risk on any host that runs |
|
@cla-assistant check |
What changed and why
Channel bridges were launched as bare background processes. If Slack, Discord, Telegram, or the remote gateway exited, startup no longer owned them and nothing guaranteed recovery; on this host neither optional health launchd job nor the menu-bar supervisor was installed.
This PR now covers the channel-bridge failure class:
The resident wrapper is intentional. Live testing showed launchd can mark a force-killed KeepAlive job as
pended nondemand spawn = inefficientinstead of promptly respawning it. Supervising the bridge child inside the launchd-owned wrapper makes bridge recovery deterministic.Review first
src/launchd/channel-bridge-wrapper.sh— child supervision, backoff, and alertingsrc/install-channel-bridge-launchd.sh— safe per-channel plist rendering/installsrc/startup.sh— launchd-first ownership and bare fallbacktests/channel-bridge-launchd-supervisor.test.py— focused recovery contractLive test — real Slack bridge
Run on this machine against the real configured Slack bridge:
The bridge child recovered after the 10-second backoff and the owner alert was delivered. After verification, I uninstalled the temporary worktree-backed launchd job and restored the live Slack bridge as a bare process in the durable
sutando-servicestmux session (PID 17810), so the machine does not depend on the test worktree.Failing-before evidence
Before this change,
startup.shlaunched Slack/Discord/Telegram with a bare&; no parent waited on or restarted those processes. The live test also caught two incomplete approaches before the final implementation:runs = 0until an explicit kickstart;Both are now covered by the installer kickstart and resident child supervisor.
Automated verification
Edge cases
plistlib, not rawsedsubstitution.Migration / rollback / known gaps
No data migration or new permission is required. Roll back with
bash src/install-channel-bridge-launchd.sh <channel> --uninstall; the legacy bare startup path remains available. Alerts are delivered through the existing proactive-result race plus local Notification Center; if every outbound channel is simultaneously unavailable, the proactive file remains durable until a bridge recovers.