Skip to content

meet-bot: implement POST /play_audio streaming endpoint#25974

Merged
siddseethepalli merged 1 commit into
mainfrom
run-plan/meet-phase-3-voice/pr-1
Apr 16, 2026
Merged

meet-bot: implement POST /play_audio streaming endpoint#25974
siddseethepalli merged 1 commit into
mainfrom
run-plan/meet-phase-3-voice/pr-1

Conversation

@siddseethepalli
Copy link
Copy Markdown
Contributor

@siddseethepalli siddseethepalli commented Apr 16, 2026

Summary

  • Adds skills/meet-join/bot/src/media/audio-playback.ts with startAudioPlayback/stopAudioPlayback/flushSilence spawning pacat into bot_out
  • Replaces 501 stub in http-server.ts with streaming POST /play_audio and DELETE /play_audio/:streamId for cancellation
  • Unit tests cover ordering, cancellation, and silence flush

Part of plan: meet-phase-3-voice.md (PR 1 of 6)


Open with Devin

@siddseethepalli siddseethepalli merged commit 09c044b into main Apr 16, 2026
@siddseethepalli siddseethepalli deleted the run-plan/meet-phase-3-voice/pr-1 branch April 16, 2026 00:16
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 000519e64f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

} catch {
// Lock may already be released after `cancel()`; fine.
}
activeStreams.delete(streamId);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep replacement stream registered on stream_id reuse

When two POST /play_audio requests reuse the same stream_id, the first request’s finally block always executes activeStreams.delete(streamId). If the second request has already stored its own controller in the map, this unconditional delete removes the new in-flight stream entry, so DELETE /play_audio/:streamId can no longer cancel it (returns 404) and barge-in semantics break under overlap. Only clear the map entry if it still belongs to the current request’s controller.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 2 potential issues.

View 3 additional findings in Devin Review.

Open in Devin Review

} catch {
// Lock may already be released after `cancel()`; fine.
}
activeStreams.delete(streamId);
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.

🔴 Superseded stream's finally block unconditionally deletes from activeStreams, removing the replacement stream's entry

When POST /play_audio receives a stream_id that matches an in-flight stream, it aborts the old stream (line 264) and overwrites the map entry with the new stream (line 276). However, the old stream's finally block at line 352 unconditionally calls activeStreams.delete(streamId). Because AbortController.abort() fires its listener synchronously (setting cancelled = true and resolving abortPromise) but the old stream's read loop only processes this on the next microtask, the new map entry is always written (line 276) before the old stream's finally runs. The old stream therefore deletes the new stream's entry, making DELETE /play_audio/:streamId return 404 for the superseding stream. This silently breaks barge-in cancellation for any stream that reuses an ID.

Execution trace showing the race
  1. New POST: existing.controller.abort() — sync, marks old abortPromise resolved
  2. New POST: activeStreams.set(streamId, { controllerB, handle }) — overwrites entry
  3. (await yields to microtask queue)
  4. Old stream: Promise.race settles with "abort", breaks loop
  5. Old stream finally: activeStreams.delete(streamId)deletes stream B's entry
  6. DELETE /play_audio/:streamId → 404 (stream B is gone from the map)
Suggested change
activeStreams.delete(streamId);
const current = activeStreams.get(streamId);
if (current?.controller === controller) {
activeStreams.delete(streamId);
}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +267 to +269
let handle: AudioPlaybackHandle;
try {
handle = playbackFactory(playbackSpawnOptions);
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.

🚩 Concurrent streams interleave PCM on shared pacat stdin — intentional but undocumented at HTTP layer

The playbackFactory (backed by startAudioPlayback) is a singleton — all concurrent POST /play_audio requests write to the same pacat stdin. If two streams overlap, their PCM bytes are interleaved, and when one is cancelled, the trailing silence flush (line 356) injects zeros into the middle of the other stream's audio. The module-level comment in audio-playback.ts:142-145 explicitly states mixing is "a daemon concern, not the bot's", but the HTTP route doesn't document this or guard against it. The daemon should ensure only one stream is in flight at a time (or the bot should reject concurrent streams), but that's an API contract decision for Phase 3, not a bug in this PR.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

asharma53 added a commit that referenced this pull request Apr 16, 2026
…25977)

* fix(gmail): make retry sleeps signal-aware in Gmail client

The abort controller from sender-digest can now interrupt retry sleep
delays, preventing Promise.allSettled from hanging past the deadline
when batchGetMessages enters exponential backoff.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(macos): gate thinking-anchor reset to toolRunning only (#25968)

Resetting the thinking anchor on .streamingCode can erase valid post-tool
thinking intervals when a late code preview fires after tools complete.
Restrict the reset to .toolRunning so the thinking duration is accurate.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(gmail): safe default for has_prior_reply, time-budget enrichment (#25967)

- Default has_prior_reply to true on API errors (safe direction per SKILL.md)
- Skip reply checks when already rate-limited to avoid wasting quota
- Add time budget to enrichment step using remaining TIME_BUDGET_MS
- Over-fetch sender candidates before capping to max_senders

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(gmail): restrict archive fallback to expired scans, sanitize query (#25966)

* fix(gmail): restrict archive fallback to expired scans, sanitize query

- Only fall back to query-based archiving when scan is truly expired (null),
  not when sender IDs don't match (empty array).
- Quote emails in fallback query to prevent Gmail query injection.
- Update SKILL.md to reflect new fallback behavior.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* docs(gmail): clarify SKILL.md scan expiration fallback behavior

Document the distinction between expired scan (null, falls back to
query) vs sender ID mismatch (empty array, returns error).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(oauth): add gmail.settings.basic scope to Google OAuth defaults (#25970)

The Gmail settings scope is required for filter creation/deletion, label
management, and other settings-level operations. Without it, the
gmail_filters tool fails with 403 ACCESS_TOKEN_SCOPE_INSUFFICIENT.

Existing tokens will be flagged by the credential health service's scope
drift detection, prompting users to re-authenticate.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat: add contacts + contact_channels tables to gateway SQLite (#25951)

* feat: add contacts + contact_channels tables to gateway SQLite

Gateway cutover step 1: declare contacts and contact_channels tables
in the gateway DB schema. This is the foundation for moving contact
auth/authz ownership from the assistant daemon to the gateway.

- contacts: mirrors assistant's contacts table (auth/authz fields only)
- contact_channels: mirrors assistant's contact_channels table with
  same indexes (type+external_user_id, type+external_chat_id)
- m0002-seed-contacts: one-time data migration that seeds both tables
  from assistant.db on first startup (INSERT OR IGNORE, transactional)
- ContactStore: read-only store with prepared-statement queries
  (getContact, listContacts, getContactByChannel, getChannelsForContact)
- IPC handlers: list_contacts, get_contact, get_contact_by_channel,
  get_channels_for_contact — wired into the gateway IPC server
- Tests: ContactStore unit tests + IPC round-trip tests

* review: address Vargas feedback on PR #25951

- Strip contacts table to auth/authz-only: remove notes, user_file,
  contact_type columns (not needed for actor validation)
- Remove m0002-seed-contacts data migration — hold off until endpoints
  have cutover and we're dual-writing
- Move test imports to top level (no more inline await import())
- Use fake channel IDs in tests instead of real ones
- Clean test state between runs (DELETE before seed)
- Update ARCHITECTURE.md + gateway/ARCHITECTURE.md to document the
  contacts ownership migration direction
- Add Drizzle migration + test preload env var cleanup tasks to
  workstream Up Next

---------

Co-authored-by: root <root@assistant-89f9b42a-2563-4bbe-96b7-b2840c145b37-0.assistant-89f9b42a-2563-4bbe-96b7-b2840c145b37.warm-pool.svc.cluster.local>

* fix(runtime): wake adapter drains queue, persists with metadata, broadcasts to all clients (#25972)

* meet-join: SKILL.md guidance for voice participation (#25973)

* fix(gmail): persist blocklist only after archive succeeds (#25971)

Move addToBlocklist() call from before the batch archive operation to
after it succeeds. Prevents corrupted cleanup state when archiving fails.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor(environments): env-aware data and config path layout (#25499)

* feat(environments): daemon vellumRoot() honors BASE_DATA_DIR per-instance override (#25455)

* feat(environments): Swift-side VellumPaths env-aware helpers (#25457)

* feat(environments): route CLI lockfile R/W and allocator through environment helpers; delete getDataDir (#25458)

* feat(environments): route CLI platform token, guardian token, and device-id paths through getConfigDir(env) (#25456)

* feat(environments): route CLI platform token, guardian token, and device-id paths through getConfigDir(env)

* fix(cli): use spyOn for platform-client and guardian-token in teleport tests

`mock.module()` in bun:test replaces a module globally in the process and
provides no way to unmock. `teleport.test.ts` was using it to stub both
`../lib/platform-client.js` and `../lib/guardian-token.js`, so those mocks
leaked into `platform-client.test.ts` and `guardian-token.test.ts` when they
ran in the same bun test process — every call to `readPlatformToken()` in
the platform-client tests returned the literal string "platform-token" from
the stale teleport mock, and `loadGuardianToken()` in the guardian-token
tests returned a minimal fake object missing the `guardianPrincipalId`
field the tests assert on.

Mirror the existing `assistant-config` pattern (already using `spyOn` for
the same reason per its inline comment) for `platform-client` and
`guardian-token`. `spyOn()` mutates the imported module namespace object
only, and `mockRestore()` in `afterAll` fully reverts the stubs so other
test files see the real implementations.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(cli): delete unused LOCKFILE_NAMES export (#25488)

* fix(environments): route daemon protected/ callers through platform helpers (#25493)

* fix(environments): orphan-detection and recover find daemons across all lockfile entries (#25481)

* fix(environments): orphan-detection and recover find daemons across all lockfile entries

* fix(recover): scope collision check to recovering entry's own target path

Addresses Codex P1 and Devin P1 on #25481: the iterate-all-entries loop
blocked recovery whenever any unrelated local assistant was still installed.

* refactor(environments): route Swift client path sites through VellumPaths (#25483)

* refactor(environments): route Swift client path sites through VellumPaths

* fix(environments): remove dead xdgDataHome field + reject relative XDG paths

Addresses Devin and Codex P2 findings on PR #25457:
- xdgDataHome was stored but never read; remove the field and its resolver helper
- resolveXdgConfigHome() no longer rewrites relative XDG_CONFIG_HOME values against cwd — relative values are rejected for parity with the TypeScript env package

* fix(environments): make daemon XDG platform-token and device-id env-aware (#25497)

* refactor(device-id): inline base-dir helper into migration 003

Removes the stale getDeviceIdBaseDir() export from device-id.ts —
getDeviceId() no longer uses it, so it was a maintenance trap whose
return value diverged from where device.json actually resolves in
non-production envs. Its sole remaining caller was workspace migration
003, so inline the 2-line containerized-vs-homedir branch there.

Brings migration 003 closer to the self-containment rule in
assistant/src/workspace/migrations/AGENTS.md (no external imports
beyond types/logger).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* docs(environments): update AGENTS.md and ARCHITECTURE.md for per-assistant data layout (#25504)

* fix(environments): CLI falls back to production on unknown VELLUM_ENVIRONMENT (parity with daemon and Swift) (#25541)

* fix(permissions): restore legacy signing-key path in risk classification (#25542)

* fix(config-watcher): use || for GATEWAY_SECURITY_DIR fallback to match sibling convention (#25543)

* fix(cli): read platformBaseUrl from lockfile instead of legacy workspace config path (#25544)

* fix(chrome-ext): native host reads env-aware lockfile path (#25547)

* fix(environments): VellumPaths accepts relative XDG_CONFIG_HOME to match TS/daemon (#25550)

* refactor(recover): drop unreachable legacy fallback in collision check (#25575)

* fix(cli): sync platformBaseUrl to lockfile on vellum use / vellum wake (#25578)

* fix(environments): env-seed fallback for getPlatformUrl, revert H1 sync-on-switch (#25595)

Under our invariant "each env has its own lockfile, and all assistants in
that lockfile share a platform URL", the H1 workspace-config→lockfile sync
on `vellum use`/`vellum wake` was load-bearing for nothing: switching the
active assistant within a single env cannot change the platform URL.
Revert that sync. When no lockfile is seeded yet, fall back to the current
environment's seed URL instead of the hardcoded production default so
`VELLUM_ENVIRONMENT=dev vellum …` targets `dev-platform.vellum.ai` out of
the box.

- Revert H1: `vellum use` no longer calls `syncActiveAssistantConfigToLockfile`,
  `vellum wake` no longer re-runs `syncConfigToLockfile`, and the helper
  itself is deleted (the hatch-time sync is still done by `syncConfigToLockfile`,
  unchanged).
- `getPlatformUrl()` fallback: prefer `getCurrentEnvironment().platformUrl`
  over the hardcoded prod URL so non-prod CLI users get the right tenant
  before any assistant is registered.
- Tests: drop the H1 sync-on-switch suite, add a dev-env seed fallback test,
  keep the existing prod fallback test.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(environments): drift-guard for KNOWN_ENVIRONMENTS across TS sites (#25617)

The set of recognized environment names is duplicated in three TS
locations: `cli/src/lib/environments/seeds.ts` (SEEDS), the daemon's
`assistant/src/util/platform.ts` (KNOWN_ENVIRONMENTS), and the Chrome
native host's `native-host/src/lockfile.ts` (NON_PRODUCTION_ENVIRONMENTS).
Cross-package imports don't work today — assistant's tsconfig restricts
`include` to its own src tree, and the native host is a standalone TS
project with `rootDir: ./src`.

Add a drift-guard test in cli that parses the literal Set bodies from
both external files and asserts they agree with CLI's SEEDS (minus
`production` for the native host set). Catches any future addition to
the seed table that fails to propagate to the other two sites.

Also refresh the comments on all three declarations to point at the
drift-guard test and the fast-follow plan: hoist the shared name list
into a `packages/environments` package (mirroring `packages/ces-contracts`
etc.) so this check becomes a compile-time import instead of a runtime
regex. That refactor is planned alongside CLI-driven context support.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(environments): forward VELLUM_ENVIRONMENT across desktop→CLI→daemon handoffs (#25633)

Two call sites were stripping VELLUM_ENVIRONMENT from spawn whitelists,
breaking environment isolation for the main desktop launch path:

1. macOS `VellumCli.makeBaseEnvironment()` — `forwardedEnvKeys` did not
   include `VELLUM_ENVIRONMENT`, so every bundled-CLI command launched
   from the app (hatch, wake, sleep, retire, …) ran as production even
   when the app itself was built for a non-production environment.
   The app's Info.plist sets `VELLUM_ENVIRONMENT` at build time
   (`build.sh:1054`), so forwarding it is sufficient.

2. `cli/src/lib/local.ts` compiled-daemon spawn — the `daemonEnv`
   whitelist used when `bun run` is unavailable (packaged desktop
   builds) also omitted `VELLUM_ENVIRONMENT`. Even when the CLI
   process itself had the variable set, the spawned daemon fell back
   to production path/env behavior, so assistant-side env-scoped state
   (device ID, XDG-backed tokens and config reads) bled into prod.

Note: the source/watch daemon spawn path in `local.ts:281` is
unaffected — it uses `{...process.env}` and inherits everything.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* default to dev environment

* remove duplicate env var

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* meet-bot: implement POST /play_audio streaming endpoint (#25974)

* fix(macos): reset thinking anchor on streamingCode when tools active

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com>
Co-authored-by: root <root@assistant-89f9b42a-2563-4bbe-96b7-b2840c145b37-0.assistant-89f9b42a-2563-4bbe-96b7-b2840c145b37.warm-pool.svc.cluster.local>
Co-authored-by: siddseethepalli <siddseethepalli@gmail.com>
Co-authored-by: clopen-set <33433326+clopen-set@users.noreply.github.com>
@siddseethepalli
Copy link
Copy Markdown
Contributor Author

Follow-up fix merged in #26267 — addresses stream_id reuse race, barge-in overlap tracking, ffmpeg probe cache transient errors, playbackChain deadlock, and cancel DELETE timeout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant