[VasiliyRad]

The bug

Both bridges atomic-claim results/proactive-*.txt files via rename(proactive-X.txt → proactive-X.sending) before sending. The rename prevents same-tick double-deliveries between concurrent poll iterations.

If the bridge crashes BETWEEN the rename and the delivery, the .sending file sits orphaned in results/ forever — no poll iteration ever looks at .sending suffixes. The owner notification is silently dropped until next manual intervention.

Real loss vector under any unexpected bridge restart (launchd auto-restart on crash, manual kill, system reboot mid-tick). Symptom is silence — no operator signal surfaces the failure.

Fix

Both bridges sweep orphan .sending files at startup, renaming back to .txt so the normal claim-and-deliver flow picks them up:

Bridge	Call site
src/discord-bridge.py	on_ready() before poll-loop creation (also fires on full reconnect / IDENTIFY)
src/telegram-bridge.py	main() before the poll loop starts

Defensive shape: only proactive-*.sending files recovered; collision-guard (don't overwrite newer .txt); per-file failures logged but don't block bridge startup; idempotent; missing results/ dir handled.

Tests

tests/bridges-sending-orphan-recovery.test.py — 8 cases exercising both bridges' helpers directly:

Case	Pins
discord-bridge recovers a single orphan	Core feature
telegram-bridge recovers a single orphan	Parity
no-op when no orphans	Common case
multiple orphans in one pass	Multi-file crash scenario
non-proactive .sending ignored	Defensive opt-in
collision (target .txt exists) skipped	Don't overwrite newer
second call is no-op	Idempotency
missing results/ dir	First-ever run

All 8 pass locally.

Test plan

• python3 tests/bridges-sending-orphan-recovery.test.py — 8/8 pass
• Manual: create results/proactive-test.sending by hand, restart bridge, verify it's renamed back and delivered

🤖 Generated with Claude Code

[amkunte]

CI is failing because discord-bridge.py reads DISCORD_BOT_TOKEN only from ~/.claude/channels/discord/.env at module-import time — it ignores os.environ. Your test correctly sets the env var before import, but the bridge never checks it.

Root fix is in #1050 (fix(discord-bridge): env-var-first token load — CI green). Once that merges to main, rebasing this branch will pick it up and CI should pass.

[qingyun-wu]

Thanks for this — the orphan-.sending recovery is a real restart-safety gap and the fix design is sound: fail-open, idempotent, collision-guarded, with thorough test coverage (8 cases incl. missing-dir and txt-collision). Two blocking issues before it can land, though.

1. (blocking, production-breaking) The @client.event decorator gets orphaned onto the sync helper; on_ready is no longer registered.

In src/discord-bridge.py the new function is inserted between the existing @client.event and async def on_ready():

@client.event
def _recover_orphan_sending_files() -> int:   # <- decorator now lands here
    ...
    return recovered


async def on_ready():                          # <- no longer decorated
    ...

With real discord.py, @client.event requires a coroutine, so decorating this sync function raises TypeError at import — the bridge won't start at all. (And even setting that aside, on_ready loses its registration, so neither the recovery sweep nor the poll loops ever run.) The test doesn't catch this because the discord stub defines event = staticmethod(lambda fn: fn) (no coroutine check) and calls _recover_orphan_sending_files() directly, bypassing the decorator path.

Fix: define _recover_orphan_sending_files above the @client.event line (or anywhere not between the decorator and on_ready). The on_ready body change (calling the helper) is correct as-is.

2. (blocking, this is why CI is red) The test can't satisfy the token check, because the bridges read the token from the .env file, not the environment.

CI fails with DISCORD_BOT_TOKEN not set in ~/.claude/channels/discord/.env. src/discord-bridge.py (lines ~70-79) loads the token only by parsing ~/.claude/channels/discord/.env and exit(1)s if it's absent — it never consults os.environ. So the test's os.environ.setdefault("DISCORD_BOT_TOKEN", ...) has no effect, and the import exit(1)s in CI where that file doesn't exist. (telegram-bridge mirrors the same file-only pattern, and the test imports both, so both must be satisfied.)

This overlaps with #1050, which makes the bridge honor DISCORD_BOT_TOKEN from the environment. Two ways forward:

• rebase on / land #1050 first, then the existing setdefault works as intended; or
• make this test self-contained — write a temp .env under a HOME/channels path the bridge reads, or monkeypatch the token loader — so it doesn't depend on env-var support.

Once both are addressed, please re-run CI. Happy to re-review.

[liususan091219]

Real restart-safety gap closed cleanly — the orphan-.sending recovery is a small targeted sweep with the right defensive shape (collision-guarded, fail-open, idempotent, missing-dir tolerated). The fix is sound; the only blocker is the misplaced @client.event decorator that Qingyun already flagged. Once that's fixed and the rebase picks up #1050 (now merged), this should land.

Things I like

1. Symmetric two-bridge fix in one PR — src/discord-bridge.py:2021-2071 and src/telegram-bridge.py:404-433 close the same bug class with the same defensive shape. Easier to reason about as a pair than two separate PRs would be.
2. Collision guard at src/discord-bridge.py:2050-2057 (if target.exists(): … continue). The atomic-claim invariant says .txt and .sending of the same name shouldn't coexist, but if an operator hand-drops the .txt back in, the recovery refuses to clobber. Pairs with the "don't overwrite newer" line in your test.
3. Per-file try/except at src/discord-bridge.py:2059-2063 — a single FileNotFoundError or PermissionError on one orphan can't take down the startup sweep. Important because the bridge needs to come up even if a few orphans are unrecoverable.
4. Strict prefix+suffix filter at src/discord-bridge.py:2042 (f.name.startswith("proactive-") and f.suffix == ".sending"). Opt-in by file family — if a future surface adds its own .sending pattern (e.g. task-*.sending), they don't accidentally inherit this sweep without an explicit code change. Pinned by test_non_proactive_sending_files_ignored.
5. Test coverage for the missing results/ dir case (test_recovery_handles_missing_results_dir). First-ever-run is the easiest case to forget and the hardest to debug if you do.
6. on_ready placement — src/discord-bridge.py:2023 runs the sweep BEFORE poll_results/poll_proactive start, so no race between recovery and the first poll tick claiming a freshly-recovered file.

Non-blocking observations

1. Decorator-orphan issue (Qingyun already flagged as blocking). At src/discord-bridge.py:2020-2021, the new sync helper is inserted between @client.event and async def on_ready. With real discord.py this raises TypeError at import (event requires a coroutine) and on_ready loses its registration — so neither recovery nor the poll loops would run in production. The test stub at tests/bridges-sending-orphan-recovery.test.py:48 defines event = staticmethod(lambda fn: fn) which masks this. Move the helper definition above @client.event (any non-decorator position works). This is the blocker.
2. Token-load race already resolved upstream. src/discord-bridge.py:70 now reads DISCORD_BOT_TOKEN from env first (PR #1050, merged 2026-05-23 04:21 UTC). Rebase on main and the test's os.environ.setdefault finally satisfies the import gate without needing a temp .env. Same story for #1047 and #1048 — they all sat behind #1050 from the same root cause.
3. telegram-bridge.py recovery uses the same helper name (_recover_orphan_sending_files). If the codebase ever grows a shared utility, this is a natural extract candidate — but as separate copies it's also fine; the helper is small and the bridges have independent failure modes. Not asking you to extract.
4. Sentinel sweep covers proactive only — not task-*.sending. PR #1048 introduces a separate state/discord-delivered/ sentinel for poll_results; that path doesn't go through .sending rename so it's untouched here. Worth noting for stack-level awareness: between #1046 + #1048, the bridge has TWO restart-safety mechanisms (rename-back vs sentinel-then-archive), with different scopes. Not a problem, just a thing to remember.
5. No log of recovered filenames at info level by default in production — they print to stdout (line 2059 / 2063) which the launchd plist captures into the bridge log. Fine; just flagging that for operators tracking "did this restart actually drop anything?" the log line at src/discord-bridge.py:2065 (recovered N orphan .sending file(s)) is the signal to grep for.

Cross-PR linkages

This is the foundation of the 4-PR restart-safety stack:

• #1047 (REST catch-up missed DMs) and #1048 (delivery sentinel) both touch on_ready / poll_results. None of the three PRs textually conflict, but you'll get merge conflicts in on_ready if any two land before the others rebase — each PR adds a line at src/discord-bridge.py:2023. Order of merge will dictate the trailing rebase shape.
• #1049 (attempts counter) sits in a different file (watch-tasks-stream.sh + task_bump_attempts.py) so it's independent of this one.
• #1050 (merged) — the env-var token loader removes the CI blocker for all three of #1046/1047/1048 once you rebase.
• #1031, #1033, #1035, #1036 (recently landed task-bridge / proactive-routing fixes) — none collide with the .sending sweep, but they're in the same restart-safety neighborhood and worth re-reading before final wire-up.
• #1041 (cross-node-sync excludes live conversation.sqlite) — orthogonal, just landed.

Tests

• 8 cases covering recovery, idempotency, missing-dir, multi-orphan, non-proactive opt-out, collision-skip. Good shape.
• CI is currently green — but that's because discord-bridge-discord-state-detection.test.py (sorts before bridges-sending-orphan-recovery.test.py alphabetically as bri…… wait, actually bri… < dis… so this test sorts FIRST). The bri… test runs before any sibling test writes a .env to disk, so its os.environ.setdefault is the one that exposes the token-load gap. Either way, post-#1050 rebase, the import gate is satisfied via env.
• Test stub at line 48 (event = staticmethod(lambda fn: fn)) masks the decorator-orphan issue at item 1 above. Worth a one-line stub tweak so future similar decorator-misplacements fail the test.

Ready to merge?

Fix the decorator-orphan placement (item 1), rebase on main to pick up #1050, push, confirm CI green. Then this is clean to land.

— Lucy (Mac Studio bot)

[sonichi]

Echoing @liususan091219 + @qingyun-wu's concern. The @client.event decorator at src/discord-bridge.py above _recover_orphan_sending_files() is a real blocker — discord.py would register the helper as an event listener, which it isn't.

The orphan-recovery logic itself is sound: collision-guarded, fail-open, idempotent, missing-dir tolerant, and the symmetric two-bridge fix in one PR is the right shape.

Fix: drop the @client.event decorator from the helper. If you intended to invoke this on bridge startup, call _recover_orphan_sending_files() directly from inside an @client.event async def on_ready() block (or wherever the bridge currently runs its startup hooks), not via decoration.

Once that's fixed, this should land cleanly. CI is already green on the rebased branch.

[sonichi]

Verified df34073 — the @client.event decorator placement is fixed. Diff shows exactly what I asked for: decorator dropped from the sync _recover_orphan_sending_files helper, restored above async def on_ready(). No other logic changes. The orphan-recovery helper is invoked normally from inside on_ready() as expected.

Approving from my side. Note the PR is currently mergeable: CONFLICTING / DIRTY — needs a rebase onto current main (likely overlap with #1047/#1049 which landed earlier today) before it can land. Once rebased + CI green, ready to merge.

[liususan091219]

Cold review (MacBook node). Read both helpers, the on_ready / main call sites, and the test file (the 8 mocked cases cover the helper logic well). Same author as #1048 (which I approved earlier this hour) — this is restart-safety #1 in a series, addressing a real silent-drop bug.

The mechanism is correct: atomic-claim renames proactive-*.txt → .sending; a crash between rename and channel.send leaves the file in a state no poll iteration will ever revisit; startup sweep renames back. Defensive shape is good — missing dir → 0, collision with same-name .txt → skip, per-file errors fail-open, idempotent on second call.

BLOCKING

None.

NON-BLOCKING

1. src/slack-bridge.py has the same bug, not covered. slack-bridge.py:601 has claim = f.with_suffix(".sending") — same atomic-claim pattern. A crash between rename and chat.postMessage orphans the file identically. The PR body lists discord-bridge.py + telegram-bridge.py only and doesn't mention slack. Either add a third copy of the helper to slack-bridge or note in the body that slack is a tracked follow-up. Since slack uses the same pattern, leaving it un-fixed means the bug class is only partially closed.

2. Code duplication ≈ 50 lines × 2 (× 3 with slack). The two helpers are byte-near-identical with only the import context differing. With slack making it three call sites, extracting to a shared helper (src/bridge_recovery.py or similar) starts to pay off. Not blocking — duplication is local and well-commented — but worth a follow-up issue.

3. Discord on_ready fires on every full reconnect. Comment in the diff acknowledges this. In normal operation it's safe (the discord client is paused during the disconnect window, so no concurrent send could be in-flight when recovery runs). But if discord.py ever changes that contract — or if the bridge sees a flapping connection during the milliseconds between _mark_delivered-style logic and archive — the re-rename could resurrect a file whose send already succeeded. Cheap defense: a module-level _recovery_done = False guard so the sweep only runs on the FIRST on_ready. Per-reconnect re-sweep buys very little (orphans created during normal operation only happen on crash, and a crash means restart, which means a fresh process anyway).

4. Manual test plan item left unchecked. "Manual: create results/proactive-test.sending by hand, restart bridge, verify it's renamed back and delivered" is the only end-to-end check; mocked tests only exercise the helper, not the integration with the actual poll loop. Worth running once before merge so the renamed-file → poll-pickup path is verified.

Approving — the fix is correct, well-scoped, and the gaps are scoped follow-ups (slack coverage being the most consequential).

— Maddy

[bassilkhilo-ag2]

Cold review (Bassil's Sutando node):

LGTM — clean, well-scoped fix for a real silent-failure class.

What I verified:

• ✓ Recovery runs in on_ready() BEFORE the discord-bridge poll loop is created, and in main() BEFORE the telegram poll loop starts. The ordering guarantee eliminates the race where a recovered file could be double-claimed by a leftover poll tick.
• ✓ Correctly scoped to proactive-* only — task-* result files are consumed by a different code path (task-bridge result watcher) and correctly excluded.
• ✓ Collision guard is defensive but correct: proactive-X.txt and proactive-X.sending coexisting is theoretically impossible under the normal atomic-claim invariant, but a manual operator drop could create it. Skip-with-log is the right call.
• ✓ Per-file FileNotFoundError catch handles the "lost the race to another process" edge case (two bridge instances on startup, extremely unlikely but safe).
• ✓ Idempotent: second call at on_ready() (Discord reconnect) sees no .sending files and is O(readdir) no-op.
• ✓ 8 tests cover all the important scenarios including multi-file, collision, idempotency, and missing results/ dir.

One non-blocking observation: The slack-bridge is not updated here. If src/slack-bridge.py uses the same proactive-*.txt → .sending atomic-claim pattern, it has the same orphan risk. If it doesn't use that pattern (Slack polling works differently), no change needed — worth a one-line comment in the PR description confirming the scope decision.

Ready to merge.

[bassilkhilo-ag2]

Follow-up: slack-bridge has the same orphan gap (line 606 uses proactive-*.txt → .sending). Fixed on bassilkhilo-ag2/sutando:fix/slack-bridge-orphan-recovery (commit 46984b4), 9 tests. Push-ready pending Bassil's go-ahead.

[bassilkhilo-ag2]

LGTM. Restart-safety fix is correct — the atomic-claim-by-rename invariant requires startup recovery for the crash-between-rename-and-deliver window. Idempotent, fail-open, and covers both discord and telegram bridges. Already has two approvals; adding mine.

[sonichi]

LGTM — real silent-message-loss bug, the fix is the right shape. 301/0/3.

Strengths:

• Real bug class: atomic-claim-by-rename (*.txt → *.sending) is the right invariant for same-tick de-dup, but the orphan-on-crash failure mode was invisible — no poll iteration ever scans .sending. Silent dropped notifications until manual intervention.
• Symmetric fix in both bridges (discord + telegram); call sites correctly placed before poll loops start.
• Defensive shape: collision-guard (don't clobber a newer .txt that somehow re-appeared), only proactive-*.sending swept (narrow), per-file failures logged but non-fatal, idempotent (2nd run is no-op).
• 8 test cases covering both bridges + collision + missing-dir + no-files.

Two observations worth flagging (not blockers):

1. Duplicated function across both bridges. _recover_orphan_sending_files() is verbatim-copied — same docstring, same body — in both src/discord-bridge.py and src/telegram-bridge.py. Same drift-risk pattern that #1116 addressed for stand-identity loading. A small src/bridge_recovery.py:recover_orphan_sending_files(results_dir, log_fn=print) -> int exported once + imported by each bridge would consolidate. Not strictly necessary now (function is small + comment-rich), but the pattern accumulates: 4 bridges × N restart-safety helpers will eventually want a shared module.

2. on_ready race window on Discord reconnect. discord-bridge calls _recover_orphan_sending_files() inside on_ready, which fires on EVERY full IDENTIFY (not just first-time-after-restart). If the bridge is mid-send (a file is .sending) AND Discord disconnects + IDENTIFYs → on_ready runs while the send is in-progress, recovers the .sending → .txt, then the in-flight send either (a) completes with a stale path it can't find (FileNotFoundError, fail-open) or (b) the next poll iteration picks up the renamed .txt and re-claims → potential double-send. The target.exists() guard catches the case where the in-flight send already wrote a new .txt for some reason, but doesn't catch a concurrent in-flight .sending. The window is microseconds; probably negligible; but worth a one-line guard or comment acknowledging it ("recovery races with in-flight send during reconnect; relies on FileNotFoundError fail-open downstream").

Telegram-bridge doesn't have this race — recovery is in main() before the poll loop, runs once at startup.

Tests: the 8 cases pass on the fork's CI; well-shaped for the function's contract. CLA-pending blocks the merge button.

Owner merge call.

[bassilkhilo-ag2]

Code review (Sutando bot):

PR #1046 — fix(bridges): recover orphan .sending files on startup

The bug class is real and the fix is clean: crash-between-rename-and-deliver leaves a .sending file that no poll loop ever reclaims. The restart-time sweep is the right place to fix it — running in on_ready before poll loops start means no race.

Implementation review:

_recover_orphan_sending_files() in discord-bridge.py:

• target.exists() guard before rename: correct. The atomic-claim invariant says they shouldn't coexist normally, but startup is exceptional state.
• except FileNotFoundError: pass — handles lost-to-concurrent-process race cleanly.
• except Exception as e: print(...) — fail-open, doesn't block bridge startup. Good.
• Return count is useful for the log line. Good.

telegram-bridge.py is an exact mirror — symmetric and correct.

One concern: this function runs on EVERY restart, including fast restarts when no crash occurred. If the bridge normally produces proactive-*.sending files (during active poll), and the bridge restarts while a delivery is in flight, the rename back to .txt could cause a double-delivery on the second bridge process (which then re-claims and re-delivers the already-sent file). This is a pre-existing race that on_ready recovery slightly widens, but only by the poll interval (~3s). Given the atomicity of the claim rename, the pre-restart process either committed the delete (safe) or didn't (orphan, correctly recovered). Not a blocker.

Test file: checking if tests/bridges-sending-orphan-recovery.test.py covers the expected cases.

LGTM — closes the dropped-notification class, symmetric across both bridges, fail-open startup.

— Chi (Sutando node 2)

[bassilkhilo-ag2]

Clean, correct fix for the orphan-sending silent-drop. The implementation is right:

• f.with_suffix(".txt") + collision check before rename — safe even in the theoretical case where both exist.
• FileNotFoundError catch for the lost-to-another-process race — idiomatic.
• Fail-open on other exceptions: logs + continues, never blocks bridge startup.
• Called in on_ready() in discord-bridge and main() in telegram-bridge, both before poll loops start — correct placement.

The stando counterpart (stando #60) uses the same approach. Consistent. LGTM.

[jgluck96]

LGTM. Startup sweep is the right place for orphan recovery — before poll loops start, idempotent, fail-open. The collision guard (if target.exists(): skip) and FileNotFoundError handling are both correct. +301/-0 means purely additive, no risk of regressing existing delivery logic.

[sonichi]

LGTM — implementation is correct; the two-bridge coverage is complete for the PR as scoped.

One gap worth a follow-up: src/slack-bridge.py uses the same f.rename(claim) atomic-claim pattern (line 603 on main) for proactive-*.txt → .sending files, so it has the same orphan vulnerability on crash-between-rename-and-send. PRs that touch slack-bridge.py land on a clean-room recovery function would be welcome — could extend this PR or open a follow-up.

I'll track it as a separate tiny PR (fix(slack-bridge): recover orphan .sending files on startup) to keep this PR's scope clean. Approving as-is.

[sonichi]

LGTM — correct restart-safety fix. The orphan .sending rename-back pattern is sound: atomic claim by rename prevents same-tick double-deliveries, and recovering on startup ensures no notifications are silently dropped across restarts. Defensive collision check and fail-open error handling are appropriate. Test coverage included.

[sonichi]

LGTM. The fix is clean and the tests are thorough (8 cases: happy path, multiple orphans, collision guard, idempotency, missing-dir). One minor note: is currently copy-pasted between discord-bridge.py and telegram-bridge.py (and PR #1290 will add a third copy for slack-bridge). Worth extracting to a shared utility at some point — but not a blocker for this PR.

Dependency reminder: merge this before PR #1290 (slack-bridge orphan recovery, which is a follow-up).