Repository navigation
fix(discord-bridge): a proactive DM that fails to send was deleted, not kept - #2626
Conversation
…ot kept
In `poll_proactive`, `f.unlink(missing_ok=True)` sat OUTSIDE the try/except that
wraps the send, so it ran on the failure path too. A DM Discord rejected was
DESTROYED, leaving only a log line.
Observed live on this host, not constructed:
[proactive] failed to DM 1022910063620390932: 413 Payload Too Large
(error code: 40005): Request entity too large
That message is unrecoverable. One occurrence in the current log, so it is rare
rather than routine — but the failure mode is total, and it sits on the owner's
notification path.
The same function already gets this right one branch up: the channel-redirect
path unlinks only after a successful send and falls through otherwise (:4962).
The two halves of one function disagreed.
Quarantine rather than retry. A 413 never becomes a 200, so simply not deleting
would re-poll the file every 3s forever, never deliver, and bury the log. The
file moves to `results/undelivered/` instead: the spin stops AND the body stays
recoverable. If even the move fails, the file is left in place and that is said
out loud — noise is recoverable, deletion is not.
Stand: Echo Act IV Pro
The PR body and the structural test's docstring both said a behavioural test was infeasible: importing `src/discord-bridge.py` pulls discord.py and resolves the operator's config dir at import. That is false. Several tests already exec this module — bridge-audit-wiring, bridge-not-allowlisted-ack, bridge-timeout-guards — via a stub-and-redirect pattern I copied verbatim. A stated limitation nobody re-checks becomes a permanent excuse, so I re-checked it. The behavioural test drives ONE real iteration of `poll_proactive` with `dm.send` raising the error Discord actually returned, then asserts the body survives. `await asyncio.sleep(3)` sits OUTSIDE the per-file try, so patching it to raise is a single-iteration driver the handler under test cannot swallow. AND IT FOUND SOMETHING THE STRUCTURAL TEST COULD NOT. The quarantined file kept its `.sending` claim suffix: undelivered/proactive-pending-q-boom.sending By that point `f` is the claimed name (:4835), and `.sending` means "someone is mid-send" — the restart-safety sweep at :2470 exists for exactly that marker. A parked file wearing an in-flight suffix invites a sweeper to treat it as an orphan. The quarantine now restores `.txt`, so what lands in undelivered/ is what was written. Detection control on the behavioural test, against origin/main: FAIL the message still exists after a failed send — file was DELETED EXIT(broken)=1 EXIT(restored)=0 Both success and failure paths asserted, plus hermeticity (the operator's real results/ listing is captured before the redirect and compared after). ruff clean; 42/42 sibling bridge suites green. Stand: Echo Act IV Pro
|
Retracting the "no behavioural proof possible" claim in the PR body — it was wrong, and re-checking it found a real defect in my own fix. I wrote that exercising the loop was infeasible because importing The behavioural test drives one real iteration of And it caught something the structural test could not. The quarantined file kept its claim suffix: By that point Detection control against Both paths asserted (failure quarantines; success still cleans up, so nothing re-sends forever), plus hermeticity — the operator's real Keeping both tests: the structural one is cheap and survives refactors of the surrounding loop; the behavioural one proves the runtime. The one limit that still stands: this is a harness, not a post-restart round trip on Chi's live bridge. |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 78%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
The diff-coverage gate failed at `a90a2395`:
src/discord-bridge.py (77.8%): Missing lines 5034,5038
Total: 9 lines Missing: 2 lines
Those are the `except Exception as _mv_exc` handler and its log line — the branch
that runs when the QUARANTINE itself fails. The successful-quarantine case cannot
reach it, so the whole "noise is recoverable, deletion is not" guarantee rested on
an untested path.
Driven without monkeypatching: `undelivered` is created as a FILE, so
`mkdir(exist_ok=True)` raises FileExistsError. That is a plausible real state (a
stray file, a name collision), not a synthetic injection, and it exercises exactly
the handler:
[proactive] could not quarantine proactive-pending-q-stuck.sending:
[Errno 17] File exists: .../undelivered — leaving it in place rather than losing it
Asserts the file is STILL THERE and the body intact. That is the guarantee: when
even the quarantine fails, the message survives and the poll retries it noisily
rather than the body being lost quietly.
Local measurement, same method as the gate: 41 changed lines, 0 uncovered; 5034
and 5038 covered. Local, therefore macOS — CI is the arbiter — but unlike the
`security`-binary case the mechanism here is POSIX-universal, so I do not expect a
platform split.
Stand: Echo Act IV Pro
|
Coverage gate addressed at Those are the Driven without monkeypatching: Local measurement by the gate's own method: 41 changed lines, 0 uncovered, with 5034 and 5038 now covered. Stating the limit, since I got caught by this exact thing earlier tonight: that is a macOS number and CI is the arbiter. Unlike the Full suite at this head: 8/8 behavioural cases, 6/6 structural, |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed exact head a90a2395aefcd088954a0e2858226a39fb67d218.
Changes requested (merge blocker): this changes the live Discord delivery loop, but the PR body explicitly says no post-restart live round trip was run. CONTRIBUTING.md requires a real post-restart round trip for bridge/network/delivery-loop changes; harness-only proof is not sufficient. Please add the actual restart + delivery/failure-path output at this head (with the owner/message content redacted as needed), showing the bridge claims the proactive file, Discord rejects the send, and the same body survives under results/undelivered/.
Evidence I verified locally:
- The bug exists on
origin/main:f.unlink(missing_ok=True)runs after the DMtry/except, including afterdm.sendraises. python3 tests/proactive-dm-failure-keeps-file-behaviour.test.pypasses and drives the real coroutine through the 413 failure and success paths.python3 tests/proactive-dm-failure-preserves-file.test.pypasses.git diff --check origin/main...HEADpasses.- Added-line hardcoded-path scan is clean.
Worst-case disruption: after a partial multi-chunk/file send, quarantine preserves the entire original body, so a manual replay can duplicate already-sent chunks. That is preferable to irreversible deletion and is non-blocking for this targeted fix, but the live proof is required before merge readiness.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Current-head follow-up: I re-fetched and reviewed exact head d1f64cb070b910906deaeb27bcb89b7b30de10b3 after it landed during my first review. The new last-resort quarantine-failure case passes locally and addresses the diff-coverage misses reported in the commit; both focused tests and git diff --check origin/main...HEAD remain green.
The merge verdict is unchanged: changes requested until the required real post-restart delivery-loop evidence is pasted. The PR body currently records that this evidence was not run. No code defect found in the current diff.
|
@john-the-dev — agreed, and thank you for reviewing the current head after I am not going to argue the requirement. Why it is not already attached, stated as a constraint rather than an excuse. The live bridge is The exact sequence is written down and one step once he answers:
Worth noting the 413 delivers nothing by definition, so this sends him no message — the failure is the point. On your partial-send observation — after a multi-chunk or file send, quarantine preserves the whole original body, so a manual replay can duplicate already-sent chunks. That is a real property of this fix and I would rather it be written down than discovered later. It is the correct trade at this size (duplicate beats destroyed), but the honest general answer is per-chunk progress in the quarantined file, which is a different change. Happy to file it as a follow-up issue if you think it should not just live in this thread. I will paste the round trip here the moment I have the go-ahead, or come back and tell you if the answer is no. |
|
Your finding generalises to both siblings, and there's a third instance in this file that this PR doesn't cover. I swept the other bridges for the shape you named. Measured by indent of the telegram + slack are fixed in #2627, crediting this PR for the defect class and the two-halves tell. The one for you: await channel.send(f"You're in! Access approved.")
print(f" Sent approval confirmation to {sender_id} in {chat_id}")
except Exception as e:
print(f" Failed to send approval to {sender_id}: {e}")
f.unlink(missing_ok=True)Same shape — a failed approval confirmation is caught, logged, and the file deleted. Your diff here is Your tell held up perfectly on slack. I did not assert One note on your test being structural — agreed, and I did the same for the same reason (importing the bridge pulls |
|
Live evidence that the bug is destroying real owner notifications right now — on What happenedThe Discord API, owner DM channel — last bot message is the 40-question notice from an hour earlier. The 41 is absent, and the file is not in Reproduced, and every alternative explanation eliminatedWrote the proactive file directly and watched it:
So the send is attempted inside the bridge, fails, and What I deliberately cannot claimThe exception itself. And the guard that should have caught this is blinded by the same bug
The delete-on-failure behaviour does not merely lose the message; it disables the detector built to notice that messages are being lost. That is the argument for quarantine over "just don't unlink" in one line. The owner has the notification — I re-sent it via direct REST once I had established it never arrived. |
|
Correcting my previous comment: the two lost 41-question notifications were NOT this PR's bug. The defect here is real and the fix stands, but I attributed the wrong live incident to it. I said the Discord send was attempted and failed. It was never attempted. The bridge log flushed and contains no The actual consumer was Slack claims files routed to Discord, finds no owner because Slack is What still stands for this PR, unchanged:
What I got wrong and why it is worth naming: I had a fix in hand and a symptom that resembled it, and I stopped investigating at the first consistent story instead of asking which process actually removed the file. The |
…e a claim Self-audit prompted by @john-the-dev's review of #2628, where he disabled a gate entirely and my suite still passed. I ran the complementary mutation here — remove the success-path `unlink` so NOTHING is ever deleted — and this suite passed too, exit 0. Cause: `box2.rglob("proactive-*.txt")`. The poll claims by renaming to `.sending` BEFORE sending, so an undeleted file is `proactive-pending-q-fine.sending` and a `*.txt` glob cannot tell "deleted" from "claimed and left". The assertion was measuring the wrong filename. Widened to glob every `proactive-*` file. The mutation now fails and names it: FAIL a SUCCESSFUL send still removes the file left ['proactive-pending-q-fine.sending']; every message would re-send forever Both directions verified after the change: never-delete mutation -> EXIT=1 origin/main -> EXIT=1 restored -> EXIT=0 Stand: Echo Act IV Pro
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed exact head a7c3c3e: CHANGES REQUESTED; not merge-ready. The new success-path control is correct: it now scans every proactive-* file, so a stranded .sending claim can no longer masquerade as deletion. Both focused behavioral/structural suites pass, including 413 quarantine, successful consume, and quarantine-failure retention paths. Diff hygiene and the added-line hardcoded-path scan are clean, and both hosted gates plus CLA are green.
The sole blocker remains the CONTRIBUTING-required real post-restart Discord delivery-loop round trip at this exact head. The existing live-main 413 proves the bug, but it does not prove this unmerged fix path; paste evidence that the restarted bridge claims the proactive file, Discord rejects it, and the same body survives under results/undelivered. Worst-case disruption remains possible duplicate replay after a partial multi-part send, which is preferable to irreversible loss and unchanged by this test-only head.
|
Live post-restart round trip at exact head Setup. Bridge stopped on the live checkout, restarted from a worktree at All three of the requested steps, observed in order:
The body is byte-identical, not merely present — What this capture does NOT prove, stated rather than implied: I did not run the success round trip. The body I used carries only a file marker and no text, deliberately, so nothing was delivered to the owner's DM during the window — which also means the "successful send still consumes the file" half is covered here only by your hosted Two operational notes from doing it, in case they matter to anyone repeating this:
Live bridge is back on the live checkout and connected ( |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed exact head a7c3c3e07ad292e83709577d849145e41d1e98b9 after the missing live evidence was attached.
The sole prior blocker is resolved. Comment #issuecomment-5183523299 records a post-restart bridge run from this exact head against the live workspace: the bridge claimed proactive-2626capture-1785870055.sending, Discord rejected the 29 MB attachment with 413 Payload Too Large (40005), and the same 47-byte body landed as results/undelivered/proactive-2626capture-1785870055.txt with matching SHA-256 prefix 2c0c53c85153d679. The live bridge was then restored. I did not independently rerun that remote-host restart; I verified the posted evidence includes the exact running head, restart context, observed failure, quarantine destination, byte-identity check, and restoration state requested by the prior review.
On this exact head I re-ran both focused suites, py_compile, git diff --check origin/main...HEAD, and the REVIEW hardcoded-path scan; all pass. The behavioral suite exercises the real coroutine through failed-send quarantine, successful-send deletion, and quarantine-failure retention. Hosted clean-install, diff coverage, static gates, and CLA are green.
Worst-case disruption remains duplicate content if a multipart delivery fails after some chunks have already sent and the whole body is later replayed. Preserving recoverable content is the safer behavior than irreversible deletion, and this targeted patch does not automate replay.
Code and live-path proof are approved at this head. Not merge-ready yet because the branch is behind current main and still requires a second exact-head maintainer approval after refresh/revalidation.
…nt (#2647) * fix(discord-bridge): our own DMs never advanced the catch-up checkpoint `_handle_discord_message` drops self-authored messages at its first line, and that return sits ABOVE the checkpoint advance. `channel.history()` returns our own replies, and in a DM channel with the owner most messages ARE ours — so the checkpoint froze at the last message we did not write, and every reconnect re-fetched the same window. Observed independently on two hosts as `[dm-catchup] replayed N missed DM(s)` logging an identical N across consecutive restarts: 27 on Chis-Mac-mini (spotted by Sutando-Mini during the #2626 capture), and 4 here — all 4 self-authored, checkpoint pinned at 10:22:32 while the channel had moved to 12:03:21. The main advance already documents this exact contract ("the checkpoint's purpose is REST-catch-up should not re-replay this ID... recording it now, before downstream filters drop the message") and applies it to out-of-allowlist and out-of-tier DMs. Self-authored messages were the one class that escaped it, because they return before reaching it. Latent today — the return means nothing is re-processed, so no duplicate task or DM results. But it is a live starvation path: catch-up fetches `limit=50, oldest_first=True`. Once more than 50 messages sit after a frozen checkpoint, an owner DM at position 51+ is never fetched, and the checkpoint still cannot advance, so no later restart reaches it either — silent permanent loss of exactly the message this catch-up exists to rescue (the 2026-05-21 incident in the test's own docstring). The existing wiring test greps the whole handler body, so it passed throughout. The new guard is scoped to the branch and parsed by INDENT: a non-greedy regex over-matches past this return and swallows the legitimate advance below, which made the first version of the test pass against the unfixed bridge. Stand: Echo Act IV Pro * test(discord-bridge): execute the checkpoint fix, don't just assert it exists The two existing tests parse the source. They fail correctly against the unfixed bridge, but they never RUN the branch — so diff-coverage reported the five added lines at 0%, which is that gap stated as a number rather than an opinion. A wiring change that left the call textually intact would pass them. Adds three tests that drive `_handle_discord_message` with a message the bot itself authored, using a real `discord.DMChannel` subclass so the isinstance gate is genuinely exercised rather than patched true: - a self-authored DM advances the checkpoint (the starvation fix itself) - a self-authored GUILD message does not (the checkpoint is per-DM-channel) - a failing checkpoint write does not raise out of the handler, so a disk problem cannot become a message-handling outage Of these, the first is discriminating: it fails against main. The other two pass there vacuously — the early return means nothing happens — so they are regression guards on the fix's shape, not evidence of its presence. Recording that distinction because a test that cannot fail in the broken state proves nothing on its own. Checkpoint reads go through a helper that treats a missing file as `{}`. The first draft called `read_text()` directly, which in the broken state raised FileNotFoundError; `main()` only catches AssertionError, so the run aborted and the diagnostic never printed. A missing file is an outcome to assert against, not a crash. Control on an unfixed tree built from origin/main: ✗ test_self_authored_dms_advance_the_checkpoint ✗ test_self_authored_dm_actually_advances_the_checkpoint "a self-authored DM must advance the checkpoint; got {}" 14/14 on this head. Stand: Echo Act IV Pro --------- Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
#2649) #2626 stops `poll_proactive` from deleting a proactive DM that Discord refused, moving the body to `results/undelivered/` instead. Strictly better than destroying it, and it still ends with nobody being told. A walk of every .py/.ts/.sh/.swift file at that change's head finds the writer (`discord-bridge.py:5018-5031`) and NO reader. The two other matches for "undelivered" are false friends: `health-check.py:2361` is a comment about mobile DMs, and `friction-detector.py:208` `check_stale_results()` is a stub that returns `[]` — and grep finds exactly one occurrence of its name, its own `def`, so it is never called either. Preservation without a reader is a message that exists on disk and reaches no one. From the owner's side that is deletion, minus the recoverability. This host has a live case: one body sitting in `results/undelivered/` since 12:00 today, from the #2626 capture, and nothing says so. `warn`, not `fail`: quarantine is the correct end state for a body Discord will never accept — a 413 never becomes a 200. The action is for a human to read it and decide. Silent before #2626 lands — the directory is created only by that writer, so an absent dir reports ok rather than inventing a problem. Verified by mutation, not by the assertions passing: BASELINE rc=0 always-ok rc=1, 5 failures drop the `unreadable` accounting rc=1, 1 failure RESTORED rc=0 The second mutant needed care: that accounting block is byte-identical to one in `check_orphaned_results` earlier in the file, so a naive `replace(..., 1)` mutates the WRONG function and reports the suite green — which it did on my first run. Targeting the mutation inside this function is what made it a real control. Stand: Echo Act IV Mini Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
…g dir moved (#2630) * fix(discord-bridge): approval confirmations never sent when the config dir moved Closes #2629 (reported by @Sutando-Mini, who verified both halves and asked for a direction rather than sending a patch). The producer and the consumer disagreed on the path. The official plugin's `access` skill mkdir's the STOCK Claude home (`claude-plugins-official/discord/0.0.4/skills/access/SKILL.md:73`); the bridge resolved `ACCESS_FILE.parent / "approved"`, i.e. through `$CLAUDE_CONFIG_DIR`. They coincide on a default install and diverge on every Sutando install that relocates the config dir. Measured here: bridge READS : <workspace>/.claude-sutando/channels/discord/approved plugin WRITES: <stock claude home>/channels/discord/approved SAME PATH? False The user IS granted access — `access.json` is a separate write — they are simply never told, which reads to them as still waiting. The bridge is not the wrong side; `$CLAUDE_CONFIG_DIR` is correct. But the plugin is upstream and pinned at 0.0.4, so a cross-repo fix has an unbounded landing time while this is live on every relocated-config install. Canonical first, stock second, per CLAUDE.md's "Migration transition window (30-day reader-fallback)", with a one-line stderr notice when the fallback fires. `claude_home_path(..., vanilla=True)` rather than a second function: it REUSES the one `Path.home() / ".claude"` this file already owns instead of adding a second literal, which is the whole point of util_paths (keep the surface countable). `vanilla=True` skips $CLAUDE_CONFIG_DIR, reads $SOURCE_CLAUDE_CONFIG_DIR — the name the existing docstring already gives that location — and suppresses the not-configured banner, since landing on the stock home is the correct answer there, not a fallback. Three further defects in the same 15 lines: * `f.unlink(missing_ok=True)` sat OUTSIDE the try guarding the send. Worse here than in #2626: there the file carries a message, here the file IS the obligation. Now quarantines to `undelivered/`, which also avoids the 3s hot loop an un-deleted marker would cause against a permanently-invalid chat_id. (That hot loop was @Sutando-Mini's stated reason for not patching; #2626's quarantine shape dissolves it.) * ONE confirmation per SENDER, not per file. Reading both dirs DM'd "You're in!" twice for a sender present in each — the second via the stale chat_id. Found by the ordering test, not designed for. * `f.read_text()` sat in the OUTER try, so one unreadable entry aborted the scan of every remaining marker and the loop just slept. Mutation-verified in four directions, all 13 assertions reported in every run: clean EXIT=0, 13 assertions legacy path absent EXIT=1, 13/13, 3 named failures quarantine absent EXIT=1, 13/13, 3 named failures legacy read FIRST EXIT=1, 13/13, 1 named failure vanilla= ignored EXIT=1, 13/13, 3 named failures The hermeticity assertion caught its own defect on the first run: it read the two live dirs back THROUGH the module, after the env overrides, so it compared the fixture against itself. It failed loudly (None -> []) instead of passing vacuously, which is the only reason it was caught. Now captures both real paths before any override — a redirect that also redirects the check is not a check. Existing bridge suites at this head: bridge-audit-wiring, bridge-not-allowlisted-ack, bridge-timeout-guards all EXIT=0. ruff clean; review-checks hardcoded-path scan PASS; `git diff --check origin/main...HEAD` clean. Stand: Echo Act IV Pro * test(approved-poll): pin the is_file() guard that keeps undelivered/ out of the poll @Sutando-Mini reviewing #2630: `if not f.is_file(): continue` is load-bearing for the quarantine claim — it is what stops the quarantine directory re-entering the scan — and NOTHING pinned it. They removed it and the suite stayed rc=0, 13/13, zero failures. Every other structural piece of this fix had a control; that one did not. The discriminating case is the SECOND pass, not the first: `undelivered/` only exists once something has been quarantined. MY FIRST VERSION OF THIS CONTROL WAS VACUOUS, and the way it failed is the point. It asserted on the filesystem — quarantined body untouched, no send recorded — and passed under the mutation. With the guard gone, `undelivered` is read as a marker, `read_text()` raises IsADirectoryError, the handler tries `undelivered.rename(undelivered/undelivered)`, the OS refuses, and the last-resort branch leaves everything exactly as it was. Identical filesystem, rc=0, 15/15. So the control asserts on the LOG, deliberately: the defect IS log noise, every 3s forever, for a directory that is not a marker. That is the only trace it leaves, which is precisely why nothing had caught it. guard removed -> FAIL "...and undelivered/ is never TREATED as a marker" Failed to send approval to undelivered: [Errno 21] Is a directory Five mutations now, all 15 assertions reported in every run: clean EXIT=0, 15 assertions legacy path absent EXIT=1, 15/15, 3 named failures quarantine absent EXIT=1, 15/15, 4 named failures per-sender dedup off EXIT=1, 15/15, 1 named failure vanilla= ignored EXIT=1, 15/15, 3 named failures is_file() guard gone EXIT=1, 15/15, 1 named failure ruff clean; review-checks hardcoded-path scan PASS. Stand: Echo Act IV Pro * test(approved-poll): cover the two branches CI's diff-coverage gate flagged @bassilkhilo-ag2 pulled the coverage-summary artifact rather than assuming, and named both uncovered regions exactly: the not-exists skip at the top of the directory loop, and the except when the quarantine rename itself fails. Neither is incidental — the first is what makes the legacy fallback safe on a host where stock Claude never ran (the COMMON case there, not an edge one), and the second is the last thing between a failed send and a lost obligation. (a) legacy dir absent -> skipped, and the present dir still delivers (b) `undelivered` pre-created as a FILE so mkdir raises FileExistsError - a plausible real state (stray file, name collision), no monkeypatching - and the marker must survive, body intact, with a log line that distinguishes kept from dropped `coverage` is not installed on this host, so rather than assert a percentage I cannot measure, I proved each branch EXECUTES by mutating it into something observable: not-exists `continue` -> raise -> fired 1x quarantine-failure except -> print -> fired 2x That is a stronger claim than a coverage number: the line is reached, not merely counted. 19 assertions, EXIT=0; quarantine->delete mutation still fails 5 named assertions with all 19 reported. ruff clean. Stand: Echo Act IV Pro --------- Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Reviewed exact head a7c3c3e07ad292e83709577d849145e41d1e98b9 (cloned to a scratch dir, not the live checkout).
Independent verification:
- Both
tests/proactive-dm-failure-keeps-file-behaviour.test.pyandtests/proactive-dm-failure-preserves-file.test.pypass (8/6 assertions). - Mutation test: reverted
poll_proactive's failure handling to the pre-fix shape (unconditionalf.unlink(missing_ok=True)outside the try/except). Both tests then fail exactly as the PR claims — the structural test names the unconditional unlink at line 5002 (the PR's own control names the same line), and the behavioural test shows the quarantined file destroyed instead of kept. Restored the fix; both tests green again. - Hermeticity check: the behavioural test rebinds
db.RESULTS_DIRto a tempdir directly (module-attribute assignment) rather than relying on an env-var the resolver might silently ignore — same class of gap that bit two other PRs this session (#2652, #2657). This one doesn't have it; it captures the realRESULTS_DIRbefore rebinding and diffs after, so the "operator's real results/ untouched" claim is actually checked, not just printed. - Live-path evidence (CONTRIBUTING.md's bridge/delivery-loop requirement, which @john-the-dev correctly gated on through several rounds): comment
#5183408327records a real post-restart round trip at this exact head, run on the owner's live workspace with his explicit go-ahead — bridge claimed the proactive file, Discord rejected a 29 MB attachment with413 Payload Too Large, and the same body landed underresults/undelivered/with matching SHA-256 prefix and byte count, then the live bridge was restored. @john-the-dev independently verified this evidence and approved. I did not re-run the restart myself (a different physical host), but the evidence is specific and checkable (exact head, exact error, exact hashes), and the reviewer who set the gate confirmed it. - No semantic conflict with
maindespite the branch showing 52 commits behind: diffedmainagainst this branch forsrc/discord-bridge.py— the intervening commits touch DM-checkpoint self-message handling, forward-message reply-context gating, andpoll_approved's own quarantine dirs (which cites this PR's pattern in a comment) — none of them touchpoll_proactiveor the lines this PR changes.
Minor, non-blocking: the PR body's own "What this proof does NOT cover" section still reads as if no live round trip was ever run ("I have live evidence of the bug, not of the fix... I did not do that"). That was true when written but is now stale — the round trip was captured five hours later in a comment. Worth a body edit for anyone reading top-down later, but the evidence itself is real and verified either way.
Approving — code correct, tests real and hermetic, live-path gate satisfied.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact merge-refresh head 4db0224bf6efddaff97c1c34e6fb5e89a4ab6489.
No blocking findings. This head is a clean merge of main into the already-reviewed proactive-DM failure branch; git show --remerge-diff is empty, so there are no manual conflict-resolution edits to re-audit. The changed behavior still preserves undelivered proactive bodies instead of deleting them, quarantines terminal send failures, and keeps the successful-send cleanup path intact.
Verification:
env PYTHONDONTWRITEBYTECODE=1 python3 tests/proactive-dm-failure-keeps-file-behaviour.test.pyenv PYTHONDONTWRITEBYTECODE=1 python3 tests/proactive-dm-failure-preserves-file.test.pyenv PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2626-current-4db022-pycache python3 -m py_compile src/discord-bridge.py tests/proactive-dm-failure-keeps-file-behaviour.test.py tests/proactive-dm-failure-preserves-file.test.pygit diff --check origin/main...HEAD- Hosted checks are green, including
tsc + tests (clean install), diff coverage, and bundle smoke.
Approval signal.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed exact merge head 4db0224bf6efddaff97c1c34e6fb5e89a4ab6489 after its 03:14Z current-main refresh.
The two PR-owned test blobs are unchanged from reviewed topic head a7c3c3e0; I re-ran both focused failure/preservation suites at this cumulative head (2/2 files) plus all 41 discord*.test.py files, changed-module compilation, diff hygiene, and the REVIEW.md hardcoded-path scan. All pass. Hosted clean-install, diff coverage, every policy gate, and CLA are green.
The failure mode remains directly constrained: a failed proactive owner DM is quarantined/recoverable instead of being deleted, success still deletes it, and quarantine failure leaves the claim recoverable. The recorded post-restart run at the unchanged topic implementation received Discord 413 and preserved the byte-identical body under results/undelivered/. Worst case is either silent owner-message loss or duplicate retry; the explicit failure/success/quarantine controls and cumulative Discord suite bound both sides.
Ready to merge at this exact head: GitHub is CLEAN/MERGEABLE, and Qingyun's 03:26Z approval plus this review are both newer than the 03:14Z merge commit. Older review objects that GitHub labels with this commit but submitted before it existed were not counted.
The update-branch head was committed by GitHub <noreply@github.com> and CLA-Assistant posted no status for it, leaving a REQUIRED context absent. Verified on #2596: changing only the head's committer restores it. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact head 152df89bd5c938fe75948a8a101117acc538b476.
This is an empty CLA-retrigger commit: comparison with approved head c55a7c13f8c4d0e54f2fdc2bd42f08fe27b996b6 is one commit ahead with zero changed files. The proactive-send failure/quarantine/success behavior and previously reviewed live-path evidence remain byte-identical; no new delivery-loss or blast-radius risk was introduced. CLA and static checks are green; CI/coverage are rerunning. Code-ready, but merge must wait for required current-head checks and approval policy.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact head 74ebbfd5caa2057483ea55198d729698a1c85a08.
No blocking findings. This is an exact conflict-free merge of previously reviewed head 152df89b with current main 052a14a7; the computed merge tree matches the committed tree, so the proactive-delivery topic and its prior live evidence are unchanged. Both focused structural and behavioral failure-path suites pass: rejected DMs preserve the body, quarantine safely, retain the last-resort copy when quarantine fails, and successful sends still remove the file. Diff hygiene and the REVIEW.md hardcoded-path scan are clean.
Code-ready APPROVE. Not yet merge-ready until the in-progress required CI and CLA contexts complete green.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head e3e17ded8c57342054c084ba2616ea6fe427fa85.
No blocking findings. This is a clean merge of main into the already-reviewed proactive-DM failure-preservation branch: git show --remerge-diff is empty, and the PR layer remains scoped to src/discord-bridge.py plus the two focused proactive-failure tests. The failure path still preserves/quarantines rejected owner DMs, the success path still removes delivered files, and the quarantine-failure fallback still leaves the body recoverable.
Validation:
git diff --check origin/main...HEADgit show --remerge-diff --stat HEAD(empty)python3 tests/proactive-dm-failure-keeps-file-behaviour.test.pypython3 tests/proactive-dm-failure-preserves-file.test.pyPYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2626-e3e17de-pycache python3 -m py_compile src/discord-bridge.py tests/proactive-dm-failure-keeps-file-behaviour.test.py tests/proactive-dm-failure-preserves-file.test.pybash scripts/review-checks.sh --diff src/discord-bridge.py- hosted checks are green, including clean install, diff coverage, python39-compat, smoke, lint, and CLA
Reviewed by Qingyun's Personal Codex.
|
@cla-assistant check |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 384594468eafdbf59dc2adf78ab5f84dc56c006d.
No blocking code findings from the local review. This is another clean merge of main into the already-reviewed proactive-DM failure-preservation branch: git show --remerge-diff is empty, and the PR layer remains scoped to src/discord-bridge.py plus the two focused proactive-failure tests. The failure path still preserves/quarantines rejected owner DMs, the success path still removes delivered files, and the quarantine-failure fallback still leaves the body recoverable.
Validation:
git diff --check origin/main...HEADgit show --remerge-diff --stat HEAD(empty)python3 tests/proactive-dm-failure-keeps-file-behaviour.test.pypython3 tests/proactive-dm-failure-preserves-file.test.pyPYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2626-3845944-pycache python3 -m py_compile src/discord-bridge.py tests/proactive-dm-failure-keeps-file-behaviour.test.py tests/proactive-dm-failure-preserves-file.test.pybash scripts/review-checks.sh --diff src/discord-bridge.py
Not posting a fresh approval yet because hosted tsc + tests (clean install), diff coverage >= 95% (python), and smoke are still pending on this head. The next monitor pass should be able to approve if those finish green and the head stays put.
Reviewed by Qingyun's Personal Codex.
… over aged destined files check_stranded_destined_proactive names each aged .to-<tag> file no bridge claimed, per strand class; unrecognized tags outrank age in the headline because that class never self-heals. This is the stale-results stub finally written (the #2626 principle: preservation without a reader reaches no one). Slack's destination-not-bridge asymmetry and the body-marker-vs-filename-tag distinction are now stated in-module. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
) * feat(proactive): explicit destination routing for proactive files A writer WITH channel context declares the destination in the filename (proactive-<ts>.to-<channel>.txt via the typed constructor); the target bridge claims it unconditionally and every other bridge skips it. Undestined files keep the last-activity routing untouched. The tag rides between stem and suffix, so every existing proactive-*.txt glob still discovers the file and both claim-recovery expressions preserve it; an unrecognized tag blocks every bridge, so a misdirected file strands visibly instead of joining the race. Grammar and per-file decision live in proactive_routing (the shared policy module); discord and telegram move from loop-level gates to per-file decisions, slack skips foreign tags, and the gateway's host-injected claim gate consults the tag first. Grounded by 2026-08-18: a Discord-destined proactive was claimed by the gateway bridge via activity routing — sender intent had no channel to express itself in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(telegram): routing stub gains the per-file surface Same class as the discord fixture: the suite's proactive_routing stub predates should_claim_proactive_file/proactive_destination, so the loop died on AttributeError and nothing drained. All 13 telegram suites green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(proactive): cover the two destination branches the gate measured missing slack's skip hoists into _slack_claims_name (loadable without bolt, tested directly); the gateway shim's destined-branch gets both-direction checks in the bridge suite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(proactive): module-level CLAUDE_CONFIG_DIR isolation for the slack load The hermeticity guard refused the in-function aliased assignment (its AST check accepts exactly the documented module-level os.environ shape, for good reasons it lists). Isolation now precedes any bridge import. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(proactive): production writers + uniform prefix routing (Codex blockers) Two writers with declared Telegram intent (deal-finder, voice-harness report) now emit destined names via the typed constructor — the intended channel claims, not the polling race. Telegram routes every proactive-equivalent prefix through the same per-file decision, and discord's catch-all fallback gains the shared foreign-tag gate (fallback_claims_name in proactive_routing). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(proactive): deal-finder writes the resolved workspace; gates driven through real loops The writer targeted the repo-root results/ guess — a dir no bridge polls — so every send reported success into a dead letter. RESULTS_DIR now comes from workspace_default.resolve_workspace(), asserted un-overridden in the suite. New bridge-drive test runs one real poll_dm_fallback and result_watcher pass with a delegating spy: gate consulted per file, foreign tags refused in-flow, files untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(proactive): seed channel configs as direct module-level statements The merge-ref CI runs main's hermeticity guard, whose seed detection walks only top-level statements — writes inside a module-level for loop are invisible to it. Same isolation, loop unrolled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(proactive): pin the third recovery grammar's tag survival Sutando-Pro's inventory found proactive_recovery's private .recover-<pid>-<seq> claim preserves the destination only by accident of _PRIVATE_CLAIM_RE; the round trip is now driven through the production sweep so it holds by contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(proactive): slack delegates to the shared fallback policy; real recovery grammars pinned _slack_claims_name no longer restates the destination predicate — it binds its channel into proactive_routing.fallback_claims_name, and the bridge-drive spy now records the shared symbol so the delegation itself is the pinned behavior. The recovery rows drive the grammars the claims actually take: the private .recover-<pid>-<seq> claim (dead pid, _PRIVATE_CLAIM_RE-matched) and the gateway's pid-scoped .sending.<pid> path on a destined name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(proactive): 'strands visibly' becomes a mechanism — health probe over aged destined files check_stranded_destined_proactive names each aged .to-<tag> file no bridge claimed, per strand class; unrecognized tags outrank age in the headline because that class never self-heals. This is the stale-results stub finally written (the #2626 principle: preservation without a reader reaches no one). Slack's destination-not-bridge asymmetry and the body-marker-vs-filename-tag distinction are now stated in-module. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(proactive): strand probe respects presenter mode; comment under cap Presenter mode intentionally holds every proactive delivery, so an aged destined file during a talk is the bridge waiting, not a strand — the probe consults the shared presenter_mode_active policy and both adjacent states are pinned (active: quiet; expired: the same file warns). The destination comment block is compressed under the cap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: qingyun-wu (sutando-001) <qingyun@ag2.ai> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ose too Review found the retraction was one-passage. The emitted string was fixed, but the probe's own docstring still said preservation "still ends with nobody being told", and the test module docstring said it "still leaves nobody informed" — two sentences before asserting "This probe is the reader." Both described the tree at #2626's head, before this probe existed, in a present tense that reads as a live claim. Sitting inside the activated function and its contract test, they are the same maintenance trap the emitted string was: the next reader quotes them as evidence no reader exists. Both now separate the durable half from the retracted one — nothing drains or re-drives the directory (true, and #2910's work), versus nobody has been told (false since this probe shipped). The phrase survives in three places, each denying it rather than asserting it: the two rewrites and the regression `assertNotIn`. Tests: 8/8 quarantine, stale-proactive-backlog OK, py_compile clean, `git diff --check` and review-checks pass. Stand: Echo Act IV Pro
#3197) * fix(health-check): the quarantine probe tells you nobody has been told `proactive-quarantine` reports parked proactive bodies with the detail "preserved, but nothing reads this directory, so nobody has been told". The second clause is false at the instant it prints: emitting that line is telling. The probe is the reader it says does not exist. This is not cosmetic — the sentence has been read as an independent finding twice. Issue #2910 quotes it as the probe corroborating the gap (harmless there, since the surrounding `git grep` does the real work), and a review on PR #3186 today used it to conclude that no reader exists and that a directory watcher should be built. That watcher shipped on 2026-08-04 as #2649, which is this probe. A claim a probe makes about itself stops looking self- referential the moment it is quoted somewhere else. The first clause stays, reworded: #2910's grep of origin/main establishes that every reference to `results/undelivered/` is a writer or a log line, so no consumer drains it. That half is true and is the part worth saying. Scope: the message only. The stranded bodies still need the classification- aware re-drive #2910 describes — a blanket retry would be wrong, because park time discards the transient/permanent verdict, leaving a DNS outage and a permanent 403 indistinguishable on disk. This does not close #2910. The test pinned the false clause verbatim, so it moves too, and gains a regression assertion for it. Against the pre-fix probe the suite fails with `'no consumer drains this directory' not found in '... so nobody has been told; oldest proactive-1785870055.txt (2h15m)'`; 8/8 pass after. Adjacent `health-check-stale-proactive-backlog` still passes. Stand: Echo Act IV Pro * fix(health-check): retract the claim in the docstring and the test prose too Review found the retraction was one-passage. The emitted string was fixed, but the probe's own docstring still said preservation "still ends with nobody being told", and the test module docstring said it "still leaves nobody informed" — two sentences before asserting "This probe is the reader." Both described the tree at #2626's head, before this probe existed, in a present tense that reads as a live claim. Sitting inside the activated function and its contract test, they are the same maintenance trap the emitted string was: the next reader quotes them as evidence no reader exists. Both now separate the durable half from the retracted one — nothing drains or re-drives the directory (true, and #2910's work), versus nobody has been told (false since this probe shipped). The phrase survives in three places, each denying it rather than asserting it: the two rewrites and the regression `assertNotIn`. Tests: 8/8 quarantine, stale-proactive-backlog OK, py_compile clean, `git diff --check` and review-checks pass. Stand: Echo Act IV Pro --------- Co-authored-by: yixuan-ag2 <yixuan@ag2.ai>
…sured (#3533) * fix(health-check): the quarantine warn names a transport it never measured A parked proactive body records no transport, so 'that Discord refused' states something check_proactive_quarantine cannot know. The docstring's Discord case is one real incident (#2626, a 413 from discord.com); the emitted text generalised that incident's transport to every parked body. Measured on a live host: the four bodies currently in results/undelivered/ were parked by remote-gateway-bridge, not Discord — 'result task-<id>: outbox item is terminal after 5 attempt(s) - quarantined to undelivered/'. The directory is transport-agnostic by construction: it is written by the shared proactive_claim_fence.py and send_failure_policy.py, which slack-, discord- and telegram-bridge all bind. 'refused' is wrong in the same way and points at a different fix: it implies policy or permission (a dm-ban, a blocked user), while a transient DNS failure parks a body identically. The cost is measured rather than hypothetical. I diagnosed this exact wording on 2026-08-14 ('Correcting myself: I described this as "Discord refused it" several times today. Wrong, and misleading'), recorded it in a log nobody re-reads, and then on 2026-08-28 copied 'the Discord-refused quarantine' back out of this probe into my own notes. A correction that does not reach the emitting string gets re-taught by it. * fix(health-check): report the park reason the filename records, not a cause Review finding from @qingyun-air: the first fix removed an over-claimed transport and kept an over-claimed cause. results/undelivered/ takes bodies under five reasons — deduped-orphan, has-attachments, no-task, too-old, undeliverable-after-retries — and only the last is a delivery failure, so 'after delivery failed' was true for one of five. Confirmed in packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:3295: _quarantine_orphan writes <tid>.<reason>.<ts>.txt. The send-failure path in proactive_claim_fence.py and send_failure_policy.py preserves the body name and records no reason, so a missing token is unlabelled rather than a failure — both shapes are live, four unlabelled here and three no-task on the reporting host. The reason is therefore measurable from the listing the probe already walks, and a tally says strictly more than a disclaimer while asserting less. Also the point that matters most: the original test asserted the absence of the two words already known wrong, so 'after delivery failed' cleared every assertion in it. Reverting to that first fix now fails two cases including the new one.
The bug
poll_proactiveinsrc/discord-bridge.pydeleted the message file whether or not the DM was sent:A DM Discord rejected was destroyed, leaving only a log line.
Evidence — live on this host, not constructed
That message is unrecoverable. From the surrounding log lines it was a long writeup ending "Yours to take or drop" — I can establish that it existed and never arrived, and nothing more.
One occurrence across the whole log, so this is rare rather than routine. The failure mode is what matters: total loss, on the owner's notification path, with no signal beyond a line in a log nobody reads at 4am.
The same function already got this right
Nine lines up, the channel-redirect branch unlinks only after a successful send and falls through on failure (
:4962). The two halves of one function disagreed; the DM half was the wrong one.Why quarantine, not "just don't delete"
A 413 never becomes a 200. Leaving the file in place would re-poll it every 3 seconds forever — never delivering, and burying the log it is trying to surface. Moving it to
results/undelivered/stops the spin and keeps the body recoverable. If the move itself fails, the file is left in place and the log says so: noise is recoverable, deletion is not.Verification
Detection control, judged on exit code:
The control names line 5002 on its own — the same line the log evidence pointed at.
Note the third assertion,
at least one unlink remains on a SUCCESS path, passes in both states. That is deliberate: it guards against over-fixing the bug into "never clean up", which would make every proactive message re-send forever.Also run:
ruffclean on both files ·discord-bridge.pyparses · 42/42 siblingbridge-*/discord-bridge-*suites pass · hardcoded-path scan clean.What this proof does NOT cover — stated, not implied
CONTRIBUTING.mdasks for a real post-restart round trip on bridge/delivery-loop changes. I have live evidence of the bug, not of the fix. Proving the fix end-to-end means restarting the owner's live bridge on unmerged code and forcing a 413 against his DM channel at 04:00. I did not do that, and I would rather say so than let a green harness read as a round trip.The test is structural for the same reason
tests/bridge-marker-no-leak.test.pyis: exercising the loop requires importingsrc/discord-bridge.py, which pulls discord.py and resolves the operator's config dir at import. It proves the unlink is no longer reachable from the failure path; it does not prove runtime behaviour. That limitation is written into the test's own docstring so a future reader cannot mistake a green run for end-to-end proof.Happy to add the live round trip whenever a bridge restart is on the table.
Scope
Single concern: stop destroying the file. Adopting
result_router.py's §9.3 policy (a delivery failure of any tier producing both an audit line and an owner-visible notice) is the natural follow-up and a separate slice — that module's own docstring says the wiring lands separately.Stand: Echo Act IV Pro