fix(photon): preserve sidecar delivery retryability by westkite1201 · Pull Request #51193 · NousResearch/hermes-agent

westkite1201 · 2026-06-23T04:22:42Z

Summary

fixes Photon sidecar hides delivery failure classes behind generic 500s #50971
adds safe error_class and retryable fields to Photon sidecar 500 responses
preserves structured retryability in SendResult for outbound sends and attachments
prevents structured non-retryable sidecar errors from being retried via the legacy generic-error matcher

Testing

uv run --with pytest-timeout pytest tests/plugins/platforms/photon/test_overflow_recovery.py -q
uv run --with pytest-timeout pytest tests/plugins/platforms/photon -q
uv run ruff check plugins/platforms/photon/adapter.py tests/plugins/platforms/photon/test_overflow_recovery.py
python3 -m py_compile plugins/platforms/photon/adapter.py tests/plugins/platforms/photon/test_overflow_recovery.py
node --check plugins/platforms/photon/sidecar/index.mjs
git diff --check
python3 scripts/check-windows-footguns.py --diff origin/main

Notes

Live Photon/Spectrum delivery was not tested locally.

tonydwb

Code Review Summary

Verdict: Approved

Overview

Fixes Photon adapter to properly propagate the retryable flag from structured sidecar errors, preventing non-retryable errors (auth/config) from being unnecessarily retried.

Analysis

Adds PhotonSidecarError exception class with structured fields: path, status_code, error, error_class, retryable
_sidecar_error_from_response() parses the error response into the structured exception
_sidecar_send and _sidecar_send_attachment now catch PhotonSidecarError and return SendResult(success=False, retryable=e.retryable)
_is_retryable_error() updated to explicitly return False for retryable=false and auth_or_config errors
Sidecar (index.mjs) updated to classify errors and return structured responses

Tests

3 new tests: test_structured_non_retryable_sidecar_error_not_legacy_retried, test_structured_sidecar_retryable_error_preserved, test_send_with_retry_uses_structured_retryable_flag

✅ Looks Good

Clean architectural improvement
Proper error propagation with typed exceptions
Good test coverage for both retryable and non-retryable cases

Reviewed by Hermes Agent

tonydwb

Code Review Summary

Verdict: Approved

Preserves sidecar delivery retryability for Photon platform by properly preserving error classes through the error handling path, preventing the sidecar from treating retriable errors as permanent failures.

Changes (3 files, +204/-11)

plugins/platforms/photon/adapter.py: +55/-6
plugins/platforms/photon/sidecar/index.mjs: +48/-5
tests/plugins/platforms/photon/test_overflow_recovery.py: +101 lines test coverage

Quality

Good test coverage
Targeted fix for error class preservation
No security concerns

Reviewed by Hermes Agent

teknium1

Thanks for preserving a safe structured sidecar result. The premise is confirmed on current main: plugins/platforms/photon/sidecar/index.mjs:600-606 still emits only the generic 500 body, and plugins/platforms/photon/adapter.py:97-105,1427 classifies that text as transient.

Problems

plugins/platforms/photon/adapter.py:1282 on this PR only prevents entry into the exponential retry loop. For auth_or_config, _send_with_retry still falls through to the unconditional fallback self.send(...) at plugins/platforms/photon/adapter.py:1342-1351, causing a second delivery attempt. The existing new test checks classification, not this send-count behavior.

Suggested changes

Carry the structured permanent classification to _send_with_retry and return before its fallback send; add a test asserting one send attempt for auth_or_config.

Automated hermes-sweeper review.

teknium1 · 2026-07-15T01:03:06Z

@@ -1232,6 +1279,8 @@ def _is_retryable_error(error: Optional[str]) -> bool:
        if not error:
            return False
        lowered = error.lower()
+        if "retryable=false" in lowered or "auth_or_config" in lowered:


This only skips the exponential network-retry path. _send_with_retry still falls through to the unconditional fallback self.send(...) at lines 1342-1351 for this error, so a structured auth_or_config failure is sent again. Preserve the permanent classification through SendResult and return before that fallback; add a one-attempt test.

Photon's Node sidecar intentionally hides raw handler exceptions, but the Python adapter still needs a safe failure class and retryability bit so delivery retries do not collapse into an opaque generic 500. Constraint: Sidecar responses must not leak raw stack traces or private exception text Rejected: Retry every internal sidecar error | masks permanent auth/config failures Confidence: high Scope-risk: narrow Directive: Keep sidecar error text generic; extend safe error classes instead of exposing raw SDK failures Tested: uv run --with pytest-timeout pytest tests/plugins/platforms/photon/test_overflow_recovery.py -q Tested: uv run --with pytest-timeout pytest tests/plugins/platforms/photon -q Tested: uv run ruff check plugins/platforms/photon/adapter.py tests/plugins/platforms/photon/test_overflow_recovery.py Tested: python3 -m py_compile plugins/platforms/photon/adapter.py tests/plugins/platforms/photon/test_overflow_recovery.py Tested: node --check plugins/platforms/photon/sidecar/index.mjs Tested: git diff --check Tested: python3 scripts/check-windows-footguns.py --diff origin/main Not-tested: Live Photon/Spectrum delivery against a real iMessage account Related: NousResearch#50971

…owed Maintainer follow-up to the #51193 salvage: - _send_with_retry: permanent classes (auth_or_config, target_not_allowed) now short-circuit BEFORE the unconditional plain-text fallback resend, including when a retry attempt surfaces one — no more double-sends of permanently-failing requests. - sidecar classifySidecarError: new structured code target_not_allowed for Spectrum's 'Target not allowed for this project' AuthenticationError (shared/free-tier lines cannot initiate outbound sends to new targets). Classification applies to every handler sharing the catch-all serverError path (/send, /send-attachment, /react, /typing, ...). - _standalone_send now parses the structured error body too (it reads sidecar responses independently of _sidecar_call) and returns error_class/retryable alongside the message. - target_not_allowed maps to a canonical user-facing message in both paths; raw upstream error text never leaks through the structured code. Closes the actionable halves of #50971, #51897, #52794.

teknium1 · 2026-07-29T05:24:42Z

Merged via #73563 — cherry-picked with authorship preserved, plus maintainer rework on top: auth_or_config now also returns before the fallback send (double-send fix), error-class emission widened to /send-attachment, /react, /typing and _standalone_send, and a structured target_not_allowed class was added. Closes #50971.

…owed Maintainer follow-up to the NousResearch#51193 salvage: - _send_with_retry: permanent classes (auth_or_config, target_not_allowed) now short-circuit BEFORE the unconditional plain-text fallback resend, including when a retry attempt surfaces one — no more double-sends of permanently-failing requests. - sidecar classifySidecarError: new structured code target_not_allowed for Spectrum's 'Target not allowed for this project' AuthenticationError (shared/free-tier lines cannot initiate outbound sends to new targets). Classification applies to every handler sharing the catch-all serverError path (/send, /send-attachment, /react, /typing, ...). - _standalone_send now parses the structured error body too (it reads sidecar responses independently of _sidecar_call) and returns error_class/retryable alongside the message. - target_not_allowed maps to a canonical user-facing message in both paths; raw upstream error text never leaks through the structured code. Closes the actionable halves of NousResearch#50971, NousResearch#51897, NousResearch#52794.

tonydwb approved these changes Jun 23, 2026

View reviewed changes

teknium1 reviewed Jul 15, 2026

View reviewed changes

teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026

westkite1201 force-pushed the fix/photon-sidecar-error-classes-50971 branch from ea25810 to cac10c8 Compare July 15, 2026 02:45

teknium1 mentioned this pull request Jul 28, 2026

fix(photon): sidecar lifecycle cluster — crash recovery, event-loop, structured errors (salvage x7) #73563

Merged

teknium1 closed this Jul 29, 2026

This was referenced Jul 29, 2026

fix(photon): recover sidecar after outbound transport drops (salvaged v2) #65392

Closed

Photon sidecar hides delivery failure classes behind generic 500s #50971

Closed

Navigation Menu

Uh oh!

fix(photon): preserve sidecar delivery retryability - #51193

fix(photon): preserve sidecar delivery retryability#51193
westkite1201 wants to merge 1 commit into
NousResearch:mainfrom
westkite1201:fix/photon-sidecar-error-classes-50971

westkite1201 commented Jun 23, 2026

Uh oh!

tonydwb left a comment

Uh oh!

tonydwb left a comment

Uh oh!

teknium1 left a comment

Uh oh!

teknium1 Jul 15, 2026

Uh oh!

teknium1 commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

westkite1201 commented Jun 23, 2026

Summary

Testing

Notes

Uh oh!

tonydwb left a comment

Choose a reason for hiding this comment

Code Review Summary

Overview

Analysis

Tests

✅ Looks Good

Uh oh!

tonydwb left a comment

Choose a reason for hiding this comment

Code Review Summary

Changes (3 files, +204/-11)

Quality

Uh oh!

teknium1 left a comment

Choose a reason for hiding this comment

Problems

Suggested changes

Uh oh!

teknium1 Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

teknium1 commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants