Skip to content

feat(discord-bridge): pull sibling-message attachments referenced by a mention - #2126

Open
john-the-dev wants to merge 13 commits into
sonichi:mainfrom
john-the-dev:feat/discord-sibling-attachment-scan
Open

john-the-dev wants to merge 13 commits into
sonichi:mainfrom
john-the-dev:feat/discord-sibling-attachment-scan

Conversation

@john-the-dev

Copy link
Copy Markdown
Collaborator

Problem

When someone pings the bot with text but no media of their own and references another user — e.g. @bot make the video @Alice sent — the media usually lives on that user's own earlier, un-mentioned messages. Those messages neither invoke the bot nor are replied-to, so the primary attachment loop (this message's attachments) and the reply-context loop (a replied-to parent) both miss them. The task is written blind to the source material the owner is pointing at.

Real repro (2026-07-16): a friend posted a garden-app video in a guild channel, then the owner @mentioned the bot in a separate message ("make the video X requested"). The video was silently dropped — only the text arrived. (Public-link assets the friend shared worked fine; it's specifically raw Discord attachments on sibling messages that vanish.)

Fix

When the triggering mention has no own attachments and references a non-bot user, scan recent channel history (last 15 msgs / 15 min window, cap 5 attachments) for attachments posted by any referenced user and pull them in — reusing the existing save + _safe_attachment_basename + _ref_from_attachment + vision-push pattern with a clear provenance note ([File attached (from Alice's recent message): …]).

Gated on referencing a non-bot user so a plain @bot doesn't vacuum unrelated media; bounded by time + count so it can't over-fetch.

Testing

Selection logic is extracted to a pure helper _select_sibling_attachments(history, referenced_ids, cutoff, cap) — kept out of the async I/O so it's unit-testable without a live discord channel. New tests/discord-bridge-sibling-attachments.test.py (9 cases): referenced-user filtering, time-window early-stop, cap, oldest-first ordering, attachmentless/empty handling, multi-user.

$ python3 tests/discord-bridge-sibling-attachments.test.py
  ✓ test_picks_referenced_users_media
  ✓ test_skips_non_referenced_users
  ✓ test_respects_cutoff_early_stop
  ✓ test_cap_limits_total
  ✓ test_oldest_first_ordering
  ✓ test_messages_without_attachments_ignored
  ✓ test_empty_history_returns_empty
  ✓ test_empty_referenced_ids_returns_empty
  ✓ test_multiple_referenced_users
All 9 sibling-attachment selection tests passed.

The async download loop carries # pragma: no cover (block-level), matching the existing primary/reply-context attachment loops — the behavioral logic under test is the pure selector.

🤖 Generated with Claude Code

…a mention

When a mention carries no media of its own but references another user
("@bot make the video @alice sent"), the media usually lives on that user's
own earlier, un-mentioned messages. Those messages neither invoke the bot nor
are replied-to, so the primary attachment loop and the reply-context loop both
miss them — the media is silently dropped.

Add a bounded sibling-attachment scan: when the trigger has no own attachments
and @mentions a non-bot user, pull that user's recent attachments from channel
history (last 15 msgs / 15 min, cap 5), reusing the existing save + sanitize +
vision-push pattern with a clear provenance note.

Selection logic is extracted to a pure, unit-tested helper
(_select_sibling_attachments); the async download I/O stays out of the covered
path, matching the existing attachment-loop convention.

Repro (2026-07-16): a friend posted a garden-app video in a guild channel, then
the owner @mentioned the bot in a *separate* message ("make the video X
requested") — the video never reached the task. Public-link assets worked; raw
Discord attachments on sibling messages did not.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@john-the-dev john-the-dev added the ag2product AG2 product related (packages, PyPI, task-relay) label Jul 16, 2026
@github-actions

github-actions Bot commented Jul 16, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

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

Diff Coverage

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

  • src/discord-bridge.py (100%)

Summary

  • Total: 40 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.

Not LGTM yet.

P2 - Sibling attachments from a single Discord message are emitted in reverse order
src/discord-bridge.py:492-499 appends attachments in the order Discord gives them, then calls picked.reverse() on the flattened list. That makes the helper reverse attachment order within the same message, not just message chronology. On the current head, a single referenced message with attachments ['one.png', 'two.png', 'three.png'] comes back as ['three.png', 'two.png', 'one.png'].

Evidence:

  • python3 /private/tmp/review-pr-2126-latest/tests/discord-bridge-sibling-attachments.test.py passes, so the existing suite covers cross-message ordering but not this intra-message case.
  • Direct repro on the PR helper returned ['three.png', 'two.png', 'one.png'] for one referenced message containing ['one.png', 'two.png', 'three.png'].

Why this matters: for screenshot sequences, multi-part documents, or any workflow where the author expects "first attachment" to mean the first file in their Discord message, the task gets the sibling attachments in the opposite order from the UI. That can scramble the source order the agent sees even though the user supplied a single ordered batch.

Suggested fix: keep message chronology oldest-first without reversing each message's own attachment order. For example, collect per-message batches and reverse only the message batches, or iterate the selected history oldest-first before extending picked. Please add a regression with one referenced message that carries multiple attachments so the sequence stays ['one', 'two', 'three'].

Qingyun's Personal Codex Agent

@liususan091219 liususan091219 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.

Justification: [change-requested: missing_before_after]

Daily triage sweep (gate 2) — conditions fired on this PR:

  • missing_before_after (behavior claim needs a significant BEFORE/AFTER, not just unit tests): Add before/after Discord evidence showing sibling attachments now arrive.

Addressing these (push fixes or reply with evidence) resets the clock; the re-review loop re-judges automatically after new commits.

@liususan091219 liususan091219 added the request-change Has an active CHANGES_REQUESTED review label Jul 16, 2026
@liususan091219
liususan091219 dismissed their stale review July 17, 2026 16:32

Downgrading this change-request to non-blocking per maintainer decision 2026-07-17 (the ask was before/after evidence or a soft policy point, not a correctness blocker). The original review points remain in the thread for the author to consider.

@liususan091219 liususan091219 removed the request-change Has an active CHANGES_REQUESTED review label Jul 17, 2026
@sonichi

sonichi commented Jul 18, 2026

Copy link
Copy Markdown
Owner

Sweep findings (read-only review, 2026-07-18), two items beyond the open P2: (1) the P2 intra-message ordering bug is still live at head — _select_sibling_attachments appends per-attachment then does a flat picked.reverse(), so one referenced message with attachments [a, b, c] comes back [c, b, a]; existing ordering tests only use one attachment per message. (2) The sibling loop calls _transcribe_via_skill(...) synchronously inside the async handler — this reintroduces the event-loop-blocking pattern #2159 removes and would trip #2159's structural guard once both merge. Suggest asyncio.to_thread here to stay compatible.

@sonichi

sonichi commented Jul 19, 2026

Copy link
Copy Markdown
Owner

Codex read-only review — functional core looks right; 2 fixes before merge:

  1. tests/discord-bridge-sibling-attachments.test.py: module-level mutations of HOME, SUTANDO_WORKSPACE, and sys.modules["discord"] are never restored — contaminates same-process tests, order-dependent results.
  2. discord-bridge.py: channel.history(limit=15) can miss the referenced attachment in busy channels even inside the 15-min window — scan to the time cutoff instead of a message-count cap.

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.

The attachment selection logic and its nine focused tests are sound, but the new async download path reintroduces the Discord heartbeat-starvation bug fixed elsewhere.

Comment thread src/discord-bridge.py Outdated
try:
await att.save(s_path)
attachment_refs.append(_ref_from_attachment(att, s_path))
transcript = _transcribe_via_skill(str(s_path))

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.

_transcribe_via_skill is a synchronous subprocess path with a long timeout. Calling it directly inside _handle_discord_message blocks the gateway event loop for every selected sibling attachment (up to five), causing missed heartbeats and reconnect flapping. Offload it with await asyncio.to_thread(...) as in #2159 and add coverage for this new call site.

… event loop

The sibling-attachment scan called `_transcribe_via_skill` — a synchronous
subprocess with a long timeout — directly inside the async message handler, so
transcribing up to five referenced-user attachments blocked the gateway event
loop and caused missed heartbeats / reconnect flapping (CR sonichi#2126, qingyun-wu).

Extract the per-attachment download + transcription into
`_process_sibling_attachment`, which offloads the transcription with
`await asyncio.to_thread(...)` (same pattern as sonichi#2159) and returns the
attachment ref + note. `_transcribe_via_skill` swallows all errors and never
raises, so the extraction is behavior-preserving.

Keeping it a module-level helper (out of the un-coverable I/O handler) makes the
offload unit-testable: added tests asserting the transcription runs OFF the
event-loop thread, the voice/file note branches, download-failure → (None, ""),
image vision-push, and a swallowed vision error. Helper at 100% coverage.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QPBv1rU2s1VrdtZCVd2o8r
@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Fixed in a971b80. Extracted the per-attachment download + transcription into _process_sibling_attachment, which offloads the blocking _transcribe_via_skill with await asyncio.to_thread(...) (same as #2159) — so scanning up to five sibling attachments no longer blocks the gateway event loop or starves heartbeats. _transcribe_via_skill swallows all errors and never raises, so the extraction is behavior-preserving.

Keeping it a module-level helper (out of the un-coverable I/O handler) also made the new call site testable. Added coverage: the transcription runs off the event-loop thread (asserted via threading.current_thread()), the voice/file note branches, download-failure → (None, ""), image vision-push, and a swallowed vision error. The helper is at 100% coverage.

@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.

The asynchronous transcription update fixes the event-loop blocker. The previously reported attachment-order defect remains at src/discord-bridge.py:534: flattening attachments and then reversing the entire list also reverses attachments within each individual Discord message. Preserve message chronology without inverting each message's attachment order, and add a multi-attachment regression.

…ection

_select_sibling_attachments flattened all picked attachments into one list
(newest message first, upload order within each message) and then reversed the
WHOLE list to get oldest-message-first. That also inverted the attachments
within each individual message, so a single message's uploads came out
backwards (CR sonichi#2126, qingyun-wu).

Group attachments per message, then reverse only the group ORDER — each
message's own attachments stay in upload order. The cap still keeps the
nearest-to-the-trigger attachments (scan is newest-first). Also drops a now
-unreachable `room <= 0` guard (the bottom break keeps total < cap on entry).

Adds regressions: a single multi-attachment message preserves upload order; a
two-message multi-attachment history is oldest-message-first with per-message
order intact; and a cap spanning two messages keeps the nearest set in order.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QPBv1rU2s1VrdtZCVd2o8r
@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Fixed in the latest push. You're right — the code flattened all attachments (newest message first, upload order within each) then reversed the whole list, which also inverted attachments within each message.

Now I group attachments per message and reverse only the group order — each message's own attachments stay in upload order, and the cap still keeps the nearest-to-the-trigger set (scan is newest-first). Also dropped a now-unreachable room <= 0 guard (the bottom break keeps total < cap on entry).

Added the multi-attachment regressions: a single multi-attachment message preserves upload order; a two-message history is oldest-message-first with per-message order intact; and a cap spanning two messages keeps the nearest set in order. Changed lines at 100% diff coverage.

@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.

The current head fixes per-message attachment ordering, and all 17 focused selection/offload tests pass. I still can’t approve while the previously reported channel.history(limit=15) truncation at src/discord-bridge.py:2986 remains: it can miss a referenced attachment that is inside the promised 15-minute window in a busy channel. Please address that existing finding rather than adding another ordering change.

…t cap

The sibling-attachment scan pulled `channel.history(limit=15, before=message)`
— the 15 NEWEST messages regardless of time. In a busy channel a referenced
user's attachment that IS inside the promised 15-minute window can sit past the
15th message and be silently dropped (CR sonichi#2126, qingyun-wu).

Fetch by the time window instead: `history(limit=200, after=cutoff,
before=message, oldest_first=False)`. `after=cutoff` bounds the scan to the
15-minute window (no count-based truncation), newest-first so
_select_sibling_attachments keeps working and preserves nearest-to-trigger
selection; the 200 ceiling only guards against a firehose. _select still caps
the kept attachments.

Adds a structural regression pinning the query shape (time-bounded `after=cutoff`,
no bare `limit=15` count fetch) — the scan lives in the un-coverable async
handler.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QPBv1rU2s1VrdtZCVd2o8r
@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Addressed the history-truncation finding in the latest push. You're right — history(limit=15, before=message) fetched the 15 newest messages regardless of time, so in a busy channel a referenced attachment inside the 15-minute window could sit past the 15th message and be dropped.

Now it fetches by the time window: history(limit=200, after=cutoff, before=message, oldest_first=False). after=cutoff bounds the scan to the 15-min window (no count-based truncation), newest-first so _select_sibling_attachments keeps its nearest-to-trigger selection; the 200 ceiling only guards a firehose (and _select still caps kept attachments). Added a structural regression pinning the time-bounded query shape (the scan is in the un-coverable async handler).

@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.

The focused 18-test suite passes, but the attempted time-window fix still imposes a message-count truncation and can miss an attachment that is within the promised 15-minute window.

Comment thread src/discord-bridge.py Outdated
# window (_select_sibling_attachments still caps kept attachments).
history = [
m async for m in message.channel.history(
limit=200, after=cutoff, before=message, oldest_first=False

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.

after=cutoff bounds the history by time, but limit=200 still truncates that window by message count—the same failure mode as the previous limit=15, just at a higher threshold. A busy channel can exceed 200 messages in 15 minutes, so an in-window referenced attachment can still be omitted. Use the time bound without a count limit (Discord.py supports limit=None), or otherwise paginate through the full bounded window; update the regression to exercise >200 in-window messages instead of only asserting the query string changed.

…imit=None)

Round-4 CR sonichi#2126: `limit=200` alongside `after=cutoff` still truncates the
window by message count — the same failure mode as the old `limit=15`, just at
a higher threshold. A busy channel can exceed 200 messages in 15 minutes, so an
in-window referenced attachment can still be dropped.

Use `limit=None` so discord.py paginates the whole time window; the 15-minute
`after=cutoff` bound keeps it finite, and _select_sibling_attachments still caps
the kept attachments. Structural regression now asserts limit=None (no numeric
count cap).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QPBv1rU2s1VrdtZCVd2o8r
@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Fixed — switched to limit=None. You're right that any count ceiling (200 included) re-introduces the truncation. The scan now bounds purely by time (after=cutoff, limit=None), so discord.py paginates the entire 15-minute window; the time bound keeps it finite and _select_sibling_attachments still caps the kept attachments. Structural regression updated to assert limit=None (no numeric cap).

@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.

The prior history-truncation blocker is resolved: the scan is now bounded by the full 15-minute window with limit=None, while attachment selection still caps retained files. The full current diff and existing review threads were rechecked; all 18 focused sibling-selection/offload tests pass, along with Python compilation and git diff --check. No new blocking findings.

@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 against REVIEW.md. Verified independently, not taken from the thread.

Bug exists on main (lesson 1). In the checkout's src/discord-bridge.py, the primary loop iterates only message.attachments (:3038) and the reply-context loop only the replied-to parent (:3090-3109). Nothing reaches a sibling message. Confirmed.

Tests (lesson 3). Ran the head suite (3c18aca) against a scratch tree: 18/18 pass, including test_sibling_history_fetch_is_time_bounded. py_compile clean. Unit-level repro, not a live Discord round trip - the 2026-07-17 dismissal downgraded that ask to non-blocking, so I am not re-raising it.

limit=None termination - the open question from the last round. I read discord.py 2.7.1 abc.py::history. With oldest_first=False + after, it uses _before_strategy with a client-side predicate, and breaks when post-filter count < 100. So the first page containing a pre-cutoff message ends the scan: bounded by the 15-min window, not the channel. The approval's premise holds.

Lesson 6. No /Users/, /home/, /opt/, /private/, ~/.claude, ~/.sutando in added lines.

Lesson 5 - worst case. The block sits at :2967, before access_tier is set at :3053, so it runs for every tier. An other-tier user can cause a third party's media to be saved and vision-pushed into the core's context. Bounded (referenced non-bot users only, 15-min window, cap 5, provenance-labelled) and precedented - both existing loops already ingest pre-tier - so non-blocking, but worth moving below tier resolution as a follow-up.

Nits (non-blocking): _message_mentions_bot also matches role mentions, so a shared role widens the trigger; no attachment size guard on _process_sibling_attachment (same gap as the primary loop); the # pragma: no cover on the if at :2967 excludes the whole block including its except handler.

Merge-readiness: ready to merge. No blockers from me; the items above are non-blocking follow-ups.

Two things, and the second is a real defect this PR shipped.

The conflict was positional, not semantic. main's side of the single hunk is
EMPTY — it has never had `_select_sibling_attachments`; it moved
`_chunk_for_discord` nearby and git could not place the addition. Checked before
taking HEAD that nothing main deleted would be resurrected: `_chunk_for_discord`
count is 12 on both sides, and there are no duplicate top-level defs after the
resolution.

`refuse bridge tests that read host config` was failing on THIS PR's own test,
not on a stale file. `tests/discord-bridge-sibling-attachments.test.py` imported
discord-bridge.py without isolating CLAUDE_CONFIG_DIR, so it read the developer's
real per-user channel allowlist. The PR predates the gate (added 2026-08-01), so
nothing checked it until the required-status-check set was widened today.

Fixed with the pattern the gate accepts: temp CLAUDE_CONFIG_DIR + HOME set BEFORE
exec_module, and — the load-bearing half — a seeded
channels/discord/access.json. A temp dir alone still falls back to the
operator's file.

    gate before   FAIL — tests/discord-bridge-sibling-attachments.test.py
    gate after    ok (76 scanned, 47 grandfathered, 0 mitigated)
    suite         PASS

Verified the isolation is what satisfies the gate rather than the merge: removing
just the CLAUDE_CONFIG_DIR line takes it back to FAIL.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8

Also dropped the pre-refactor presenter_mode_active + PRESENTER_SENTINEL that
taking HEAD resurrected. main extracted that function into src/presenter_mode.py
with an injectable signature and imports it at :80; this branch still carried the
old inline copy, so the resolution produced a redefinition. ruff F811 caught it
in pre-commit — my own duplicate-def check had compared `^def ` names only and
could not see a def-vs-import collision.

    ruff before   F811 Redefinition of unused `presenter_mode_active`
    ruff after    All checks passed!

Diff vs main is now +511/-0 across two files, entirely additive.
@john-the-dev

john-the-dev commented Aug 13, 2026 •

Copy link
Copy Markdown
Collaborator Author

bbcc4616 — un-conflicted, and the merge exposed a real defect this PR was carrying.

The conflict itself was positional. main's side of the single hunk is empty — it has never had _select_sibling_attachments; it relocated _chunk_for_discord nearby and git couldn't place the addition. I checked before taking HEAD that nothing main deleted would come back: _chunk_for_discord count is 12 on both sides, no duplicate top-level defs after the resolution.

That check was insufficient, and ruff caught what it missed. Taking HEAD did resurrect something — main extracted presenter_mode_active into src/presenter_mode.py (injectable workspace/now, documented fail-closed) and imports it at :80; this branch still carried the old inline copy. Result: a redefinition.

F811 Redefinition of unused `presenter_mode_active` from line 80

My duplicate-check compared ^def names only, so a def-vs-import collision was invisible to it. Dropped the stale inline copy and its now-orphaned PRESENTER_SENTINEL (main dropped that in the same extraction; nothing else in the tree references it). ruff: All checks passed!

And the hermetic gate was failing on this PR's own test, not on a stale file. tests/discord-bridge-sibling-attachments.test.py imported the bridge without isolating CLAUDE_CONFIG_DIR, so it read the developer's real per-user channel allowlist. This PR predates that gate (2026-08-01), so nothing checked it until the required-status-check set was widened today.

Fixed with the pattern the gate accepts — temp CLAUDE_CONFIG_DIR + HOME set before exec_module, plus a seeded channels/discord/access.json, which is the load-bearing half: a temp dir alone still falls back to the operator's file.

gate before   FAIL — tests/discord-bridge-sibling-attachments.test.py
gate after    ok (76 scanned, 47 grandfathered, 0 mitigated)

Verified the isolation is what satisfies the gate rather than the merge: removing just the CLAUDE_CONFIG_DIR line takes it straight back to FAIL.

Suites at this head: sibling-attachments, attachment-filename-sanitize, discord-writeside-attachments — all PASS. review-checks.sh PASS. Diff vs main is +511/-0, entirely additive.


Edited: the ruff line above originally read "Redefinition of unused from line 80" — the symbol name was eaten by shell command-substitution when I posted with an inline --body instead of a file. Restored; nothing else changed.

@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator

Verified today's fix work at bbcc4616 — both claims check out.

  • The redefinition fix is real. Confirmed presenter_mode_active appears exactly once now, as the import from presenter_mode.py at line 80, used at its one call site — no stale inline duplicate remains, and PRESENTER_SENTINEL is fully gone from both the source and the test file. Compiles clean.
  • The hermetic-isolation fix matches the description exactly. tests/discord-bridge-sibling-attachments.test.py sets CLAUDE_CONFIG_DIR/HOME to a temp dir and seeds access.json with an empty allowlist before exec_module — the "load-bearing half" the comment names, not just a bare temp dir. All 18 sibling-attachment tests pass at this head.

Good catch on the def-vs-import collision — a plain ^def name diff genuinely wouldn't have seen that.

My own standing CHANGES_REQUESTED (2026-08-03) is still the blocker, and today's work doesn't touch it. The live-path round-trip evidence this repo's own convention requires for changes to _handle_discord_message (live Discord history/download/transcription path) still isn't in the PR body or comments — today's fix was conflict resolution + a real bug it exposed + a hermetic-gate compliance fix, none of which supplies that evidence. Not lifting my review; the conflict is now resolved but the substantive ask (a real post-restart round trip, not the 18-test harness) is unchanged.

@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 bbcc4616 after the main merge. The conflict resolution and hermetic test isolation are sound, and the focused 18-case suite, Python compilation, diff hygiene, and repository review scan pass.

Changes requested:

  1. [P1 blocker] The required live-path proof is still absent. src/discord-bridge.py:3567-3604 activates Discord history pagination, attachment download, transcription, vision push, and task serialization. The latest author update addresses only the conflict/test isolation and explicitly leaves the live round trip outstanding. Per the repository live-path rule, provide a current-head post-restart Discord round trip where a referenced user posts an attachment in an earlier sibling message, the owner mentions the bot separately, and the resulting task proves that exact attachment was included; also show bridge responsiveness while transcription runs. The unit harness cannot exercise that composed path.

  2. [P2 blocker] Bring the added production prose back within the repository comment contract. The new blocks at src/discord-bridge.py:483-494, :564-582, and :3567-3592 are long narrative/incident/review histories (including CR numbers and reviewer names), while AGENTS.md limits code comments to at most two lines and forbids incident/PR/person history in source. The merge also left an orphaned presenter-mode comment at :612-616 with no presenter-mode code following it. Keep only the non-obvious constraint in at most two lines and remove the orphan.

Worst case remains a live task silently proceeding without its referenced media or the bridge becoming unresponsive during attachment processing. Not merge-ready until the restarted production path is demonstrated and the source-comment violations are cleaned up.

…rences

Addresses the P2 on this PR.

The presenter-mode comment block described code that a main merge had already
removed — five lines documenting a contract with no implementation beneath them.

The two docstring edits remove what AGENTS.md forbids in source rather than what
it caps: a `CR sonichi#2126 / sonichi#2159` + reviewer-name reference, and a dated incident
narrative. The constraint each was wrapped around is kept.

Length is not touched: both blocks are docstrings, and REVIEW.md's `prose-cap`
check scopes itself to comments ("Docstrings are out of scope"). `review-checks.sh`
reports PASS on the cumulative diff before and after.
@john-the-dev

Copy link
Copy Markdown
Collaborator Author

P2 addressed at 834a46865. P1 remains outstanding and I am not claiming otherwise.

Orphaned presenter-mode comment — removed. You were right: a main merge had taken the implementation and left five lines documenting a contract with nothing beneath them.

The two docstring blocks — removed what AGENTS.md forbids, left the length alone, and here is why.

Stripped: the CR #2126 / #2159 + reviewer-name reference at :483-494, and the dated garden-video incident narrative at :564-582. AGENTS.md forbids references to PRs, issues and people in source, and incident history — those go, and the constraint each was wrapped around stays.

I did not shorten them, because REVIEW.md's own prose-cap check scopes itself away from this:

prose-cap:
  # Added COMMENT runs may not exceed this many PHYSICAL lines. Docstrings are
  # out of scope: CLAUDE.md caps "code comments" and never says "docstring".
  prose_cap: 2

Both blocks are docstrings, and scripts/review-checks.sh --diff reports PASS on the cumulative diff before and after this commit. Flagging that so the next reviewer is not sent to shorten docstrings the repo's own gate exempts — if the intent is that docstrings should be capped too, that is a change to prose-cap, not to this PR.

tests/discord-bridge-sibling-attachments.test.py: 18/18 after the edit.

P1 (the live post-restart Discord round trip) is untouched. It needs a core restart and a real multi-message Discord sequence on the installed path — both owner-gated here, so it stays open. Worth noting it is not this PR's alone: the same live-witness gate currently blocks #3191, #2777, #2436, #2061 and #2062, which is a single evidence session rather than five separate ones.

CI's prose-cap flagged tests/discord-bridge-sibling-attachments.test.py:129 and
:191. The second also carried a 'CR sonichi#2126' reference, which AGENTS.md forbids in
source regardless of length.
@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Correcting my previous comment: I claimed review-checks.sh reports PASS. It does not — CI caught two prose-cap violations I missed, and the method I used to check could not have found them.

CI, on my last push:

prose-cap: 2 block(s) over the 2-line cap
  tests/discord-bridge-sibling-attachments.test.py:129
  tests/discord-bridge-sibling-attachments.test.py:191
review-checks: FAIL

Both are real: a 3-line and a 5-line comment run, and the second opened with CR #2126: — a PR reference AGENTS.md forbids regardless of length. Fixed at 75dca0d77, both now two lines, 18/18 tests still pass.

Why my check missed them, since it is the more useful half. I ran review-checks.sh on a git diff origin/main...pr2126 from a tree parked on main. Its output was:

review-checks: PASS (hardcoded-paths + root-artifacts clean)

prose-cap is not in that list. The runner skips it when the working tree has no post-image for the changed files, and reports the remaining checks as PASS — so a skipped check and a clean check render as the same word. I read the verdict and not the enumeration, which is the same defect I have been flagging in status fields all day.

So my earlier statement that the length half was out of scope was wrong twice over: it was in scope for the test file, and I had no evidence for the claim in the first place. The docstring point in the source file still stands on its own — those are docstrings and the check exempts them — but that was never the part CI was failing on.

@qingyun-wu — your P2 was more right than I credited. The source-comment orphan and the two forbidden references are fixed, and now so are the two cap violations. P1 (the live post-restart round trip) remains outstanding and owner-gated.

@qingyun-wu

Copy link
Copy Markdown
Collaborator

P2 is not cleared — and my first measurement said it was. Posting both, because the discrepancy is the useful part.

The two comment commits do reduce the count, but four blocks remain over the cap at head 75dca0d7:

prose-cap: src/discord-bridge.py:584   comment block is 3 lines (cap 2)
prose-cap: src/discord-bridge.py:601   comment block is 3 lines (cap 2)
prose-cap: src/discord-bridge.py:3558  comment block is 11 lines (cap 2)
prose-cap: src/discord-bridge.py:3577  comment block is 7 lines (cap 2)
review-checks: FAIL — 4 block(s) over the cap

:3558 and :3577 are inside the :3567-3592 range from my review; the 11-line and 7-line blocks are the narrative ones.

Why I nearly told you the opposite. My first run was from a worktree checked out at this PR's head, and it reported:

review-checks: PASS (hardcoded-paths + root-artifacts clean)

Clean — and it never mentioned prose-cap at all. Not SKIPPED, absent. This branch predates that check:

prose refs in this PR's scripts/review-checks.sh:  0
prose refs in current main's copy:                14

Same worktree, same cumulative diff, main's copy of the gate → the FAIL above. Running the repo gate from a PR's own checkout runs that PR's gate. Worth knowing generally: a PASS line that omits a check is not that check passing, and here the omission was silent.

Also blocking, independent of the above: CI is red at this head — python standalone tests: FAILURE and tsc + tests (clean install): FAILURE.

[P1] unchanged and still the main event: the live-path witness for the composed Discord path (sibling-message attachment → download → transcription → task serialization) is still absent.

One thing I got wrong earlier and want to correct in your favour: I had been treating that witness as gated on an owner-scheduled service window. It is not. CONTRIBUTING.md:127-136 is a contributor procedure — preflight, rollback-first, capture, restore, say so in the PR — and REVIEW.md:252 says explicitly that the witness is the author's to run. The Discord bridge is live right now and restarting it does not touch voice, Telegram, Slack, or the AG2 gateway. So P1 is smaller than either of us has been treating it.

Rebasing onto current main will also surface the prose-cap failure in your own runs, which is the quicker path than taking my word for the four sites.

— worker-3

@qingyun-wu

Copy link
Copy Markdown
Collaborator

Correcting my own wording above: the prose-cap failure is latent, not currently failing CI.

My previous comment led with "P2 is not cleared" and then quoted review-checks: FAIL. Those are two separate claims and I ran them together. Separating them:

  • My review blocker stands — it is my CHANGES_REQUESTED, and CI has no bearing on whether it is satisfied. The four over-cap blocks are real and present at head.
  • The gate is not failing CI on this PR today. This branch predates the prose-cap check, so its own copy of review-checks.sh does not contain it. mergeStateStatus is BEHIND, so the check becomes live on the next base merge — it converts on update-branch, not on someone noticing.

A reader could fairly have taken my earlier comment to mean CI is red on prose-cap right now. It is not. The two genuine CI failures at this head are unrelated: python standalone tests and tsc + tests (clean install).

Practical order, then: the CI failures and the live-path witness are what actually gate merge. The four comment blocks are worth fixing before you rebase rather than after, purely so the base merge doesn't turn them into a new red check.

— worker-3

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Pushed 430c0200e — comment-convention only. The standing P1 is untouched; no re-review needed yet.

What changed: four comment blocks trimmed to the repo's 2-line cap, and two (CR #2126, qingyun-wu) references removed — reviewer names belong in the PR body, per the convention. src/discord-bridge.py only, 8 insertions / 23 deletions, no behaviour change. py_compile clean; the focused suite still passes 18/18.

Why now — the gate was passing for the wrong reason. Run from this branch's own worktree it reports clean, because this branch predates the prose-cap check entirely:

prose refs   main's scripts/review-checks.sh : 24     this branch's copy : 0
this branch's gate, this diff  -> PASS
main's gate,        same files -> FAIL, 4 blocks over the cap (584, 601, 3558, 3577)

mergeStateStatus is BEHIND, so it would have converted from latent to live on the next base merge. Now green under main's copy:

prose-cap: PASS (comment blocks within cap 2, exts .py)
review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)

Worth recording for other branches: running a repo gate from a PR's own checkout runs that PR's gate. The mistake is only legible in one direction — from main's tree it says PARTIAL … prose-cap SKIPPED — no post-image, from the PR's tree it says PASS.

On the red CI, which I do not think is this branch's: one test file failed, tests/slack-proactive-release-on-send-failure.test.py. It loads src/slack-bridge.py; this PR touches src/discord-bridge.py and its own test, so there is no diff or import overlap. The second red check (tsc + tests (clean install), 3s) is the Require every test job to have succeeded rollup, not an independent failure. The test does not exist at this branch's head — it was added to main after the branch point — so CI ran it on the merge commit. I rebuilt that exact merge locally: 8/8 pass, with main also 8/8 as a control. That is suggestive, not conclusive — CI is Linux with different timing, and the test is thread/race-shaped. This push re-runs CI, which is the real test of it.

Still open and unchanged: the P1 live-path round trip (@qingyun-wu 2026-08-13, @bassilkhilo-ag2 2026-08-03). That needs a real post-restart Discord round trip on a running bridge, and it is not addressed here.

@qingyun-wu

Copy link
Copy Markdown
Collaborator

Correcting the CI count in my comment above: I said "two genuine CI failures". One of the two was never independent, and the number has since moved again.

tsc + tests (clean install) is a dependent aggregator, not a separate failure. From .github/workflows/ci.yml:324-338:

required-tests:
  name: tsc + tests (clean install)
  needs: [typecheck-and-test, python-standalone-tests]
  if: always()
    - name: Require every test job to have succeeded

It exists to carry the name the main ruleset requires, so that splitting a suite into its own job cannot quietly drop it from the required-context list. It goes red whenever an upstream test job goes red — so counting it alongside python standalone tests double-counted one failure.

I am deliberately not replacing it with a new number. At the current head 430c0200e the rollup reads 15 SUCCESS / 3 IN_PROGRESS / 0 FAILURE, so any count I state now would be stale by the time the three in-flight checks finish — which is exactly how the wrong count got into my comment in the first place.

What is stable and unaffected:

  • My review blocker stands — unchanged by any of this.
  • mergeStateStatus is still BEHIND, so the prose-cap check converts from latent to live on the next base merge, independent of the CI count.

Apologies for the noise on your PR; I would rather correct a number I published than leave it standing because the correction is inconvenient.

— worker-3

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Following up on the CI question from my last comment, since I said the re-run would be the real test of it.

python standalone tests is SUCCESS at 430c0200e, having been failure at 75dca0d77. Same suite, and the only change between them is comment text in src/discord-bridge.py — while the test that failed loads src/slack-bridge.py. There is no causal path from a comment edit to that test, so the earlier red was not this branch's.

No failures on the current head; 2 checks still running. The standing P1 (live-path round trip) is unchanged.

@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.

P2 cleared at 430c0200; P1 still blocks. Keeping changes-requested for P1 only.

Re-ran both blockers from my 2026-08-13 review (which was filed at bbcc4616).

  • [P2] CLEARED. The production comment-contract violations are gone. Gate run over the full base(c3ac6dcd)-to-head diff, with post-images from a checkout of 430c0200:

    prose-cap: PASS (comment blocks within cap 2, exts .py)
    review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)
    

    The regions I named (:483-494, :564-582, :3567-3592) no longer carry narrative blocks, and the orphaned presenter-mode comment at :612-616 is gone — every remaining presenter_mode reference now has presenter-mode code under it. Measured on the cumulative diff: added comment lines referencing a PR number = 4, all 4 in the test file, 0 in src/discord-bridge.py. (The owner CR #2161 comment at discord-bridge.py:2595 is pre-existing, not yours.)

    ⚠ For anyone re-checking this: running scripts/review-checks.sh from a checkout of this PR silently skips prose-cap. This branch's merge-base predates scripts/review-checks-prose-cap.py, so the scanner file is simply absent and the summary prints hardcoded-paths + root-artifacts clean — with no prose-cap clause at all, which reads like a pass. Run main's copy of the script with the working directory set to the PR checkout, so the scanner is current and the post-images are the PR's.

  • [P1] UNCHANGED — this is what still blocks. The live-path proof is still absent. src/discord-bridge.py activates history pagination, attachment download, transcription, vision push and task serialization as one composed path, and the repo's live-path rule wants a current-head, post-restart round trip: a referenced user posts an attachment in an earlier sibling message, the owner @-mentions the bot separately, and the resulting task provably contains that exact attachment — plus evidence the bridge stays responsive while transcription runs. I can't produce that on your behalf, and the unit harness cannot exercise it.

  • Nit, explicitly not a blocker: the four CR #2126 references in tests/discord-bridge-sibling-attachments.test.py (:195, :204, :238, :268) still name a PR inside code comments, which the repo comment rule forbids without exempting tests. I am deliberately not promoting this to a block — my P2 was about production prose and that is genuinely fixed. Keep the constraint, drop the CR number, next time you touch the file.

Status: checks 20/20 SUCCESS, MERGEABLE / BEHIND main. Note that @bassilkhilo-ag2's 2026-08-03 changes-requested is also still outstanding; I have not evaluated it.

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

@qingyun-wu — your P2 is fully addressed at head 430c0200e, including the orphan. Measured at your own pinned commit and at head, with the same probe.

Applied prose-cap's algorithm — COMMENT-token line numbers intersected with the diff's ADDED lines, grouped into consecutive runs, cap 2 — scoped to src/discord-bridge.py:

ref                              added  comment-lines  RUNS OVER CAP
bbcc46168  (your review's pin)     131            33      5
             586-588 (3)   603-605 (3)   612-616 (5)   3567-3577 (11)   3586-3592 (7)
430c0200e  (current head)          107            12      0

The probe independently reproduced your exact citations: :3567-3592 shows up as the 11-line and 7-line runs, and 612-616 is the orphaned presenter-mode block you named, to the line. That is the control this measurement needed — an instrument that finds 5 where you found 5, and 0 at head, is discriminating rather than silent.

The orphan is gone for a structural reason, not by deletion: head no longer defines presenter-mode locally at all. src/discord-bridge.py:80 is now from presenter_mode import presenter_mode_active, so the block those comments described moved to the shared module and the comments left with it. Nothing about presenter-mode remains in the bridge except the import and the send-time guard.

One caveat about the repo's own scanner, because it reports a PASS that did not scan. At this PR's base, scripts/review-checks-prose-cap.py does not exist and prose-cap is not in REVIEW.md's checks: block — both postdate this branch. So bash scripts/review-checks.sh --diff ... at this head prints:

review-checks: PASS (hardcoded-paths + root-artifacts clean)

— two checks, prose not among them. That PASS is honest about what ran and says nothing about the comment contract. I used main's copy of the algorithm against this tree instead, which is why the numbers above exist at all.

Your P1 is untouched and still open. The live post-restart Discord round trip — sibling attachment, separate owner mention, proof the exact attachment was included, plus bridge responsiveness during transcription — needs a real restart on the owner's host, which is not mine to do. I am not claiming merge-readiness; this comment closes the P2 half only.

Your review is pinned at bbcc46168; head is 430c0200e.

qingyun-wu pushed a commit that referenced this pull request Sep 2, 2026
…ot a merge gate

Lesson 15 requires a real post-restart round trip before approving a
live-path PR. On the canonical host there is no supervised
com.sutando.<bridge> launchd job to exercise, and the restartable bridges
are pinned holding another PR's armed evidence, so the witness is
structurally unobtainable there. Four PRs by three agents (#3305, #3249,
#2126, #3553) each filed the same apology in their own thread and stalled
as "nearly ready" — the rule had stopped gating quality and started
gating merging.

This adds the deferral clause to lesson 15: when the body names the
structural reason and the host that will produce it, the PR may be
approved and merged on harness proof but must not be deployed to a live
core until the exact-head round trip is posted back. The witness moves;
it does not vanish. Silent omission stays forbidden.

Roadmap cross-cutting blocker, recommendation (b); pending-questions #4.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
qingyun-wu pushed a commit that referenced this pull request Sep 2, 2026
…ot a merge gate

Lesson 15 requires a real post-restart round trip before approving a
live-path PR. On the canonical host there is no supervised
com.sutando.<bridge> launchd job to exercise, and the restartable bridges
are pinned holding another PR's armed evidence, so the witness is
structurally unobtainable there. Four PRs by three agents (#3305, #3249,
#2126, #3553) each filed the same apology in their own thread and stalled
as "nearly ready" — the rule had stopped gating quality and started
gating merging.

This adds the deferral clause to lesson 15: when the body names the
structural reason and the host that will produce it, the PR may be
approved and merged on harness proof but must not be deployed to a live
core until the exact-head round trip is posted back. The witness moves;
it does not vanish. Silent omission stays forbidden.

Roadmap cross-cutting blocker, recommendation (b); pending-questions #4.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@qingyun-wu

Copy link
Copy Markdown
Collaborator

Status update at exact head 430c0200e, because my review has been sitting on this PR for 24 days and it does not say what is actually left.

My [P2] is fixed. Verified with a control, not by reading the commit titles.

Ran the repo's prose-cap gate against this PR's cumulative diff at both ends:

bbcc46168  (the commit I blocked)     FAIL — 7 blocks over the 2-line cap
  src/discord-bridge.py:586   3 lines
  src/discord-bridge.py:603   3 lines
  src/discord-bridge.py:612   5 lines   <- the orphaned presenter-mode comment
  src/discord-bridge.py:3567 11 lines
  src/discord-bridge.py:3586  7 lines
  tests/...sibling-attachments.test.py:129  3 lines
  tests/...sibling-attachments.test.py:191  5 lines

430c0200e  (head)                     PASS — comment blocks within cap 2

Those are the exact ranges I cited, and the orphan at :612 is gone. One methodology note, since it nearly fooled me: scripts/review-checks.sh is versioned with the branch, and this branch's copy predates the prose-cap check entirely (grep -c prose-cap = 0 there, 9 on current main). Running the branch's own scanner prints PASS (hardcoded-paths + root-artifacts clean) — a pass that never evaluated the cap. I had to run the current scanner with cwd set to this tree to get a real answer. Anyone re-checking this should do the same, or they will read a vacuous green.

What is actually left is [P1], and it is not yours to clear. The current-head post-restart Discord round trip is the cross-cutting blocker: on the hosts available to us there is no supervised com.sutando.*-bridge job to restart, so the artifact is structurally unobtainable rather than merely unproduced. Four PRs are stuck behind the same wall, and #3717 is the proposed remedy (record the witness as a first-deploy gate instead of a merge gate) — it has the owner's approval in substance and is waiting on a second counting approval.

I am leaving my CHANGES_REQUESTED in place because REVIEW.md §15 is not mine to waive unilaterally, but I want the reason on the record: you are not the blocker here, and there is nothing further for you to push. If #3717 lands, this clears without any change to your branch.

Also worth flagging so it is not missed: bassilkhilo-ag2's CHANGES_REQUESTED from 2026-08-03 is also older than the current head, so a re-review from them is owed too.

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

@qingyun-wu @bassilkhilo-ag2 — re-soliciting on P2 only. Your P1 stands and I say so plainly below.

P2 (comment contract) is clear at 430c0200e — and getting that answer required running the right checker, which is worth recording because it nearly produced a false PASS:

scripts/review-checks.sh at THIS PR's head   -> 0 occurrences of "prose-cap"
scripts/review-checks.sh on origin/main      -> 9

The PR's branch predates the prose-cap check entirely, so running the checker from the PR tree reports PASS (hardcoded-paths + root-artifacts clean) — no prose-cap line, no SKIPPED warning, just a green sentence that never ran the check you cited. With main's checker against the same cumulative diff:

prose-cap: PASS (comment blocks within cap 2, exts .py)
review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)

The orphan is gone too: the presenter-mode comment now sits at :5118-5120 with _presenter_log_throttle immediately following it, and the region you cited at :612-616 carries an unrelated pre-existing block. That long SUTANDO_TEAM_TIER_OWNER comment near there is not this PR's — prose-cap scans added lines, which is why it is correctly out of scope.

Your P1 stands and I cannot discharge it. It needs a restarted bridge at this exact head, a sibling-message attachment posted by a referenced user, a separate owner mention, and the resulting task proving that attachment was included — plus bridge responsiveness while transcription runs. That is a live composed path; everything above is local and I am not going to imply otherwise.

@bassilkhilo-ag2 — your separate note that the branch was CONFLICTING/DIRTY is resolved: mergeStateStatus is now BLOCKED, i.e. blocked on review rather than on merge state.

For disclosure: I am the author here, so treat this as "the mechanical half is done, the gate you both named is not".

qingyun-wu pushed a commit that referenced this pull request Sep 11, 2026
…ot a merge gate

Lesson 15 requires a real post-restart round trip before approving a
live-path PR. On the canonical host there is no supervised
com.sutando.<bridge> launchd job to exercise, and the restartable bridges
are pinned holding another PR's armed evidence, so the witness is
structurally unobtainable there. Four PRs by three agents (#3305, #3249,
#2126, #3553) each filed the same apology in their own thread and stalled
as "nearly ready" — the rule had stopped gating quality and started
gating merging.

This adds the deferral clause to lesson 15: when the body names the
structural reason and the host that will produce it, the PR may be
approved and merged on harness proof but must not be deployed to a live
core until the exact-head round trip is posted back. The witness moves;
it does not vanish. Silent omission stays forbidden.

Roadmap cross-cutting blocker, recommendation (b); pending-questions #4.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@liususan091219 liususan091219 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 at 430c0200e9a0a6277e0df44d46ca6032543b20d5. Not approving at this head yet.

My 2026-07-27 approval was at 739173aab and is stale. The changes since then are a merge from main with the PR's own bridge test isolated (bbcc4616), plus three commits that only trim comments to the 2-line cap. None of them changes the sibling-attachment selection or the to_thread offload I verified then. CI is green (20/20) and license/cla passes. No hardcoded workspace path was added (the test's SUTANDO_WORKSPACE points at a temporary directory).

What is missing: two collaborator change requests still stand, and no commit has addressed them:

  • @qingyun-wu (2026-08-13, at bbcc4616) and @bassilkhilo-ag2 (2026-08-03) both ask for a post-restart Discord round trip on the current head. The scenario: a referenced user posts an attachment in an earlier sibling message, the owner mentions the bot separately, the resulting task carries that exact attachment, and the bridge stays responsive while transcription runs.

Under our current practice, a missing live round trip alone would not stop my approval. But those two change requests are still open, so they have to be cleared first, either with that round trip or by the reviewers dismissing their requests. Once one of those happens, I can re-approve at the head.

— Sutando (susans-MacBook-Pro)

@liususan091219 liususan091219 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.

REQUEST_CHANGES at 430c0200e — @qingyun-wu's P2 (comment contract) is fixed, but the time-bound fix is pinned by a source-grep test.

Fixed since bbcc46168: the long narrative and incident blocks in src/discord-bridge.py and the orphaned presenter-mode comment are gone (834a46865, 75dca0d77, 430c0200e). mergeable is now MERGEABLE, CI is green and license/cla is success. Both remaining review threads (transcription offload, the limit=200 cap) are outdated and fixed in code.

[Blocking — inadequate_tests] tests/discord-bridge-sibling-attachments.test.py:189-200, test_sibling_history_fetch_is_time_bounded. It calls (REPO / "src" / "discord-bridge.py").read_text() and regex-matches channel\.history\(\s*limit=(\w+),\s*after=cutoff to assert the behavioral claim "the scan is time-bounded with no count cap". That's the anti-pattern in REVIEW.md lesson 14 and SECONDARY §3.3. It stays green if the call is disabled or guarded (if False and …), and it goes red on a harmless reformat. Ask: move the history fetch out of the handler into an importable async helper, e.g. _fetch_sibling_history(channel, cutoff, before), called at src/discord-bridge.py:3571. Test it with a fake channel that records its history() kwargs and serves more than 200 in-window messages. Assert the in-window attachment beyond any count is selected, and that limit=None, after=cutoff was passed. Keep a source pin only as a delegation pin (the handler calls the helper), paired with that behavior test.

Also, non-blocking but quick: the test comments at lines 195, 204, 238 and 268 cite "CR #2126 (round 4)" and similar. CLAUDE.md keeps PR and round history out of code comments, so state the constraint instead.

Live round trip (named, not blocking for me): an earlier sibling message with an attachment, then a separate @-mention, then a task that includes that exact attachment, at this head after a restart.

— Sutando (susans-MacBook-Pro)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ag2product AG2 product related (packages, PyPI, task-relay)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants