Skip to content

fix(photon): don't kill sidecar at boot when legacy spectrum-ts dist is absent - #55387

Closed
Mengchee118 wants to merge 1 commit into
NousResearch:mainfrom
Mengchee118:fix/photon-sidecar-nonfatal-missing-dist
Closed

fix(photon): don't kill sidecar at boot when legacy spectrum-ts dist is absent#55387
Mengchee118 wants to merge 1 commit into
NousResearch:mainfrom
Mengchee118:fix/photon-sidecar-nonfatal-missing-dist

Conversation

@Mengchee118

Copy link
Copy Markdown
Contributor

Summary

The Photon Spectrum sidecar dies at every boot once the spectrum-ts SDK layout drifts away from the legacy @spectrum-ts/imessage/dist path (e.g. after the rename to top-level spectrum-ts/dist + @photon-ai/* scopes).

patchSpectrumTs() in patch-spectrum-mixed-attachments.mjs hard-codes that legacy path and does throw new Error(...) when it's missing. index.mjs catches that and turns it into process.exit(3) — so the sidecar exits immediately on startup.

User-visible symptoms:

  • Gateway reports Photon connected, but the status is stale — the sidecar is actually dead.
  • No inbound iMessage is ever delivered.
  • Outbound fails with Photon standalone send requires a running sidecar.
  • Gateway loops on Reconnect photon failed, next retry in 300s.

Fix

The mixed-attachment shim only adjusts text + multi-attachment child ordering, so a missing legacy dist should be a no-op — not fatal. Return { patched: false } and skip gracefully instead of throwing. The spectrum-ts package itself is intact in this scenario; only the patch's hard-coded old path is stale.

   if (!fs.existsSync(dist)) {
-    throw new Error(`@spectrum-ts/imessage dist not found: ${dist}`);
+    // Layout drift: newer spectrum-ts ships as top-level spectrum-ts/dist
+    // (+ @photon-ai/* scopes) and no longer exposes @spectrum-ts/imessage/dist.
+    // The shim only affects text+multi-attachment ordering; skip gracefully
+    // instead of killing the sidecar at boot.
+    return { patched: false, reason: "legacy @spectrum-ts/imessage dist absent" };
   }

Test plan

  • With the legacy dist absent, the sidecar boots, listens on 127.0.0.1:8789, and [photon] connected — streaming inbound over gRPC appears in the gateway log.
  • /healthz returns {"ok":false,"error":"unauthorized"} (alive + enforcing the sidecar token).
  • Verified on macOS against a drifted SDK layout (@photon-ai/* scopes present, @spectrum-ts/imessage/dist absent).

…is absent

The mixed-attachment shim hard-coded node_modules/@spectrum-ts/imessage/dist
and threw a fatal Error when it was missing, which index.mjs catches and turns
into process.exit(3). After the spectrum-ts SDK layout drifted (top-level
spectrum-ts/dist + @photon-ai/* scopes), that path no longer exists, so the
sidecar died at every boot: the gateway reported 'connected' but no inbound
iMessage was ever delivered and outbound failed with 'requires a running
sidecar'.

The shim only adjusts text+multi-attachment child ordering, so a missing legacy
dist should be a no-op, not fatal. Return {patched:false} and skip gracefully
instead of throwing.
@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 Jun 30, 2026

@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: LGTM

Clean 1-file fix tolerating absent legacy spectrum-ts dist in photon sidecar boot. Gracefully returns instead of killing the sidecar.

Looks Good

  • Minimal fix with clear rationale
  • Proper fallback (patched: false, reason provided)
  • Prevents unnecessary sidecar crashes on SDK layout drift

Reviewed by Hermes Agent

@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 boot-failure branch.

Problems

  • This adds no regression test for the new return path. The existing patch tests create node_modules/@spectrum-ts/imessage/dist at tests/plugins/platforms/photon/test_spectrum_patch.py:155-160, so they do not exercise an absent directory.
  • Current main deliberately pins spectrum-ts 8.0.0 (plugins/platforms/photon/sidecar/package.json:16), whose lockfile contains @spectrum-ts/imessage (package-lock.json:605-621), and documents that scoped dist directory as the patch target (plugins/platforms/photon/README.md:174-183). Please establish why an installation missing that required artifact should continue without the mixed-attachment patch.

Suggested changes

  • Add a temp-root Node regression test asserting that a missing dist returns nonfatally.
  • Clarify the supported SDK/layout scenario, and consider aligning this narrow case with the broader nonfatal patch-failure handling proposed in #58047.

Automated hermes-sweeper review.

// `spectrum-ts/dist` (+ `@photon-ai/*`) and no longer exposes the old
// `@spectrum-ts/imessage/dist` path this mixed-attachment shim targets.
// The shim only affects text+multi-attachment ordering; when the legacy
// path is absent, skip gracefully instead of killing the sidecar at boot.

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.

Please add a regression test that invokes this patcher against a temp root without node_modules/@spectrum-ts/imessage/dist and asserts a successful nonfatal result; the current tests only create and patch the scoped dist fixture.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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
@teknium1

Copy link
Copy Markdown
Contributor

Closing as subsumed — #58047 (merged via #73563) made spectrum-patch failure non-fatal across the board, which covers the absent-legacy-dist case as a special case.

@teknium1 teknium1 closed this Jul 29, 2026
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-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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