Repository navigation
feat(discord-bridge): populate outbox_log recipient_label with human-readable channel/user names - #1078
Conversation
…readable channel/user names (follow-up to #1077) outbox_log already has an optional recipient_label field per its schema docs (\"#dev\" or \"Chi DM\"), but discord-bridge.py was never plumbing it — every entry shipped with only the numeric channel/user id. The outbox.log file owner audits via tail/jq therefore reads as opaque IDs instead of #dev / Chi DM / target #ep012. Three callsites now pass recipient_label: - poll_results (channel reply): channel.recipient.name for DM, channel.name for guild → \"Chi DM\" / \"#dev\" - poll_proactive (proactive DM): user.name → \"Chi DM\" - poll_dm_fallback (channel-redirect): target_channel.name → \"#dev\" All three derive the label via getattr fallback to None — the existing schema treats absent label as no-op, matching the optional-field contract. Existing 13 outbox-log tests pass unchanged (the additive field doesn't affect their assertions). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219
left a comment
There was a problem hiding this comment.
Verdict
Clean additive fix. 15 lines, 1 file, zero risk. The optional recipient_label field has been advertised in outbox_log.py's schema docstring all along; this PR finally has the bridge actually pass it. The audit-log readability win ("Chi DM" beats "1507725277630042122" at a glance) is exactly the kind of papercut that compounds — multiply by every line Chi's eyes scan in tail -f state/outbox.log and the savings add up. Ready to merge as-is.
Things I like
-
Three callsites, one pattern.
src/discord-bridge.py:3092-3107,:3252-3262,:3410-3420— all three use the samegetattr(..., None)+ ternary shape, so the read is uniform and an additional bridge poller wiring through the field later only needs to clone the snippet. Five minutes of consistency now pays off the next time someone adds a fourth outbound path. -
Naming convention matches the schema example exactly.
"{name} DM"for DM channels and"#{name}"for guild channels are the two shapes theoutbox_log.pydocstring shows. Reviewer doesn't have to context-switch to figure out "is this what was intended?" — it's a literal mirror. -
getattr(..., None)+ ternary keeps the optional-field contract.src/discord-bridge.py:3097-3101— ifchannel.recipient.nameis None for some weird discord.py edge case, the label degrades to"DM"(with no recipient prefix) rather than crashing with AttributeError. For guild channels at:3103, missingchannel.namedegrades all the way to_label = None, whichoutbox_log.append's optional-field handling treats as omission — matches the pre-PR behavior, no regression risk. -
channel.recipient.name + " DM"for DM,f"#{channel.name}"for guild.src/discord-bridge.py:3097-3104— the channel-type check isisinstance(channel, discord.DMChannel), which is the same predicate the existing code at line 3094 uses to derivech_type. Reusing the same branch keeps the two derived values (ch_typeand_label) trivially in sync — there's no way to end up withch_type="discord_dm"paired with a#foolabel or vice versa. -
No new outbox schema field. PR description states it explicitly and the diff confirms:
recipient_labelalready exists in the schema docstring; this PR only changes whether it's populated. That's the safest possible kind of change — every existing consumer ofstate/outbox.logcontinues to work, and any consumer that was already prepared to readrecipient_label(none yet, AFAIK, but the field was there for a reason) starts seeing real values without a config flip. -
Tests unchanged. PR description says 13 outbox-log tests pass unchanged. That's the correct outcome — the assertions in
tests/outbox-log.test.pydon't checkrecipient_label(the field wasn't populated before), so adding it can't fail any of them. Adding one explicit assertion would lock in the new behavior, but it's a nice-to-have, not a gate.
Non-blocking observations
-
Mild duplication across the 3 callsites. Each one re-derives the label from scratch with the same
if isinstance(..., DMChannel)/getattrpattern. A small helper in this same file (private_label_for_channel(channel)returning the string or None) would dedupe the logic and centralize the format-string change point if you ever want to swap" DM"for" (DM)"or similar. Three callers is at the threshold where the helper barely pays for itself; six callers will be where it's obviously right. Not worth blocking on, just flagging for the next time this pattern grows. -
user.namevsuser.global_namein poll_proactive.src/discord-bridge.py:3254usesgetattr(user, "name", None)which is the legacy unique username (e.g.susanliu_). Discord users also haveglobal_namewhich is the chosen display name (e.g.Susan). For an audit log, the unique username is more stable (display names can change at any time), souser.nameis actually the right call — but it's worth a one-line comment noting the choice so future-you doesn't think it should be "upgraded" toglobal_name. Not blocking. -
poll_dm_fallback uses target_channel, not source channel.
src/discord-bridge.py:3411-3413— when the body has a[channel: <id>]redirect, the audit log shows the redirect target (the channel that received the message). That's the right call — the audit log cares about what was delivered to whom, not where the message originated. Just noting it explicitly so a future reader who wonders "shouldn't we also log the original channel?" sees the answer in this comment: no, that's already captured by the original task's recipient if it had its own outbox entry. -
Tiny test addition would lock the new behavior. A two-line parametrized case in
tests/outbox-log.test.pyalong the lines ofassert json.loads(line)["recipient_label"] == "Chi DM"would prevent a future refactor from silently droppingrecipient_label=from one of the three callsites. Optional follow-up.
Cross-PR / repo linkages
- Direct follow-up to #1077 (per PR body — the producer-side schema change Chi merged earlier; this PR is the bridge-side wire-up that makes the schema field actually carry data).
- No conflict with #1079 (qingyun-wu's sync-memory SUTANDO_MEMORY_DIR fix — different file, different concern).
- Aligns with the audit-log discipline Chi has been building out (
outbox_log.py,tail -f state/outbox.logworkflow). Making the log human-readable rather than UUID-soup matters when you're scanning live to debug a delivery issue — exactly the kind of friction we've been hitting in the recent #1080 / proactive-result debug threads.
Tests note
PR description: 13 existing outbox-log tests pass. That's correct (additive field, no contract change). The nice-to-have is the one-line assertion mentioned in NBO 4. Trade-off is trivial: ~3 lines of pytest in exchange for a regression net against the new behavior. If it's not in this PR, an issue link in the comment thread would be enough to remember to add it.
Ready-to-merge closer
No blockers. The pattern is consistent, the schema field already existed, tests pass unchanged. NBO 1 (helper) and NBO 4 (assertion) are post-merge polish.
— Lucy (Mac Studio bot)
…pient_label (sonichi#1078) and submodule/worktree detection (sonichi#1076) sonichi#1078: recipient_label= in outbox_log.append, '#<channel>' format, '<user> DM' format, getattr safe access, 'DM' fallback, None fallback for missing channel name, 3 delivery paths all covered. sonichi#1076: [ -e .git ] not [ -d .git ] in repo discovery AND commits section, no bare -d .git checks remaining, submodule/worktree comment, CATCHUP_HOURS + positional arg, git log capture-then-head SIGPIPE safety. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…readable channel/user names (follow-up to sonichi#1077) (sonichi#1078) outbox_log already has an optional recipient_label field per its schema docs (\"#dev\" or \"Chi DM\"), but discord-bridge.py was never plumbing it — every entry shipped with only the numeric channel/user id. The outbox.log file owner audits via tail/jq therefore reads as opaque IDs instead of #dev / Chi DM / target #ep012. Three callsites now pass recipient_label: - poll_results (channel reply): channel.recipient.name for DM, channel.name for guild → \"Chi DM\" / \"#dev\" - poll_proactive (proactive DM): user.name → \"Chi DM\" - poll_dm_fallback (channel-redirect): target_channel.name → \"#dev\" All three derive the label via getattr fallback to None — the existing schema treats absent label as no-op, matching the optional-field contract. Existing 13 outbox-log tests pass unchanged (the additive field doesn't affect their assertions). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ask-metadata, and outbox labels Covers: - 22 tests for delivery-idempotency sentinels + send_allowlist (sonichi#1048/sonichi#1029) - 19 tests for task-file channel/guild metadata + inline-tools workspace scan (sonichi#1077/sonichi#1081) - 15 tests for outbox_log recipient_label (sonichi#1078) and submodule/worktree detection (sonichi#1076) All 56 tests pass against current main. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pient_label (sonichi#1078) and submodule/worktree detection (sonichi#1076) sonichi#1078: recipient_label= in outbox_log.append, '#<channel>' format, '<user> DM' format, getattr safe access, 'DM' fallback, None fallback for missing channel name, 3 delivery paths all covered. sonichi#1076: [ -e .git ] not [ -d .git ] in repo discovery AND commits section, no bare -d .git checks remaining, submodule/worktree comment, CATCHUP_HOURS + positional arg, git log capture-then-head SIGPIPE safety. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ask-metadata, and outbox labels Covers: - 22 tests for delivery-idempotency sentinels + send_allowlist (sonichi#1048/sonichi#1029) - 19 tests for task-file channel/guild metadata + inline-tools workspace scan (sonichi#1077/sonichi#1081) - 15 tests for outbox_log recipient_label (sonichi#1078) and submodule/worktree detection (sonichi#1076) All 56 tests pass against current main. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pient_label (sonichi#1078) and submodule/worktree detection (sonichi#1076) sonichi#1078: recipient_label= in outbox_log.append, '#<channel>' format, '<user> DM' format, getattr safe access, 'DM' fallback, None fallback for missing channel name, 3 delivery paths all covered. sonichi#1076: [ -e .git ] not [ -d .git ] in repo discovery AND commits section, no bare -d .git checks remaining, submodule/worktree comment, CATCHUP_HOURS + positional arg, git log capture-then-head SIGPIPE safety. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Follow-up to #1077. The companion read-side patch I offered Chi.
Problem
outbox_log.append()accepts an optionalrecipient_labelfield per the schema docstring insrc/outbox_log.py:…but
src/discord-bridge.pynever passed it. The audit log Chi reads viatail -f state/outbox.logtherefore shows only the numeric IDs:{ "recipient": "1507725277630042122", "body_preview": "..." } // ← which channel was that?…instead of the intended human-readable line:
{ "recipient": "1507725277630042122", "recipient_label": "Chi DM", "body_preview": "..." }Change
Three discord-bridge.py callsites now plumb a derived
recipient_label:poll_results(channel reply)channel.recipient.name + " DM"if DMChannel, elsef"#{channel.name}"poll_proactive(proactive DM)user.name + " DM"poll_dm_fallback(channel-redirect)f"#{target_channel.name}"All three use
getattr(..., None)fallback so a missing attribute degrades torecipient_label=None(whichoutbox_log.appendtreats as omitted — matches the existing optional-field contract).Tests
tests/outbox-log.test.py— all 13 tests pass unchanged. The additive field doesn't break any existing assertions.Notes
recipient_labelalready exists in the JSON-line schema, it just wasn't populated.channel_name/guild_nameto the task-file body. This PR does the symmetric thing for the outbox-log side.🤖 Generated with Claude Code