Skip to content

fix(photon): make packaged voice transcoding deterministic - #65836

Open
redeye1011 wants to merge 6 commits into
NousResearch:mainfrom
redeye1011:fix/photon-voice-static-ffmpeg
Open

fix(photon): make packaged voice transcoding deterministic#65836
redeye1011 wants to merge 6 commits into
NousResearch:mainfrom
redeye1011:fix/photon-voice-static-ffmpeg

Conversation

@redeye1011

Copy link
Copy Markdown

What does this PR do?

Makes Photon/iMessage voice replies deterministic on clean installs.

spectrum-ts already transcodes non-M4A voice bytes before upload, but its ffmpeg-static peer is optional and the Photon sidecar did not install it. That leaves first-hop installs dependent on a system ffmpeg. After a successful transcode, Spectrum also preserves the source filename (for example, reply.mp3) while uploading M4A bytes, which iMessage can render as a broken or zero-duration audio attachment.

This PR exact-pins Spectrum's supported static ffmpeg peer as optional and extends the sidecar's existing fail-loud Spectrum patch so transcoded voice uploads always use an .m4a filename. Supported hosts get deterministic conversion; unsupported architectures retain Spectrum's system-ffmpeg fallback instead of failing the whole Photon install. The patch migrates existing installs that already have the older mixed-attachment marker and remains idempotent on subsequent starts.

This is complementary to draft PR #65322: that PR improves adapter-level voice normalization and fallback behavior; this change makes Spectrum's final upload hop deterministic on current main and when the host has no system ffmpeg.

Related Issue

Related: #65322 (companion fix; does not close it)

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Pin ffmpeg-static@5.2.0 as an optional sidecar dependency and lock its transitive tree as optional.
  • Extend patch-spectrum-mixed-attachments.mjs to rewrite Spectrum's post-transcode upload filename to .m4a.
  • Preserve migration/idempotency for already-patched sidecar installs.
  • Ship the complete Photon sidecar runtime and lockfile in wheels and sdists; current release artifacts contain only its README.
  • Add an executable regression fixture proving reply.mp3 becomes reply.m4a at the upload boundary.
  • Extend the existing packaged-artifact CI smoke to inspect the real wheel and sdist for all four sidecar runtime files.
  • Add the nested Photon lockfile to the repository's recurring OSV scan.

How to Test

  1. cd plugins/platforms/photon/sidecar && npm ci
  2. Verify the postinstall patch applies and node --input-type=module -e 'import p from "ffmpeg-static"; console.log(p)' resolves an installed binary.
  3. scripts/run_tests.sh tests/plugins/platforms/photon/ -q
  4. scripts/run_tests.sh tests/test_packaging_metadata.py -q
  5. scripts/run_tests.sh -m integration tests/test_wheel_locales_e2e.py -q
  6. scripts/run_tests.sh -q

Verified locally:

  • Clean npm ci applied the patch to the real @spectrum-ts/imessage@8.0.0 install.
  • Real MP3 -> M4A conversion through @spectrum-ts/core/authoring.ensureM4a and the installed static ffmpeg succeeded.
  • Simulated unsupported Windows ARM64 npm ci succeeded, installed Spectrum, and cleanly omitted the optional static binary.
  • Photon + packaging metadata: 123 passed.
  • Real wheel/sdist artifact smoke: 2 passed.
  • Full suite on final stable HEAD: 41,950 passed, 27 failed; those 27 failures exactly match origin/main across the same six unrelated files in this configured profile, and no Photon tests failed.
  • npm audit findings are unchanged from origin/main: 17 moderate, 0 high, 0 critical.

Supply-chain note

ffmpeg-static@5.2.0 is Spectrum's declared optional peer. Its npm install hook downloads a GPL-3.0-or-later platform binary from the package's GitHub release; the npm lockfile verifies the package tarball, not that downloaded binary. This PR does not redistribute that binary, adds no npm audit findings, adds the sidecar lock to recurring OSV scans, and keeps the dependency optional so unsupported or failed downloads preserve Spectrum's system-ffmpeg fallback. Maintainers should explicitly confirm that trade-off before merge.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs and documented the relationship to Fix Photon iMessage voice replies and threading #65322
  • My PR contains only changes related to this fix
  • I've run the full test suite and all tests pass
  • I've added tests for my changes
  • I've tested on Linux (clean sidecar install plus real static-ffmpeg transcode)

Documentation & Housekeeping

  • Documentation update: N/A; no user-facing configuration changes
  • cli-config.yaml.example: N/A
  • CONTRIBUTING.md / AGENTS.md: N/A; no architecture or workflow change
  • Cross-platform impact considered: unsupported Windows ARM64 install was exercised and degrades to Spectrum's existing system-ffmpeg fallback
  • Tool descriptions/schemas: N/A
  • Maintainer: review the CI-sensitive workflow changes and apply ci-reviewed

Screenshots / Logs

photon-sidecar: spectrum mixed attachment patch patched: .../node_modules/@spectrum-ts/imessage/dist/index.js
supported static-ffmpeg transcode ok: 2020 bytes
unsupported win32/arm64 install degrades cleanly: optional ffmpeg omitted
123 passed
2 passed (real wheel/sdist artifact smoke)

@redeye1011
redeye1011 requested a review from a team July 16, 2026 17:59
@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 sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation labels Jul 16, 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: Approved

Makes packaged voice transcoding deterministic. Larger diff due to test/refactor scope. No security concerns.


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 tracing the clean-install final-hop failure. The premise is present on current main: plugins/platforms/photon/sidecar/package.json:15-17 installs only spectrum-ts, while package-lock.json:595-602 shows ffmpeg-static is merely its optional peer.

Problems

  • tests/test_packaging_metadata.py:160-181 adds a Python source-shape test that reads package.json and workflow text. AGENTS.md:1327-1329 requires package.json/package-lock.json assertions in the JS suite, and AGENTS.md:1380-1384 bans source-shape tests. This duplicates the behavior-oriented packaged artifact coverage added in tests/test_wheel_locales_e2e.py:61-117.

Suggested changes

  • Remove the new metadata test (and json import), keeping the wheel/sdist artifact smoke. If a direct package manifest assertion is required, place it in Vitest.

Automated hermes-sweeper review.

assert "include plugins/platforms/photon/sidecar/*.mjs" in manifest
assert "include plugins/platforms/photon/sidecar/*.json" in manifest

package = json.loads(

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 new Python test reads and asserts package.json/workflow source shape. AGENTS.md requires package-manifest assertions in Vitest and bans source-shape tests; the packaged wheel smoke added in tests/test_wheel_locales_e2e.py already validates the runtime behavior. Please remove this metadata check or move any manifest-only assertion to JS.

@teknium1 teknium1 added sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 18, 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-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation 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 sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants