Skip to content

feat(recording): capture meeting audio over a WebSocket - #5738

Open
kaizawa97 wants to merge 1 commit into
kirodotdev:mainfrom
kaizawa97:pr/meetings-recording-core
Open

feat(recording): capture meeting audio over a WebSocket#5738
kaizawa97 wants to merge 1 commit into
kirodotdev:mainfrom
kaizawa97:pr/meetings-recording-core

Conversation

@kaizawa97

@kaizawa97 kaizawa97 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

The Meetings app transcribes a meeting but keeps no audio. Once the meeting ends, the transcript is the only record — a misheard sentence, a number the STT mangled, or a dispute about what was actually said cannot be checked against anything. And transcription only hears the local microphone: the other participants of a call (the audio the conferencing app plays back) never reach the app at all.

Why it matters

For any real meeting the far side is most of the content. Without capture of both sides and a durable recording, the notes are only as good as one microphone plus the recognizer's first guess, with no way to recover from either.

What changed (motivation → approach → change)

Goal: record the whole meeting (mic + far side) to a plain WAV in the user's own data directory, without a second audio capture and without coupling core to the app.

  • Core src/kiro_crew/recording/ package — a per-session state machine, a WAV writer, a transcript persister, and crash-recovery scaffolding. Exposed at /api/ws/recording, a separate socket from /api/ws/stt on purpose: that one caps a stream at 300 s (right for dictation, far too short for a meeting). The socket refuses non-loopback clients by default (recording.require_local_gateway, new core config key) — raw room audio is the most sensitive artifact this app produces. Interim/final transcript events emitted back to the browser are redacted first (recording/ws.py is a registered redaction sink in security_posture.py).
  • App-side path resolution via a seam, not an import. Core must not import an app, but the file has to land in the right per-meeting directory. The app registers backend/recording_store.py (a MeetingStore adapter) from register_routes; safe_meeting_id + contain stay in the app, and an id core cannot place is refused rather than started.
  • One capture, many consumers. useMeetingTranscription owns the single audio pipeline and TEES every PCM chunk to the recording through an onPcm option — transcription and recording are two readers of one stream, not two captures. audio/systemAudio.ts captures the far side from the display surface (getDisplayMedia with a 1 fps video track that is immediately stopped and removed — audio-only capture is not expressible per spec). A RecordingMeter shows input level so the user can see the capture is live.
  • Manifest: /api/ws/recording permission + one highlight (mirrored into all 12 locale catalogs per the manifest-sync gate). Spec updated in the same commit (docs/system-specs/modules/meetings.md: Recording section, layout/data rows, security posture).

Tests

  • test/test_recording.py — writer + session state machine (invalid transitions, pause/resume, failure marking).
  • test/test_recording_storage.py — the MeetingStore seam and the app's adapter (containment, refusal of an unplaceable id).
  • test/test_recording_ws.py — socket protocol, the loopback guard, redaction of emitted events.
  • test/test_recording_recovery.py — recovery scanning and status transitions.
  • website/src/test/MeetingsRecording.test.ts — the recording hook's socket lifecycle and PCM tee.
  • website/src/test/MeetingsSystemAudio.test.ts — the getDisplayMedia constraint guard (the shape that rejects must not come back) and track extraction.

Local runs: full backend suite green (mypy 1095 files, flake8, black/brand/harness/subprocess-encoding/docs-lint gates); website tsc, eslint, i18n:check (18 checks), i18n + meetings vitest suites green.

Manual verification

Exercised end-to-end against a live harness gateway (fake ACP backend, loopback client):

  • Opened /api/ws/recording, sent {"type":"start"}, received ready, streamed 2 s of 16 kHz mono s16le PCM in 4096-byte binary frames, then {"type":"stop"}.
  • Confirmed a well-formed audio.wav (32000 frames, 16 kHz, mono) was written under the meeting's own data directory, with the app's contain() check applied to the path.
  • Confirmed interim level/transcript events were emitted back over the socket and passed through redaction.

The screenshots below were captured against the same gateway, showing the live Recording state and the input-level meter. Still pending before merge sign-off: a pass against a real conferencing call's far-side audio — that path (getDisplayMedia system audio) is Electron-dependent and is covered by the stacked follow-up PR.

Screenshots / video

Meeting view, recording controls (dark theme).

Meeting live, Record control shown

Recording in progress: Recording badge, level meter, Pause

Idle meeting (before recording starts)

Meeting idle

Related Issues

Part of the meetings feature stack split from the feat/meetnote branch. A follow-up PR (Electron system-audio capture) depends on this one and will be submitted once this merges.

Checklist

  • Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

Note: local gate runs were made against base abc8222fd, where tsc (and 15 vitest tests in 7 non-meetings files) failed on a then-unfixed main bug (imeLatchRef in useDialogFocusTrap.ts, since repaired by #5522). PR CI builds the merge ref against current main, so that failure does not apply here. This branch adds no typecheck or test failures of its own.

Adds a recording package that owns an audio session: a state machine
per session, a WAV writer, a transcript persister, and recovery for a
session whose process died mid-recording. The dashboard exposes it at
/api/ws/recording, and the Meetings app records what a session
produced through its own recording store.

The frontend captures both the microphone and the system output,
mixes them through the PCM worklet's two inputs, and shows input level
in a meter so a user can see the capture is live before the meeting
starts.
@kaizawa97
kaizawa97 requested a review from a team August 25, 2026 01:32
@kaizawa97
kaizawa97 requested a review from a team as a code owner August 25, 2026 01:32
@kaizawa97
kaizawa97 requested a review from hoang-phan98 August 25, 2026 01:32
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: checking Automated validation is still running labels Aug 25, 2026
@chenmingwei23 chenmingwei23 added the needs-pr-triage PR scanner: awaiting automated triage label Aug 29, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: bolichen97#bb3ad1ca]: This PR has been inactive for 7+ days with failing CI. I reviewed the blockers but they require your input:

  • This PR introduces a new core subsystem (src/kiro_crew/recording/, +5212 lines across 41 files), which is beyond the scope our automation will modify safely — per pipeline policy, PRs over 3000 lines adding a new subsystem are handed back to the author.
  • CI has real failures that need your attention: Backend Tests red on 3.10 and 3.12 (shards 1+2) and Windows (shard 2), plus Coverage Gate, Loop-Bound Locks Gate, and Cross-Platform Portability.
  • The branch also has a merge conflict with main and needs a rebase.

When you've addressed these, the pipeline will re-assess on its next cycle.

@bolichen97 bolichen97 added needs-author-decision PR blocked on author input and removed needs-pr-triage PR scanner: awaiting automated triage labels Aug 29, 2026
@bolichen97
bolichen97 enabled auto-merge August 30, 2026 00:01

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Description / code mismatch

The Description, the spec, the module docstring, and the Manual verification section all describe the recording socket emitting redacted transcript events, but nothing on the socket's request path can produce one.

1. The recording socket never emits the redacted transcript events the Description says it emits

The Description says

Interim/final transcript events emitted back to the browser are redacted first (recording/ws.py is a registered redaction sink in security_posture.py).

and, under Manual verification:

Confirmed interim level/transcript events were emitted back over the socket and passed through redaction.

The code does_emit_transcript is the only place a partial/final transcript event is built and the only place the redactor is called, and api_ws_recording never calls it. No transcription is wired into this socket at all, so the redactor call is dead and no transcript event can reach the browser: src/kiro_crew/recording/ws.py:199. The PR's own frontend documentation agrees with the code rather than with the prose — the wire-protocol header in website/src/apps/meetings/hooks/useMeetingRecording.ts ("Wire protocol — conforms to kiro_crew/recording/ws.py:api_ws_recording") lists only ready and level coming back, while the backend module docstring claims the socket "emits ready, partial, final, level, and error events".

Risk — Four separate surfaces tell a reader that live transcription with credential/exfil redaction happens on this socket: the Description bullet, the spec's Recording section in docs/system-specs/modules/meetings.md, the recording/ws.py module docstring, and the Security Posture row. The panel therefore counts an egress boundary that no request can reach, and the drift guard cannot catch it — test/test_security_posture.py's TestRedactionSinkRegistry only checks that the named module contains a redactor call, so a dead one satisfies it. The sink registration is itself a consequence of the dead call rather than an independent problem: the guard forces any module calling a redactor into either _REDACTION_SINKS or NON_EGRESS_REDACTION_MODULES, so the entry had to be added once the unreachable call existed. What blocks is that the Manual verification bullet reports an observed outcome the code cannot produce, which leaves the rest of that section unreliable for a reviewer trying to decide what was actually exercised.

Required change — Either wire transcription into api_ws_recording so _emit_transcript is reached on the live path, or drop the claim: remove _emit_transcript and the now-unreachable persister path, remove the security_posture.py sink row with it, and correct the Description bullet, the spec's Recording section, the recording/ws.py module docstring, the test module docstring, and the Manual verification bullet to state that the socket carries audio in and ready/level/error out.

@bolichen97

Copy link
Copy Markdown
Collaborator

Audit note — this PR and #7196 are one branch stack, not duplicates

A duplicate-detection sweep flagged these two as near-identical because they share a large block of added lines. They are not duplicates: one branch contains the other's commits, so the shared lines are inherited, and the review diff overstates what this PR actually authored.

This is the textbook stacked-branch artifact, provable by SHA rather than by reading code: git rev-parse pr/5738 = 4bb2f16 and git rev-parse pr/7196^ is the SAME sha, off the same merge base abc8222. git log <mb>..pr/7196 is exactly two commits (4bb2f16 then 375f708) while git log <mb>..pr/5738 is exactly one (4bb2f16). So #5738's entire 41-file/+5212 diff appears verbatim inside #7196's three-dot diff, and the 41 shared files are a STRICT SUBSET of #7196's 47 (comm -23 on the two files.tsv is empty). Only #7196's own commit -- 22 files, +597/-15, none of them backend -- is its own work, and it is strictly additive: it never rewrites a line #5738 wrote, it extends #5738's systemAudio.ts, SYSTEM_AUDIO_KEY and meetings.md Recording section on top. Neither PR duplicates the other's behaviour: #5738 is the backend recording subsystem plus browser capture, #7196 is the Electron main-process loopback grant, and #5738 touches no file under website/electron/ at all.

What is genuinely each PR's own

If #5738 merges, #7196 still carries all of its own work and loses nothing: 6 files #5738 never touches (website/electron/display-media.js +102, main.js +35, preload.js +27, electron/test/display-media.test.js +95, website/src/apps/meetings/audio/captureTier.ts +95, website/src/test/MeetingsCaptureTier.test.ts +171), plus its 35-line extension of systemAudio.ts, the 3-line no-loopback row in SYSTEM_AUDIO_KEY, one catalog key in 13 locales and 36 lines of spec. That is the whole capability of granting real system audio in the desktop shell and degrading to mic-only where the platform cannot -- unlanded and unduplicated. The dependency runs one way only: #7196's own commit CANNOT apply to main alone, because website/src/apps/meetings/audio/ does not exist on origin/main (git ls-tree empty) and SYSTEM_AUDIO_KEY/sysAudioUnsupported have zero hits there, so the files it modifies are created by #5738. Correct sequencing is #5738 first, then #7196 rebases; #7196 merging as-is would simply carry #5738's commit in with it, which is not redundancy but the base of the stack.

Suggested action

This is the base of the stack — it can go first on its own. #7196 should rebase after it lands.


From a repository-wide duplicate/overlap audit of every pull request open against main (2026-09-02, 330 PRs, one reviewer per PR). Each PR was read as its full merge-base diff plus its description and every comment and review, then compared against each candidate PR's own diff and against origin/main at 1a765b88ceb7. This PR is not being closed — the note is informational. If the reading is wrong, please correct the reasoning rather than just the conclusion.

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • This PR is OVERLAPPING with PR #5741. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5738: CONTINUE_DEVELOPMENT. Complementary halves of meeting-audio support with no behavioural duplication; the only interaction is a mechanical co-edit of security_posture.py, register_routes and the meetings spec. Files: src/kiro_crew/security_posture.py, src/kiro_crew/apps/builtins/meetings/backend/routes/__init__.py, src/kiro_crew/recording/writer.py.
  • This PR is OVERLAPPING with PR #7194. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5738: CONTINUE_DEVELOPMENT. Independent Meetings features that collide only on shared surfaces (the toolbar, the session return object, the manifest highlight numbering and 13 locale catalogs). Both should land; the second to land renumbers its highlight and resolves the toolbar conflict. Worth noting for the author because the app.json/appManifest.ts collision auto-merges without a conflict marker and would ship a mismatched App Store highlight list. Files: src/kiro_crew/apps/builtins/meetings/app.json, website/src/apps/meetings/MeetingView.tsx, website/src/apps/meetings/hooks/useMeetingSession.ts.
  • This PR is OVERLAPPING with PR #7196. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5738: CONTINUE_DEVELOPMENT. A branch stack, not a duplicate: PR #5738 is the base and PR #7196 is the dependent. Land PR #5738 first and have PR #7196 rebase onto it. Closing PR #5738 in favour of PR #7196 would be closing the base in favour of the tip and would silently make the whole 5212-line backend subsystem land under the wrong review. Files: src/kiro_crew/recording/ws.py, website/src/apps/meetings/audio/systemAudio.ts, website/src/apps/meetings/hooks/useMeetingSession.ts.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@bolichen97

Copy link
Copy Markdown
Collaborator

@kaizawa97 We audited every open PR against main today, and this one is worth finishing rather than closing.

Nothing on main covers it: no src/kiro_crew/recording/ package, no /api/ws/recording route, no recording.require_local_gateway config key, no website/src/apps/meetings/audio/ directory, and no Recording row in security_posture.py. Merged #5741 added the opposite direction (backend/routes/audio_import.py ingests an audio file someone else already made) and captures nothing. #7196 is stacked directly on this head, so closing this would strand it.

Four things are outstanding:

  1. The CHANGES_REQUESTED review is unaddressed. _emit_transcript in src/kiro_crew/recording/ws.py is never called by api_ws_recording, so the redaction claim in the PR body, docs/system-specs/modules/meetings.md, the module docstring and security_posture.py describes a boundary no request can reach.
  2. Rebase. The branch is 2066 commits behind and conflicted. The non-trivial hunks are src/kiro_crew/apps/builtins/meetings/backend/routes/__init__.py (rewritten by merged feat(meetings): import a recording into a meeting #5741), src/kiro_crew/dashboard/routes/memory.py (its /api/stt/install anchor no longer exists on main), and website/src/apps/meetings/MeetingView.tsx.
  3. The manifest highlight needs to become highlight_8: main already carries six, and feat(meetings): add a per-meeting note the user owns #7194 claims highlight_7. app.json's permissions.api array also conflicts textually with feat(meetings): add a per-meeting note the user owns #7194, since both add a line right after /api/ws/stt.
  4. fix(voice): preserve dictation and stream local speech reliably #9246 replaces website/public/pcm-worklet.js with a polyphase FIR resampler that reads only inputs[0], and it sits at main's tip. Your two-input mix will need re-implementing on top of it, not rebasing.

One smaller item: MAX_CONCURRENT_SESSIONS has two sources of truth, session.py and ws.py's own _MAX_CONCURRENT_SESSIONS.

Audited at 4bb2f16.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

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

Labels

fork Pull request from a fork (external contributor) merge conflict Branch has merge conflicts with its base — author must resolve before merge needs-author-decision PR blocked on author input readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants