fix(photon): do not retry shared-line 'Target not allowed' outbound rejections (#51897) - #61464
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: COMMENT (high surface area)
What the PR Does
Do not retry shared-line 'Target not allowed' outbound rejections.
Assessment
- High surface area: ~169 additions, 1 file.
- Correctness: Non-retry logic for specific rejection type. Human reviewer should verify the rejection classification is complete and correct.
Note: PR was previously unreviewed.
Reviewed by Hermes Agent
f103014 to
5473660
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for documenting the shared-line limitation and isolating the intended retry behavior. The code path needs rework before it can fix #51897.
Problems
plugins/platforms/photon/sidecar/index.mjs:735sends throughspace.send(), but its outer catch returns the genericinternal sidecar errorresponse (:844-850).plugins/platforms/photon/adapter.py:98treats that generic text as retryable. Therefore the new raw-text check atadapter.py:1452cannot observeTarget not allowed; the added test bypasses this boundary by mocking_sidecar_send()with the raw upstream text.- The cron paths do not reach the added handling:
tools/send_message_tool.py:727callsadapter.send()directly, and standalone delivery returns on a non-200 response atplugins/platforms/photon/adapter.py:1702-1703before either changeddata.get("ok")branch.
Suggested changes
- Return a safe structured sidecar error code for this specific rejection, without exposing raw provider errors, and consume it in both delivery paths. Add tests for that real HTTP contract.
Automated hermes-sweeper review.
| @@ -1424,6 +1452,13 @@ async def _send_with_retry( | |||
| return result | |||
There was a problem hiding this comment.
This classifier will not see the reported provider text in production: space.send() exceptions are converted by sidecar/index.mjs to HTTP 500 with error: "internal sidecar error", which the existing retryable-pattern list matches. Please propagate a safe structured rejection code from the sidecar and classify that instead.
| @@ -1703,7 +1738,18 @@ async def _standalone_send( | |||
| return {"error": f"sidecar returned {resp.status_code}: {resp.text[:200]}"} | |||
There was a problem hiding this comment.
The actual upstream rejection is an HTTP 500, so _standalone_send() returns at the preceding resp.status_code != 200 branch. This new data.get("ok") branch is not reached for the reported failure.
|
Closing — right intent, but the pattern-matching couldn't fire in practice: the sidecar collapsed upstream errors to a generic 500 before your adapter-side patterns ever saw the text. The fix had to start sidecar-side: #73563 added a structured target_not_allowed error class emitted by the sidecar and classified non-retryable in both send paths, with a clear shared-line message. Thanks — the issue you targeted (#51897) is now surfaced cleanly and documented. |
Fixes #51897
Description
Photon's free shared-line pool rejects outbound sends initiated by Hermes (e.g. cron-delivered messages) with
Target not allowed for this project. This is a permanent upstream rejection, not a transient fault — yet_send_with_retry(and_standalone_send) logged the error, retried the same rejected target through its backoff loop, then downgraded to plain text and retried again, never recognizing the failure as final.This PR:
_PHOTON_FATAL_PATTERNS+ a_is_fatal_error()classifier for permanent rejections (shared-line / free-tierTarget not allowed,not allowed for this project, etc.)._send_with_retryon a fatal error: no retry loop, no plain-text downgrade — the failure is surfaced once._standalone_send(cron path), returns a clear message explaining the shared/free-tier limitation and suggesting an alternate delivery channel.Transient errors (e.g.
reset reason: overflow) still retry exactly as before (#50185 behavior preserved).Verification
tests/plugins/platforms/photon/test_shared_line_outbound.py:pytest tests/plugins/platforms/photon/→ 118 passed.