Skip to content

fix(photon): preserve native replies - #64711

Open
phall1 wants to merge 2 commits into
NousResearch:mainfrom
phall1:fix/photon-native-replies
Open

fix(photon): preserve native replies#64711
phall1 wants to merge 2 commits into
NousResearch:mainfrom
phall1:fix/photon-native-replies

Conversation

@phall1

@phall1 phall1 commented Jul 15, 2026

Copy link
Copy Markdown

Summary

  • forward the gateway reply_to anchor through Photon text and media sends
  • reply through the cached inbound Spectrum message when available
  • preserve plain space.send behavior for proactive sends and missing/expired targets
  • keep URL-only replies on the reply-capable text route instead of dropping threading for a rich preview
  • add adapter and executable sidecar regression coverage for reply-target propagation

Problem

The gateway already passes the triggering message ID to platform adapters, but Photon accepted reply_to and then discarded it. Every generated response therefore became a new space.send instead of a native iMessage reply. This is particularly brittle for Photon shared-line conversations, where replying to the known inbound message is the authorized and correctly threaded path.

This is complementary to #59867: that PR preserves reply context on inbound user messages; this PR preserves reply context on outbound Hermes responses.

Validation

  • live Photon/iMessage inbound -> Hermes response test after gateway restart
  • scripts/run_tests.sh tests/plugins/platforms/photon -q (136 passed)
  • python -m py_compile plugins/platforms/photon/adapter.py
  • node --check plugins/platforms/photon/sidecar/index.mjs
  • node --check plugins/platforms/photon/sidecar/reply-target.mjs
  • git diff upstream/main...HEAD --check

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved

Overview

Fixes photon channel to preserve native replies instead of converting them to quotedForward-style messages.

Assessment

  • Correctness: Preserving native reply structure is the right behavior for photon.
  • Security: No security-sensitive changes.
  • Debug artifacts: None.

Summary

Clean fix. LGTM.


Reviewed by Hermes Agent

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 15, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for fixing the verified text-reply path. Current main drops the gateway anchor in plugins/platforms/photon/adapter.py:1090, while the streaming sender supplies it at gateway/stream_consumer.py:1922-1929; the PR's text-path change addresses that.

Problems

  • Native media remains unthreaded. send_image, send_image_file, send_voice, send_video, and send_document accept reply_to but omit it when calling _sidecar_send_attachment (plugins/platforms/photon/adapter.py:1100-1170). The sidecar attachment route still unconditionally uses space.send (plugins/platforms/photon/sidecar/index.mjs:738-763). This leaves the same reply-anchor loss for media responses.

Suggested changes

  • Either scope the change explicitly to text, or extend the attachment route to carry reply_to, reply through a cached target when present, retain the space.send fallback, and add coverage for that path.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 16, 2026

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This was generated by AI during triage.

Summary

Three PRs are associated with this cluster, but they address two distinct Photon failure modes: #64711 preserves native reply anchors for outbound text, while #53831 and #65203 harden standalone text delivery against transient failures and add markdown fallback. #65203 is the review-corrected successor to #53831; neither retry PR addresses the reply-threading cause fixed by #64711.

Related pull requests

  • #53831 [closed] related — (+204/-30) — superseded by #65203: This closed PR introduced bounded standalone-send retries and plain-text fallback, but its diff missed safe connection-error retries, omitted configuration documentation, and included an unrelated inbound-dedup change; it remains relevant as the original implementation superseded by #65203.
  • #64711 related — (+30/-5) — keep open pending scope completion: The diff forwards reply_to through the Photon text-send path and replies via a cached inbound message, directly fixing dropped native reply anchors for text. The contributor keep_open review on #64711 remains blocking because media methods and /send-attachment still discard the same anchor; either explicitly limit the PR to text replies or extend and test native media replies before merge.
  • #65203 [closed] related — (+302/-29) — corrected successor to #53831: This closed PR retains the standalone retry and markdown-fallback fix while adding safe ConnectError/ConnectTimeout handling, preserving no-retry behavior for ambiguous read/write timeouts, documenting both controls, and removing the unrelated dedup change; it remains relevant as the review-complete reference implementation for that separate retry issue.

Duplicates

#53831 and #65203 implement substantially the same standalone-send retry change; #65203 supersedes and corrects #53831. #64711 is not a duplicate because it addresses native reply threading.

Suggested consolidation

Merge #64711 only after explicitly addressing its contributor keep_open review by either scoping the change and claim to text replies or propagating and testing reply_to for attachments. Treat #65203 as the corrected reference for the separate retry fix and #53831 as its superseded duplicate; no retry PR should be consolidated into or closed as a duplicate of #64711.

Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 33 kB of PR diffs, 4 kB of issue/PR text, 5 kB of discussion (8 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@phall1

phall1 commented Aug 2, 2026

Copy link
Copy Markdown
Author

Addressed the automated review and rebased the implementation onto current main:

  • forwards reply_to through all Photon media helpers and /send-attachment
  • sends cached targets through Spectrum Message.reply() for both text and attachments
  • preserves space.send() for proactive sends and missing/expired targets
  • keeps URL-only replies on /send so native threading wins over an unthreaded rich-link preview
  • adds executable sidecar behavior coverage plus adapter coverage for image, voice, video, and document replies

Validation: scripts/run_tests.sh tests/plugins/platforms/photon -q — 136 passed.

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

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants