Skip to content

fix(photon): keep sidecar alive if spectrum patch fails - #58047

Closed
tianma-if wants to merge 1 commit into
NousResearch:mainfrom
tianma-if:codex/fix-photon-patch-nonfatal
Closed

fix(photon): keep sidecar alive if spectrum patch fails#58047
tianma-if wants to merge 1 commit into
NousResearch:mainfrom
tianma-if:codex/fix-photon-patch-nonfatal

Conversation

@tianma-if

Copy link
Copy Markdown
Contributor

Summary

  • downgrade Photon sidecar mixed-attachment patch failures from fatal exit to logged warning
  • keep the hard failure for missing/unimportable spectrum-ts itself
  • add a regression assertion covering the nonfatal patch path

Why

The mixed-attachment patch is an enhancement and the Python adapter already treats patch failures as warnings. If the patch script fails because the pinned SDK layout changes, the sidecar should still be able to start for normal Photon messaging instead of exiting before importing spectrum-ts.

This addresses the patch-failure path reported in #58035. It is complementary to #57943, which repairs stale node_modules before startup; this PR changes the Node sidecar's own fallback behavior after a patch failure.

Tests

  • pytest tests/plugins/platforms/photon/test_spectrum_patch.py -q
  • ruff check tests/plugins/platforms/photon/test_spectrum_patch.py
  • ruff format --check tests/plugins/platforms/photon/test_spectrum_patch.py

@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have labels Jul 4, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #58035 (the issue this addresses), #57943 (repairs stale node_modules before startup — complementary), and #55387 (legacy spectrum-ts dist-path boot guard — same recovery family, distinct mechanism). Not a duplicate; each hardens a different failure point of the Photon sidecar startup path.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for isolating the fallback to the patch-error path. The production change directly addresses current main: plugins/platforms/photon/sidecar/index.mjs:225-234 exits before the independently guarded SDK imports at :242-258.

Problems

  • tests/plugins/platforms/photon/test_spectrum_patch.py:24 adds a regression test that reads index.mjs and searches for process.exit. That does not execute the sidecar's failure path, and AGENTS.md:1358-1375 explicitly bans source-code-reading tests.

Suggested changes

  • Replace this with an executable fixture that makes patchSpectrumTs() throw while providing an importable SDK stub, then verifies the sidecar reaches a health endpoint. Keep a separate behavior test for the existing fatal unimportable-SDK path.

This is an automated hermes-sweeper review.

'await import("spectrum-ts")'
)


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This regression test reads source text instead of exercising startup behavior. AGENTS.md:1358-1375 bans source-code-reading tests; please replace it with an executable fixture that makes the patcher fail while spectrum-ts remains importable, then verifies the sidecar starts.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@tianma-if
tianma-if force-pushed the codex/fix-photon-patch-nonfatal branch from 2e43132 to 5049eed Compare July 16, 2026 10:04

@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

Two PRs touch the Photon sidecar but address distinct failure points: #50071 improves diagnosis of upstream CatchUpEvents failures after startup, while #58047 prevents an optional compatibility-patch failure from aborting startup when the SDK remains importable.

Related pull requests

  • #50071 [closed] related — (+35/-4) — distinct diagnostic fix: #50071 labels CatchUpEvents errors as upstream Photon failures while preserving stream re-subscription; although closed, it remains relevant as a complementary runtime-failure reference rather than a solution to the patch-induced startup exit.
  • #58047 related — (+138/-11) — merge candidate: #58047 removes the fatal exit specifically from the optional patch-error path while retaining exit code 3 for an unavailable SDK. Consistent with the keep_open review on #58047, the current diff replaces the criticized source-inspection assertion with executable fixtures proving both successful health startup after a forced patch failure and fatal behavior when the SDK cannot be imported.

Suggested consolidation

Merge #58047 — its production change directly matches the reported startup cause, and its revised behavioral tests address the keep_open review's blocking test-design concern. #50071 is closed and should remain so as a distinct, complementary diagnostic change; no PRs in this complex are duplicates.

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

@teknium1

Copy link
Copy Markdown
Contributor

Merged via #73563 — cherry-picked (latest head with the revised tests) with authorship preserved. Non-fatal patch failure closes the live half of #58035.

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.

4 participants