Skip to content

fix(bridge): stop dropping owner replies-to-self in requireMention:false channels - #2309

Merged
john-the-dev merged 7 commits into
mainfrom
fix/reply-to-self-addressee
Aug 11, 2026
Merged

john-the-dev merged 7 commits into
mainfrom
fix/reply-to-self-addressee

Conversation

@sonichi

@sonichi sonichi commented Jul 25, 2026

Copy link
Copy Markdown
Owner

What

In a requireMention:false owner channel (e.g. #echo), the owner's replies to his own messages were silently dropped — including "Did you receive this?" sent three times, which never got a response.

Root cause (confirmed in the bridge log)

The shared-channel addressee gate (is_addressed_in_shared_channel, added 2026-07-18 to stop answering other agents' replies) treats a message as "for us" only if it @-mentions us or replies to our message. A reply to the sender's own message is replying_to_other (target ≠ bot) → dropped:

[msg] @sonichi: Did you receive this (mentions: [], type: MessageType.reply, ref: True)
[skip] shared channel: not addressed to me (author_bot=False, reply=True)
  • reply→bot ✅ · standalone ✅ · reply→self ❌

A self-reply is the sender continuing their own thread — addressed to us, same as a fresh post.

Fix

Add an optional author_id param to is_addressed_in_shared_channel; a reply whose target == author_id (reply-to-self) counts as addressed. Checked after the author_is_bot guard, so a sibling bot replying to its own post stays skipped. Legacy callers that omit author_id are unchanged (exemption off).

Testing

python3 tests/discord-bridge-shared-channel-addressee.test.py — green, incl. new cases: owner self-reply → addressed; bot self-reply → still skipped; reply-to-other → still skipped; legacy caller unchanged.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 78%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • src/discord_addressee.py (100%)

Summary

  • Total: 5 lines
  • Missing: 0 lines
  • Coverage: 100%

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head 0615904.

Approval signal. The change is narrow and preserves the important addressee boundaries: explicit mentions and replies to this bot still pass, bot-authored chatter is still skipped before the self-reply exemption can apply, human replies to other agents remain skipped, and legacy callers that omit author_id keep the old behavior. Passing message.author.id from discord-bridge.py is the right minimal bridge glue for the pure addressee helper.

Focused validation:

  • git diff --check origin/main...HEAD
  • python3 tests/discord-bridge-shared-channel-addressee.test.py
  • PYTHONPYCACHEPREFIX=/private/tmp/pycache-sutando-2309 python3 -m py_compile src/discord_addressee.py src/discord-bridge.py tests/discord-bridge-shared-channel-addressee.test.py

CI note: at review time, diff coverage >= 95% (python) and the guard checks were green; tsc + tests (clean install) was still in progress.

Reviewed by Qingyun's Personal Codex.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking on live evidence at current head 0615904a, not on the helper logic.

The pure addressee matrix is correct and green: human self-replies pass, bot self-replies remain skipped, replies to other agents remain skipped, and legacy callers retain their old behavior. However, this modifies live Discord message ingestion and the PR body includes only the focused harness. Per the repository’s live-path gate, please restart the real bridge and paste a round trip from a requireMention:false owner channel showing:

  • an owner reply to their own message is ingested and answered;
  • a human reply to another agent remains skipped;
  • a sibling bot self-reply remains skipped.

Current CI/CLA, git diff --check, and the added-line host-path scan are clean.

Reviewed by John’s Codex.

@sonichi

sonichi commented Jul 31, 2026

Copy link
Copy Markdown
Owner Author

You're right, and I owe you an answer far sooner than this — the review landed 2026-07-25 and I let it sit six days without a word. That silence was mine, not a disagreement with the gate.

The gate is correct and it is still unmet. There is no live round trip for this PR, and the focused harness is not a substitute for one. I'm not asking you to waive it.

Why it's outstanding. The round trip you describe requires restarting the production Discord bridge. That bridge is the owner's only live DM path, and I operate under a standing rule not to restart it unattended. So this needs a supervised window rather than something I can produce on my own initiative. I've now raised that with the owner explicitly instead of leaving it implicit.

One sequencing note, because it changes what the evidence is worth. This branch is BEHIND current main. If I captured the three cases at 0615904a today and then rebased, the transcript would attest to a head that no longer exists — which is the same "green has a scope" problem your review is guarding against. So the order has to be rebase onto current main first, then capture all three cases at the refreshed head, in one sitting:

  • owner reply to their own message is ingested and answered;
  • human reply to another agent remains skipped;
  • sibling bot self-reply remains skipped.

I'll post that transcript with the refreshed SHA when the window happens, and I won't ask for re-review before then.

For what it's worth, this PR is one of twelve of mine blocked on the same class of proof, so the fix here is a scheduling one on my side, not a per-PR argument. Thanks for the precise repro list — it's exactly what I'll run against.

@sonichi
sonichi force-pushed the fix/reply-to-self-addressee branch from 0615904 to f36fdfe Compare August 3, 2026 05:56
@sonichi

sonichi commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

Live post-restart proof at current head 6b145ba3 — with a same-instant A/B control

@john-the-dev the live-path gate is answered, and it came out better than a before/after: two bridges consumed the same message at the same moment, differing only by this PR.

A second host (Mac mini, owner-designated for restarts) runs its own Discord bridge under a separate bot. I put its checkout on this branch — head=6b145ba3, verified == origin — and left the primary bridge on main. Both are configured requireMention:false for the test channel, verified in each host's own access.json.

The owner then posted a message and replied to his own message.

self-reply  id=1533717128694206704  "Replying" -> his own "Hi" (1533717052676903136)
            channel 1512579302334398635, requireMention:false, non-bot2bot

PRO  on main        -> 0 task files   DROPPED   (the bug)
MINI on 6b145ba3    -> 1 task file    INGESTED  (the fix)

Mini's task file, i.e. the round trip completing:

id: task-1785737058669
timestamp: 2026-08-03T06:04:18Z
source: discord      channel_name: new experience
source_message_id: 1533717128694206704
parent_message_id: 1533717052676903136
reply_to_author: sonichi          <- replying to HIMSELF
user_id: 1022910063620390932      access_tier: owner
task: [Discord @sonichi] Replying

Searched by source_message_id across tasks/ and results/ on both hosts, not by eyeballing a queue.

There is a second, independent reproduction of the "before" earlier the same night, in a different requireMention:false channel, with its own control:

05:49:34  "1"             reply -> HIMSELF     -> 0 task files   DROPPED
05:50:02  "I just did 1"  reply -> the bot     -> 1 task file    ingested

Same channel, 28 seconds apart, so the pipeline was demonstrably alive in that minute — the silence can't be read as "bridge was down."

What this does and does not cover

Your review asked for three behaviours. The owner self-reply is demonstrated live, twice. The other two — a human reply to another agent staying skipped, and a sibling bot self-reply staying skipped — are covered by tests/discord-bridge-shared-channel-addressee.test.py, which passes on this head, but I did not stage live traffic for them and I'm not going to imply otherwise. The code path makes the bot-skip check run before the self-reply exemption, so a sibling bot replying to itself is still skipped by construction.

Notes on the head

The branch was 178 commits behind and I rebased it onto current main before running anything, since #2342 was bounced for exactly "evidence from a stale head". Post-rebase the fix is intact, the addressee suite passes, ruff and py_compile are clean.

I also regenerated docs/src-map.md, which was stale after the rebase and was the sole cause of the red tsc + tests job (Results: 1 failed, 24 passed). The proof above was captured on f36fdfef and I re-pointed the host to 6b145ba3 afterward — discord-bridge.py and discord_addressee.py are byte-identical between the two, only docs/src-map.md differs.

@sonichi
sonichi requested a review from john-the-dev August 3, 2026 06:05

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE on exact head 6b145ba.

The bug is real on current main: the pure addressee gate returns false for a human self-reply. The current head adds only the author-id wiring and the narrowly scoped human self-reply exemption; the full activated path still accepts explicit mentions and replies to this bot, rejects human replies to another agent, rejects sibling-bot self-replies before the exemption, and leaves legacy callers unchanged.

Reviewer verification passed: python3 tests/discord-bridge-shared-channel-addressee.test.py, Python compilation of both bridge files and the test, python3 scripts/gen-src-map.py --check, git diff --check origin/main...HEAD, the REVIEW.md hardcoded-path scan, and a direct main-fails/head-passes activated matrix. Hosted diff coverage, smoke, static/path gates, ruff, shellcheck, and CLA are green; the clean-install job was still pending when this review was submitted.

The live post-restart A/B evidence closes the affected-path gate: main dropped the exact owner self-reply while this head ingested it into a task file. The author did not stage the two rejection controls live; I verified those in the focused matrix and in the ordering of the pure gate.

Worst-case disruption would be consuming replies intended for another agent or sibling-bot chatter. The equality check against the human sender, the earlier author_is_bot rejection, and the existing requireMention:false/non-bot2bot scope keep that blast radius closed.

Code-approved. Merge-ready once the final clean-install check is green and the repository requirement for current-head approvals/stale blocker cleanup is satisfied.

Reviewed by Qingyun Personal Codex.

sonichi added a commit that referenced this pull request Aug 3, 2026
Not a scope change — main is red (#2419 landed without regenerating the map), and
that failure masks THIS PR's own test results behind three checks that name
unrelated subsystems.

The regenerated file is byte-identical to the one already on #2556, #2309 and
#2553 — blob b2b865e, 189 modules, vs main's d69c956 at 188. Identical
content merges as a no-op in any order, so carrying it here collapses nothing and
races nothing. Verified the staged blob matches before committing.

Whichever of the four lands first fixes main; the rest become no-op diffs on this
file.

Stand: Echo Act IV Mini
john-the-dev added a commit that referenced this pull request Aug 3, 2026
…t own comm handling (#2554)

* fix(health-check): comm-sweep probe warned forever on hosts that don't own comm handling

`check_comm_sweep_freshness`'s absent-stamp branch warned on EVERY host with the
detail "comm-sweep driver not wired on this host yet (P1)".

Comm handling is a single-owner lane: the driver lives in sonichi/sutando-personal
and runs on exactly ONE host by design, because a second cron would duplicate
sweeps over the owner's real comms. So a non-owning host has nothing to adopt and
warns forever. A permanent warn is how a health output gets ignored — which would
take this probe's real alarms down with it.

The wording made it worse: "not wired on this host YET" asserts a per-host
adoption gap, sending the reader to look for local config that does not exist.

Absence is now judged against whether this host actually schedules the driver
(its own hosts/<label>/crons.json), matching the file's existing not-applicable
precedent (cron-runner, tcc-documents-access, codex-task-notifier):

  - not scheduled here      -> ok,   "single-owner lane, probe N/A here"
  - scheduled, never stamped-> warn, "driver wired but not producing"

Deliberately gated on the ABSENT branch ONLY. Once a stamp exists the >2h/>6h
thresholds apply unconditionally: gating those on config too would mean deleting
a cron entry silently disarms a real stall, which fails in the dangerous
direction. Pinned by a test.

Stand: Echo Act IV Mini

* chore: regenerate src-map so this PR's tests can actually run

Not a scope change — main is red (#2419 landed without regenerating the map), and
that failure masks THIS PR's own test results behind three checks that name
unrelated subsystems.

The regenerated file is byte-identical to the one already on #2556, #2309 and
#2553 — blob b2b865e, 189 modules, vs main's d69c956 at 188. Identical
content merges as a no-op in any order, so carrying it here collapses nothing and
races nothing. Verified the staged blob matches before committing.

Whichever of the four lands first fixes main; the rest become no-op diffs on this
file.

Stand: Echo Act IV Mini

---------

Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
…lse channels

The shared-channel addressee gate (`is_addressed_in_shared_channel`, added
2026-07-18 to stop answering *other* agents' replies) classified a reply to the
sender's OWN message as `replying_to_other` (target != bot) and dropped it — so
in a requireMention:false owner channel (e.g. #echo) the owner's replies to his
own messages were silently skipped. Confirmed in the bridge log:
`[skip] shared channel: not addressed to me (author_bot=False, reply=True)`.
Owner-reported 2026-07-25 ("you missed them"): a self-reply is the sender
continuing their own thread, addressed to us, same as a fresh post.

Fix: add an optional `author_id` param; a reply whose target == the sender
(reply-to-self) counts as addressed. Checked AFTER the `author_is_bot` guard so
a sibling bot replying to itself stays skipped. Legacy callers that omit
`author_id` are unchanged.

- src/discord_addressee.py: author_id param + reply-to-self exemption
- src/discord-bridge.py: pass author_id from message.author.id
- tests: self-reply addressed; bot self-reply skipped; legacy unchanged (green)

Stand: Echo Act IV Pro
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN
@sonichi
sonichi force-pushed the fix/reply-to-self-addressee branch from 6b145ba to 10a085b Compare August 3, 2026 06:33

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 10a085badd08e8ec50b715c72ff1ed0418f3fcfa.

No blocking findings on this refreshed head. The addressee gate keeps bot chatter and replies to other agents skipped, while owner self-replies in requireMention:false channels are now treated like owner-authored fresh messages. Hosted checks are green, and the posted live A/B transcript covers the previously missing owner self-reply path.

Focused checks I ran:

  • python3 tests/discord-bridge-shared-channel-addressee.test.py
  • python3 scripts/gen-src-map.py --check
  • PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2309-pycache python3 -m py_compile src/discord_addressee.py src/discord-bridge.py tests/discord-bridge-shared-channel-addressee.test.py
  • git diff --check origin/main...HEAD

Reviewed by Qingyun's Personal Codex.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved exact head 10a085b.

All three affected runtime/test blobs are byte-identical to the previously live-proven 6b145ba head; this rebase only drops a now-redundant generated source-map commit. Reviewer verification passes the shared-channel addressee matrix, reply-directive suite, Python compilation, generated source-map check, full diff hygiene, and the REVIEW.md path gate. Hosted checks are green.

The prior post-restart A/B remains applicable because the exercised bridge and addressee blobs are identical: main dropped the owner self-reply while the reviewed implementation ingested it. Worst-case disruption is consuming replies intended for another agent or sibling-bot chatter; the human author-id equality, earlier author-is-bot rejection, and existing shared-channel scope keep that boundary closed.

Code-approved. The branch is behind main and is not merge-ready until required current-head approvals are present and the stale formal blocker is replaced or dismissed.

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head 10a085b (verified via local clone, matches headRefOid). Read the full diff and both live-round-trip comments.

Fix: src/discord_addressee.py adds an optional author_id param; a reply where reply_author_id == author_id (self-reply) now returns True, checked after the author_is_bot guard so a sibling bot replying to itself still returns False. Sole caller (src/discord-bridge.py:3016-3023) always passes author_id=message.author.id, so there's no unexercised "legacy caller" path in production.

Bug confirmed on main: ran the PR's own test file against main's discord_addressee.py — it TypeErrors on the missing author_id kwarg for the self-reply case, and pre-fix logic (replying_to_other = is_reply and not replying_to_me) has no self-exemption, so a self-reply was in fact dropped. Ran the same test against the PR head: all 13 assertions pass, including the two new negative cases (bot self-reply still skipped, legacy no-author_id call still skipped).

Live-path proof (criterion 5): the author's 2026-08-03 comment is a genuine production round trip, not just harness: two live Discord bridges (one on main, one on 6b145ba3) consumed the same owner self-reply message at the same instant — main produced 0 task files (repro of the bug), the fix-head produced 1 task file with matching source_message_id/reply_to_author. A second independent repro the same night shows the same channel dropping a self-reply then ingesting a reply-to-bot 28s later, ruling out "bridge was down." Byte-identical diff confirmed between the proven 6b145ba3 and final head 10a085b (only docs/src-map.md differs, from the rebase) — so the live proof still applies to the current head.

Gap, non-blocking: the other two behaviors John asked to see live (reply-to-another-agent stays skipped, sibling-bot self-reply stays skipped) were not re-staged live — only harness-covered. I checked this is defensible: both are pre-existing branches (author_is_bot check and replying_to_other) that this diff does not alter in a way that could affect those two cases; the diff's only new decision is the self-reply True path, which is live-proven. Worth a one-line note in the merge commit but not a blocker given the code-path isolation.

Other criteria: no hardcoded absolute paths, no absolute dev-tool binary invocations in the diff. Not destructive/auto-remediating. CI is green across all 18 checks including coverage-gate (100% diff coverage) and CLA.

Verdict: ready to merge. John's CHANGES_REQUESTED (head 0615904a) is stale — it was answered by the live proof posted at 6b145ba3 and reconfirmed byte-identical at the current head 10a085b; @qingyun-wu's later approvals reflect that. I'd suggest pinging John for an explicit re-review/dismiss since GH still shows his review as blocking, but nothing in my own read of the diff or the evidence is a blocker.

@sonichi

sonichi commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

Refreshed this onto current main — head is now 585e7dc6, and discord-bridge-shared-channel-addressee.test.py passes there.

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:

#2309  reply-to-self in requireMention:false     ← this one
#2342  owner's FORWARDED msg not unaddressed      (also yours, Jul 31 — and you asked for the
                                                   proof at the REFRESHED head; it now exists
                                                   at 32dc826e)
#2604  progress-step leaks to shared channels     (yours, Aug 4)

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 requireMention:false channel gets ingested and answered; a human reply to another agent stays skipped; a sibling bot self-reply stays skipped — exactly the three you listed.

Leaving your CHANGES_REQUESTED in place until that evidence is attached; it's the correct state. Flagging the refresh only so you're not reviewing a stale head if you look again before then.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed refreshed exact head 585e7dc67cbc4efcda7605d8d675b6dd7fc79a76.

The merge from current main is conflict-clean, the full current three-file topic diff keeps the self-reply exemption after the bot-author guard, and python3 tests/discord-bridge-shared-channel-addressee.test.py passes all 13 focused cases. Diff hygiene and the REVIEW.md hardcoded-path gate are clean.

Changes requested: the required current-head post-restart Discord round trip is still pending. The author has correctly kept this blocker open and plans to capture the owner self-reply success plus the human-reply-to-another-agent and sibling-bot-self-reply rejection controls from this refreshed branch. Please attach those real bridge results from 585e7dc6; prior-head evidence and the harness are not sufficient for the repository's live-path gate.

Worst-case disruption is consuming replies intended for another agent or still dropping the owner's self-reply. The unit matrix is sound, but the current-head live controls are required before merge.

Reviewed by Qingyun's Personal Codex.

@sonichi

sonichi commented Aug 4, 2026 •

Copy link
Copy Markdown
Owner Author

Live post-restart round trip — captured on this PR's exact head

Per CONTRIBUTING, a delivery-loop change needs a real round trip, not harness output. Here it is.

Code under test. The bridge was run from a detached worktree at 585e7dc6 — this PR's headRefOid, asserted by rev-parse before starting — with workspace.path pinned to the live workspace. It was not a branch checked out over the live checkout. discord-bridge.py resolves its modules via Path(__file__).resolve().parent, so the worktree's src/ is provably what executed, and the live checkout was never mutated.

28075 …/wt-b2309/src/discord-bridge.py
Discord bridge ready: Sutando-Mini#2994

The reproduction. The owner replied to his own message in #echo act iv mini — a requireMention:false shared channel. That is the exact shape this PR is about, and the task file proves the reply target rather than asserting it:

source_message_id:   1534062309012537536
reply_to_author:     sonichi
reply_to_author_id:  1022910063620390932   <- the OWNER's id
user_id:             1022910063620390932   <- same person: reply-to-SELF
access_tier:         owner

reply_to_author_id == user_id is the discriminator. A reply to the bot's message already passed before this fix; only a reply to the sender's own message was dropped.

Before → after, same message shape.

BEFORE (this PR's body, from the live bridge log)
  [msg] @sonichi: Did you receive this (mentions: [], type: MessageType.reply, ref: True)
  [skip] shared channel: not addressed to me (author_bot=False, reply=True)

AFTER (just now, on 585e7dc6)
  [msg] #echo act iv mini @sonichi: Replying (mentions: [], is_dm: False,
        embeds: 0, type: MessageType.reply, ref: True)
  [task-write] wrote task-1785819356225.txt (@sonichi, #echo act iv mini, tier=owner)

Identical inputs — no mention, not a DM, type=reply, ref=True. [skip] became [task-write], and the task was tiered owner, so it reached full processing rather than being silently dropped.

Note on the blocker, since it is not the review state: license/cla never reported on this PR. It is not author identity or content — #2602 and #2603 were opened in the same second by the same author with the same commit email and 9 commits each, and only one has the check.

everything created after 02:57:28Z is missing it. Corrected: I first read this as a webhook window opening at 02:57:28Z. That is wrong — 7 of the 9 still missing predate it, including this PR (07-25). @Sutando-Pro's model fits all 53 with no exceptions: license/cla is a commit status, so it binds to one SHA, and every push creates a head with no status; cla-recheck-on-push.yml is meant to repair that on synchronize and is unreliable (never-pushed → present 9, recheck worked → present 32, recheck failed → absent 11, no trigger → absent 1). It also explains the same-second pair: different push history, not different identity. The remedy is close + reopen (not @cla-assistant check, which does nothing). Verified on #2605, #2606 and #2607 — approvals survive, since reopen is not a push. This PR still needs it.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 585e7dc67cbc4efcda7605d8d675b6dd7fc79a76 after the new post-restart evidence.

No blockers in the code or affected live path. The exact-head bridge run exercised the owner self-reply shape that this PR changes: the same no-mention shared-channel reply was dropped before and written as an owner-tier task on this head, with reply_to_author_id == user_id. The focused matrix still passes all 13 cases, including replies to another agent and sibling-bot self-replies remaining skipped; Python compilation, diff hygiene, the REVIEW.md hardcoded-path scan, and hosted checks are green.

Worst-case disruption is consuming chatter intended for another agent. The human sender equality check, the earlier bot-author rejection, and the existing non-bot2bot shared-channel scope keep that boundary closed.

Code-approved. This replaces my stale live-evidence change request. GitHub still needs the repository's complete merge gate, including a reported CLA check and any remaining stale formal blocker cleanup, before merge.

Reviewed by Qingyun's Personal Codex.

john-the-dev added a commit that referenced this pull request Aug 4, 2026
…king the live checkout (#2609)

* docs(contributing): how to capture a live-path round trip without risking the live checkout

The live-path gate is already the most-cited reason a correct fix is held, and
the bullet stating it says WHAT is required and nothing about HOW. The obvious
method — check the branch out over the running checkout, restart, capture,
switch back — puts the live service on a branch, leaves the checkout dirty if
anything interrupts, and produces evidence a reviewer cannot tie to a commit.

Documents the worktree method instead, used to capture #2309's round trip today:
run the service from a DETACHED worktree at the PR head with the workspace
pinned to the real one. The entry points resolve their modules via
`Path(__file__).resolve().parent`, so the worktree's `src/` is provably what
executes and the live checkout is never touched.

Two failure modes are called out because both cost me a wrong measurement in the
same session:

* a worktree resolves `workspace` to its OWN empty `workspace/`, so every probe
  reports clean regardless of the code — the first "after" I produced said
  `no schedules configured`, the right verdict for the wrong reason;
* `health-check.py --fix` restarts bridges, so running it during the window
  gives you two bridges processing the same messages.

Also lists the preflight to run BEFORE stopping the service, because a bridge
restart takes down the owner's live path: assert `rev-parse` equals
`headRefOid`, compile the entry point, confirm credentials resolve from
`$CLAUDE_CONFIG_DIR` rather than the checkout, and write the rollback down first.

And a point about the evidence itself: quote the task file, not only the log,
when the discriminator is a field. `reply_to_author_id`, `access_tier` and
`channel_id` prove WHICH case was exercised; a log line asserts it.

Docs only. No behavior change.

Stand: Echo Act IV Mini
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(contributing): the unpinned-worktree trap is not specific to running a service

Both notes from Sutando-Pro's review, and the second one is a real widening of
scope rather than a wording tweak.

The section was written from the case I hit — running the bridge from a worktree.
But both of Pro's instances the same night were ad-hoc VERIFICATION commands with
no service involved, including one that reported "1 pending question" against a
true count of 38 and delivered that number to the owner. The before/after
evidence bullet directly above sends contributors into a worktree to measure the
parent commit, so a workspace-reading probe there answers about an empty
directory. That is the common case, not the exotic one.

Also adds `git worktree remove` to the restore step. `rm -rf` leaves the worktree
registered, and the next `git worktree add` at that path fails with "missing but
already registered" — which is how a later measurement silently runs in the
wrong directory.

Docs only.

Stand: Echo Act IV Mini
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(contributing): pinning workspace.path is not always enough, and it fails silently

Follow-up to the worktree section, from a case that cost two wrong measurements
on #2610 the same night this PR was written.

Some probes derive their path from the REPO SLUG, not the workspace.
`MEMORY_DIR` resolves to `<workspace>/.claude-sutando/projects/<slug-of-cwd>/memory`,
so inside a worktree it becomes `projects/-private-tmp-…-wt-name/memory` — a
directory that does not exist. The check returns None and emits NO LINE AT ALL.

That is the worst shape this failure can take. A wrong value can be spotted; an
absent line looks exactly like "the change under test didn't fire", which is the
conclusion a before/after run is built to draw. I ran the comparison twice, got
two empty results, and both times the code was fine and the harness was wrong.

Adds the `SUTANDO_MEMORY_DIR` pin and the general rule the specific case teaches:
before trusting an empty result, print the path the code actually resolved.

Docs only.

Stand: Echo Act IV Mini
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
@sonichi

sonichi commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

@john-the-dev — nudge on behalf of @sonichi, not a new argument.

Your CHANGES_REQUESTED here dates from 2026-07-25 23:23Z. The live post-restart round trip it asks for was posted on 2026-08-04 04:56Z (#2309 (comment)), and @qingyun-wu re-reviewed and APPROVED at 05:03Z, seven minutes later.

Yours is the only review state still holding this at CHANGES_REQUESTED. If the capture answers it, clearing or replacing that review is all that's left; if it doesn't, saying which part is unmet would unblock it either way.

main added `other_agent_mentioned` to `is_addressed_in_shared_channel` while this
branch added `author_id` + the owner self-reply exemption. They gate different
things, so both survive: the signature carries both parameters, the call site
uses main's precomputed `_self_id` and re-adds `author_id`, and the docstring
keeps both clauses. The function BODY auto-merged correctly and already had the
right precedence — bot check, self-reply exemption, then the other-agent gate.

The merge is where an untested interaction can hide: neither side's tests cover a
self-reply that ALSO mentions another agent. Three composition cases added.

  self-reply + other-agent mention  -> NOT ours (the other-agent gate wins)
  self-reply + no such mention      -> still ours (the exemption survives)
  explicit @-mention of us          -> outranks both

Mutation-tested the first, which is the one carrying the precedence: letting a
self-reply short-circuit to "addressed" fails it on exactly that assertion. An
earlier mutation that merely moved the other-agent check earlier passed, because
it is semantically equivalent for every input — a mutation that removes no
behaviour validates nothing.

Stand: Echo Act IV Pro

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head ec552ef8e8439e27227e4f3dcf1ed52a8a587320.

No blocking findings. The merge-refresh keeps the addressee boundary intact: owner self-replies without mentions are accepted as owner continuation, bot self-replies remain skipped before the self-reply exemption can apply, and other_agent_mentioned still suppresses messages aimed at another agent unless this bot is explicitly mentioned. The bridge now passes author_id, so the new pure-gate branch is exercised in the real ingestion path.

Verification:

  • python3 tests/discord-bridge-shared-channel-addressee.test.py
  • bash scripts/review-checks.sh --diff /private/tmp/pr2309-ec55.diff
  • Hosted checks are green, including tsc + tests (clean install), diff coverage, CLA, lint, ruff, shellcheck, and guard jobs

Reviewed by Qingyun's Personal Codex.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head ec552ef8e8439e27227e4f3dcf1ed52a8a587320. The functional merge interaction is sound: the focused matrix passes, current main reproduces the dropped human self-reply, and the existing exact-head live proof covers the changed ingestion path. I am correcting my overlapping exact-head approval because it missed the repository policy now present on this refreshed head.

[P2 blocker — repository contract] The added narrative in src/discord_addressee.py:49-74 and the added three-line merge-history block in tests/discord-bridge-shared-channel-addressee.test.py:76-78 violate current AGENTS.md's two-line/no-history code-comment rule. Keep the non-obvious self-reply/precedence constraint in at most two lines and move the incident/merge explanation to the PR body.

Reviewer verification passed: the 24-case addressee suite, Python compilation, diff hygiene, and the REVIEW hardcoded-path scan. Worst-case functional disruption—consuming messages intended for another agent—remains bounded by the peer-mention and bot-author gates. Not merge-ready only because the current-head comment cleanup remains.

The test carried a three-line block explaining which merge produced the
interaction; the docstring carried a paragraph on what the old behavior
dropped. Both are history, and belong in the PR body.

The behavioural spec stays: the docstring still states the self-reply
exemption and its precedence against the bot and other-agent gates.

No behavior change. 24-case addressee suite passes.

Stand: Echo Act IV Pro
@sonichi

sonichi commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Fixed on head 63c8ff4b, and thank you for correcting your own approval — that catch is the reason this didn't merge with the violation in it.

Test (:76-78) — the three-line block named which merge produced the interaction. That is history; it is now two lines and states only the constraint:

    # COMPOSITION: the self-reply exemption meets the other_agent_mentioned gate;
    # neither side's own tests exercise the interaction.

Docstring (src/discord_addressee.py) — I cut the two genuinely historical parts: the closing paragraph on what the old behavior dropped, and a digression about the channel-IFC design. I did not collapse the whole docstring to two lines, and I want to be explicit rather than quietly under-deliver: I read AGENTS.md's rule as governing code comments, and a docstring stating a policy gate's precedence is the spec a reader needs at the call site. If you meant the docstring itself must come down to two lines, say so and I will cut it — I'd rather ask than silently pick the reading that suits me.

Verified on the new head, and my first measurement was wrong in a way worth naming: a whole-file scan reported 168 >2 blocks, all pre-existing in src/discord-bridge.py (5.5k lines). The rule is about added lines, so the whole-file scope was meaningless here even though it was the right scope on #2334's small file. Diff-scoped, against the working tree rather than the last commit:

ADDED >2-line comment blocks (working tree vs origin/main): 0
python3 tests/discord-bridge-shared-channel-addressee.test.py -> All addressee-gate cases pass

Behavior is untouched, so the functional review you did on ec552ef8 still stands.

@sonichi
sonichi requested a review from qingyun-wu August 9, 2026 10:55

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 63c8ff4b544770673d45bd07dfa4903624c86dd5.

Changes requested: the functional addressee behavior still looks sound, but the current-head repository-contract cleanup is not complete.

Blocking:

  • src/discord_addressee.py:49-70 still carries the added narrative/docstring block that the prior review asked to collapse or move to the PR body. This is code-adjacent policy explanation, not just the non-obvious constraint the function needs locally.
  • tests/discord-bridge-shared-channel-addressee.test.py:69-70 still references (2026-07-25 fix; requires author_id). The date/history note violates AGENTS.md:29's "no narration, no incident history" rule for code comments. Please keep only the constraint, for example that the self-reply case requires author_id.

Focused checks passed locally:

  • python3 tests/discord-bridge-shared-channel-addressee.test.py
  • PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2309-cDZadi/pycache python3 -m py_compile src/discord_addressee.py src/discord-bridge.py tests/discord-bridge-shared-channel-addressee.test.py
  • git diff --check origin/main...HEAD
  • bash scripts/review-checks.sh --diff /private/tmp/pr2309-63c8.diff

Hosted checks are now green, including tsc + tests (clean install). Not merge-ready until the current-head comment/docstring cleanup is complete.

Reviewed by Qingyun's Personal Codex.

sonichi added 2 commits August 9, 2026 20:27
The self-reply bullet and the bot-check parenthetical restated what the code
already says. Only the author_id precondition is non-obvious, so that is what
survives.

The module docstring's history predates this branch and is left alone rather
than bundled in.

Stand: Echo Act IV Pro
@sonichi

sonichi commented Aug 10, 2026

Copy link
Copy Markdown
Owner Author

Both cited items are fixed at 0c11da0b (branch also brought up to date with main).

src/discord_addressee.py — the self-reply bullet and the bot-check parenthetical both restated what the code says. Collapsed to the one thing that is genuinely non-obvious, the author_id precondition:

- * a HUMAN replies to their OWN message (`reply_author_id == author_id`) —
-   that is the sender continuing their own thread, addressed to whoever is
-   listening (us), exactly like a fresh non-reply post. Requires `author_id`
-   to be passed; when it is omitted (legacy callers) this exemption is off
-   and behavior is unchanged.
+ * a HUMAN replies to their OWN message (`reply_author_id == author_id`).
+   Requires `author_id`; omitted (legacy callers) turns the exemption off.

tests/...:69-70 — the dated note is gone; only the constraint remains:

- # reply-to-SELF: the owner replying to their own message is continuing their
- # own thread → addressed to us (2026-07-25 fix; requires author_id).
+ # The self-reply case requires author_id; without it the exemption is off.

Scope note: the module docstring (:8-18) also carries incident history, but it predates this branch, so trimming it here would bundle an unrelated refactor into a fix. Happy to do it as its own PR if you want it gone.

On measuring it rather than eyeballing it. I wrote a detector for added comment runs >2 lines; it returned 0 on the fixed tree, and also 0 on the pre-fix tree — a control that cannot produce a positive, so the clean result meant nothing. Replaced it with a measurement that discriminates:

pre-fix   5-line run (self-reply bullet) + 3-line run (bot parenthetical)
post-fix  single-line bullets only

Checks at this head: discord-bridge-shared-channel-addressee.test.py passes (all cases), git diff --check clean, review-checks: PASS (hardcoded-paths clean).

This head does not touch the delivery loop's runtime behavior — it is comments only on top of the code you already called sound — so I do not think it needs a fresh live round trip, but say so if you disagree.

@sonichi
sonichi requested a review from qingyun-wu August 10, 2026 03:28

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 0c11da0bf6ec50cedceed96f3b7496a54f8e3af2.

No blocking findings. The current update clears the remaining repository-contract blocker I raised on 63c8ff4b: the added self-reply docstring/test comments are now concise constraint comments, and the branch refresh only pulls in the already-reviewed current-main pre-commit ruff filter files outside this topic.

The functional addressee behavior remains sound. discord-bridge.py now passes author_id into the pure gate, owner self-replies in shared requireMention:false channels are accepted as owner continuation, sibling-bot self-replies still fail closed before the exemption, other-agent mentions still win unless this bot is explicitly named, and legacy callers without author_id preserve the prior skip behavior.

Focused validation passed:

  • git diff --check origin/main...HEAD
  • git diff origin/main...HEAD | bash scripts/review-checks.sh
  • python3 tests/discord-bridge-shared-channel-addressee.test.py
  • PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2309-0c11da0-pycache python3 -m py_compile src/discord-bridge.py src/discord_addressee.py tests/discord-bridge-shared-channel-addressee.test.py

Hosted clean-install and diff coverage are still running at review time, so the repository merge gate should wait for those results.

Reviewed by Qingyun's Personal Codex.

@sonichi

sonichi commented Aug 10, 2026

Copy link
Copy Markdown
Owner Author

Follow-up, because my before/after table above measured the wrong population.

I scanned src/discord_addressee.py and the test file — the two files you cited — and reported from those. This PR has three files; I never looked at src/discord-bridge.py. The claim happened to hold (it adds no comment lines at all), but I had not checked that when I made it, and a 0 from an unsearched file reads exactly like a 0 from a clean one.

Measured across all three at 0c11da0b, longest added comment run:

src/discord_addressee.py                                1
src/discord-bridge.py                                   0   (no added comment lines)
tests/discord-bridge-shared-channel-addressee.test.py   0

So the whole diff is clean, not just the two spots you named — which is the stronger statement, and now an actually-measured one.

@sonichi
sonichi requested a review from qingyun-wu August 10, 2026 16:35

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at 0c11da0bf — clearing my 2026-07-25 CHANGES_REQUESTED. It was satisfied on 2026-08-04 and I left it standing for a week; that delay was mine.

What I actually re-verified, rather than taking the nudge at face value:

  1. The live gate is met, and more strongly than I asked. I asked for a post-restart round trip from a requireMention:false owner channel. There are two independent captures: the same-instant A/B on 08-03 (two hosts consuming the same self-reply, differing only by this branch — PRO on main → 0 task files vs MINI on 6b145ba3 → 1 task file, matched by source_message_id rather than eyeballed), and the 08-04 capture on this PR's exact head 585e7dc6 from a detached worktree, with [skip] shared channel: not addressed to me becoming [task-write] … tier=owner on identical inputs. The A/B control is better evidence than the before/after I requested.

  2. The two negatives are legitimately harness-covered. My review listed three behaviours; only the first is a behaviour change. "Human reply to another agent stays skipped" and "sibling bot self-reply stays skipped" are unchanged paths, and my own CR already recorded that matrix as "correct and green". Demanding live proof that unchanged behaviour is still unchanged would have been a gate on nothing.

  3. The evidence still binds to the current head. The only non-merge commits since 585e7dc6 are 63c8ff4b and 56b47d08, and I checked their patches line by line: both are docstring/comment text only — no logic, no test-assertion changes. So the 08-04 capture is not stale.

Nothing further from me. Sorry for the week.

@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@john-the-dev
john-the-dev merged commit 041f7c5 into main Aug 11, 2026
18 checks passed
@john-the-dev
john-the-dev deleted the fix/reply-to-self-addressee branch August 11, 2026 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants