feat(meetings): import a recording into a meeting - #5741
Conversation
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound single-pipeline design; ships a dead Watch
Suggestions
[DESIGN-REVIEWED] d71d117 |
UX Review (Fable 5, fork) — ⏭️ skippedRevision |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsFINDING -- src/kiro_crew/apps/builtins/meetings/backend/routes/audio_import.py:152 -- function-local |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of All evidence gathered. Producing the review. First-Principles-Verdict: CONCERNS The What this change shipsIntent: get notes for a meeting that was recorded outside the app, by feeding an existing recording into a live meeting. ADDITION.
More than 10 differences exist; ATTRIBUTION section, spec update, and black-baseline reformats ride along mechanically. Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] d71d117 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsI've now read the full non-test source diff: the new The candidate list contains zero candidates, so Step 1 has nothing to falsify. Working Step 2, I traced the paths a defect would live on and each resolves to correct behavior:
No behavioral defect grounds to (a)/(b)/(c) at 80+ on the changed lines. No findings. [OPUS-REVIEWED] d71d117 |
|
🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: This PR has been sitting with failing CI and no recent author activity. I've assessed the blockers and they appear resolvable — I'll push fixes directly to this branch as a co-author. Assessment: Conflicts are all in high-churn registry/metadata files (locales, appManifest, app.json, security_posture, black-baseline), none in core meetings logic. Remaining blockers: the GPT session-identity finding in audio_import.py (capture the admitted session at import start and require the same session per dispatched line), re-verification against main's rewritten STT stack, and a no-visual-delta marker for the Screenshot Evidence gate. Note: sibling #5739 shares several files and will be driven first. If you'd prefer I don't touch this PR, add the |
d44de74 to
ffe590e
Compare
|
🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca] Drive-to-green update — head is now 1. Rebase + locale conflict resolution. The branch conflicted with 2. Session-identity fix (the GPT 5.6 blocking finding at 3. Re-verified against 4. 5. PR body: added the Local gates on The author's own note stands: one manual end-to-end import against a real recording + configured STT provider is still required before merge sign-off — nothing in this update performs or claims that verification. |
ffe590e to
db15e70
Compare
|
🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca] Disposition of one Design Review (fork) 🟡 CONCERNS finding on
Also restating the author's own gate rather than claiming it done: manual end-to-end verification against a real recording + configured STT provider has not been performed and remains required before merge sign-off. |
|
🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca] Disposition of one UX Review (fork) 🟡 CONCERNS finding on
|
|
🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca] Disposition of one First Principles Review (fork) 🟡 CONCERNS finding on
|
|
🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca] Applied |
db15e70 to
539df52
Compare
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
POST …/{id}/import takes a host path to an audio file, transcribes it with
the gateway's own batch speech-to-text, and dispatches each line through the
same admission transaction live speech uses, so an imported recording is
persisted to the transcript and reaches the agents exactly as if spoken.
Every dispatched line requires the SESSION OBJECT admitted at import start —
a meeting id is a name, not an identity — so a meeting stopped, deleted, and
recreated under the same id mid-import gets a 410 (meeting_session_replaced)
instead of the old recording's lines.
A recording that would split into more than MAX_IMPORT_LINES lines is
refused whole with 413 (recording_too_long) rather than silently truncated:
a capped import that returns success while the recording's tail is missing
is data loss the user cannot see.
Original feature authored by Kai Mitsuzawa (kaizawa97). Rebase onto main,
locale conflict resolution, the session-identity fix, and the overflow
rejection by Kiro Crew.
Co-authored-by: Kiro Crew <noreply@kirodotdev.github.io>
|
|
Disposition: rebutted — span=f8992237b312 (8th round for this span)
|
|
Disposition: rebutted — span=eb8103c2acd8
|
Problem / Motivation
A meeting that happened outside the app — a phone recording, a call recorded by another tool — cannot reach the Meetings app at all. Its audio has no way into the transcript, so the note-taking crew, task extraction, and the domain dictionary never see it.
Why it matters
The first meeting a user wants notes for is usually one that already happened. Without an import path, the app only works for meetings the user remembered to run it in.
What changed (motivation → approach → change)
Goal: import an existing recording into a live meeting as if it had been spoken — one pipeline, nothing re-implemented, nothing that can drift.
POST /meetings/{id}/import{audio_path}transcribes a host audio file with the gateway's own batch speech-to-text and dispatches the result line by line._common.dispatch_line, the same admission transaction live speech and the broadcast bar use — extracted here so both producers share it rather than copy it. So an imported line is persisted totranscript.jsonlBEFORE fan-out (the app-wide invariant: an accepted agent line cannot be absent from the transcript) and then gets the same dictionary correction, noise gate, per-agent batching and mute list a microphone gets. Admission is re-checked per line, so a meeting stopped mid-import fails promptly with the same 409/410 as live speech; a recording that fills the transcript ceiling gets the same 413.require_session, compared by identity): a meeting id is a name, not an identity, and a meeting stopped, deleted, and recreated under the same id while the audio was transcribing must not be contaminated with the old recording's lines — it gets a 410meeting_session_replacedinstead. The identity check runs before the expiry branch so a producer holding a stale session cannot trigger another session's teardown.copy_file_pinnedgainedmax_bytes): the fstat pre-check refuses a swapped-in oversize (or sparse) source before the destination exists, and the bounded write loop aborts after the first excess byte for a source that grows mid-copy — a ceiling checked only after the copy could exhaust the temp volume on the way to the rejection. Snapshot cleanup now JOINS the copy worker beforermtree(_remove_snapshot_dir), closing the Windows cancellation race where an open handle made the removal silently fail and stale recordings accumulate.is_owner_dashboard_request, checked before the body is read) with the shared denial shape and an audit entry — the same gate aws-control and the app job routes apply, because the capability is the same class: reading an arbitrary host file by path on the caller's say-so.hooks.validate_file_path(the shared dashboard file gate: canonicalization +is_sensitive_path), never a local check — and the extension check runs on the CANONICAL path, so a symlink named.mp3cannot smuggle in its target.transcribe_audiore-checks the path itself; rejections are SEL-audited.error_responsefix folded in (needed by this route): statuses with no literal branch fell through to 400, sostore.contain's 403 for a path escaping the data root was reported as "bad request". Added the FORBIDDEN branch (plus 502/503 for the transcription outcomes) with regression tests pinning 403-not-400.domain/audio.split_transcriptturns the returned blob into lines in three tiers: transcriber segments when given, sentence boundaries for single-paragraph providers, hard wrap at the char limit (wrapped, not truncated). A recording that would split into more thanMAX_IMPORT_LINESlines is refused whole with 413recording_too_long— never silently truncated, because a capped result is indistinguishable from a complete one and the tail would be lost without any signal.linesvsdispatchedare reported separately — the gap is what the noise gate dropped, and "400 lines, 0 dispatched" is an outcome the user must be able to see.MAX_IMPORT_AUDIO_BYTES(512 MiB) is refused 413audio_file_too_largein the vet step, BEFORE the decoder can materialize gigabytes of PCM (GPT round 4); a recording longer than the local decoder's 3600 s ceiling is refused whole with 413recording_too_longBEFORE transcription (GPT round 5) — the local decode paths stop reading at that ceiling without saying so, so without this gate a multi-hour recording would import its first hour and return 200 (silent data loss). The gate is provider-aware (transcribe.batch_duration_cap_secs): AWS Transcribe refuses oversized payloads loudly and Apple reads whole files, so neither is wrongly capped. Duration comes fromtranscribe.audio_exceeds_secs— exact WAV-header math, else a null decode by the same ffmpeg bounded at cap+1 s (metadata probes cannot answer for MediaRecorder webm, which carries no duration header). And one import runs per meeting at a time — a second concurrent request answers 409import_in_progress, because both would dispatch line-by-line into the same transcript and interleave.kiro_crew.pinned_fs.copy_file_pinned(O_NOFOLLOWopen, verdict on the descriptor'sfstat, exclusive-create destination) into a request-private0700temp dir — the duration probe and the transcription consume the snapshot, the size ceiling is re-checked on the copied bytes, a refused source answers 403, and the dir is deleted on every exit path. Round 7 completes the adoption of the central helpers: the source's ANCESTOR chain is pinned too (pinned_fs.pin_parent, oneO_NOFOLLOWopenatper component, the same shape as the app-art reader inapps/routes.py) with the pinneddir_fd+namepair handed tocopy_file_pinned, and a source that vanishes between validation and the copy is tolerated per that helper's documentedFileNotFoundErrorcontract — it answers the same 403 as any other unreadable path instead of a 500. A replacement session still initializing now answers the permanent 410meeting_session_replacedrather than the retryable 409 (dispatch_linecomparesACTIVE.get(meeting_id)withrequire_sessionbefore the no-active-meeting fallback). The handler also loads ONE speech-to-text config snapshot (transcribe.load_stt_config()) and threads it through readiness, the duration ceiling, and the transcription, so a mid-request provider switch cannot skip the gate. The duration probe now spawns ffmpeg through main's authenticated_create_ffmpeg_subprocesshelper rather than directly. Round 8 closes the guard’s last fail-open seam and hardens the snapshot’s error mapping: the duration probe now runs under the SAME time budget as the transcode (stt_config.timeout_secsis passed through; the probe decodes a strict subset of the transcode’s work, so an aligned budget means whatever defeats the probe defeats the transcode too and transcription fails loudly instead of truncating silently), and_snapshot_recordingmaps everyOSErrorout of the pinned copy — permission denied, a component swapped mid-walk, a vanished file — to the same 403audio_path_deniedinstead of letting non-ENOENT errnos escape as a 500.routes/__init__.pyand_common.pybecome black-clean and are therefore pruned from.github/black-baseline.txt(the ratchet fails on graduated files left in the baseline, so the prune cannot ship separately);ATTRIBUTION.mdgains an "Added after the port" section covering the new module. Rebased onto current main after the live-translation feature merged: the shared dispatch transaction (_common.dispatch_line) now carries main's agent-initialization hold (issue meetings: opening speech during agent initialization is not captured (ingress suspended ~46s) #4610) as an explicit opt-in — the live/typed producer holds opening speech, while a file import still refuses whole and retries, preserving the never-partial invariant.dir_fd+O_NOFOLLOW), so Windows answers 501import_unsupported_on_platformbefore any filesystem step. Every by-name variant of the no-dir_fdfallback (name sweep, post-openfd_real_pathwitness, vet-identity check) conceded a residual window: on Windows theos.openitself follows an ancestor junction planted after any check, and a UNC target fires outbound SMB authentication as a side effect of the open, which no after-open check can undo. Per the repo ruling recorded insnapshot.py's notification copy (decline the hand-rolled ctypes walk; refuse loudly), the fallback branch is now a loud fail-closed backstop and the route refuses up front with a user-actionable code. Live-microphone meetings on Windows are unaffected — only the file-import route is gated.Tests
test/test_meetings_audio_import.py(59 tests) — the refusals in order (non-owner 403 / path denied 403 / not a file 404 / oversized file 413 / over-long recording 413 / bad format 400 / concurrent import 409 / STT unavailable 503 / transcription failed 502 / over-long transcript 413),test_a_recreated_meeting_does_not_receive_the_old_recording(red-before-green: a meeting stopped and recreated under the same id mid-import gets 410meeting_session_replacedand its transcript/queues receive nothing from the old recording),test_an_oversized_file_is_413_and_never_reaches_the_decoder+test_an_oversized_file_is_refused_before_decoding(the size ceiling fires in the vet step while the decode cost is still zero),test_a_concurrent_import_into_the_same_meeting_is_409(second concurrent request refused; guard released when the first completes),test_a_line_count_overflow_is_rejected_not_sliced+test_an_over_long_recording_is_413_and_nothing_is_dispatched(a recording past the line budget is refused whole, never silently truncated), the split's boundary rules,test_imported_lines_are_persisted_before_fan_out(asserts the transcript contains exactly the imported lines withsource == "speech"), andtest_both_producers_share_the_dispatch_transaction(source-pins that both handlers go throughdispatch_lineand the expiry side effects live indispatch_admission).test/test_meetings_routes.py— 403-vs-400 containment regression tests; the audio_import module added to the no-blocking-on-loop AST scan.test_a_recording_over_the_decoder_cap_is_413_not_a_truncated_200(red-before-green: the unfixed route returned a truncated 200),test_a_provider_without_a_ceiling_skips_the_duration_probe, andtest_an_unanswerable_duration_probe_proceeds_to_transcription(route);test/test_transcribe.pygainsTestBatchDurationCap+TestAudioExceedsSecs(provider-aware cap answers, exact WAV-header math incl. rate mismatches and the exactly-at-cap boundary, honest None without ffmpeg, ffmpeg-progressparsing). Round-6 coverage:test_one_config_snapshot_feeds_readiness_cap_and_transcription(one config read; the identical object reaches all three calls),test_a_snapshot_refusal_is_403_and_nothing_is_transcribed(route), andTestSnapshotRecording(real filesystem: copy works, a symlink-swapped path is refused, a symlink-swapped ANCESTOR directory is refused, a source grown past the ceiling is refused, a vanished source is refused with 403 rather than raising);test_a_replacement_still_initializing_answers_410_not_409(the recreated session's initialization window answers the permanent 410, and nothing is buffered into its hold). Round-8 coverage:test_the_probe_gets_the_transcodes_own_time_budget(the route hands the probestt_config.timeout_secs, pinning the aligned-budget invariant) andtest_an_unreadable_source_is_refused_not_raised(a chmod-0 source answers the same refusal as any unreadable path — 403, not an unhandled 500).test/test_spawn_audit.pyallowliststranscribe.py::audio_exceeds_secsbeside its sibling_pcm_via_ffmpeg— same fixed-argv ffmpeg, same already-vetted positional path, no output file (-f null -).Local runs (rebased onto current
main): backendisort/flake8/mypy(1284 files) and the black ratchet green; 308 meetings-suite + 379 transcribe/contract/manifest/spawn-audit tests green; websitetsc -band the full vitest suite (1635 test files) green.Manual verification
Not yet performed against a real audio file + configured STT provider (no test decodes audio or calls a model, by design). One end-to-end import against a real recording is still required before merge sign-off.
Related Issues
Part of the meetings feature stack split from the
feat/meetnotebranch. Independent of the other PRs in the stack.no linked issue: feature work from the meetings stack; no tracked issue exists for it.
Screenshots / video
Why no screenshot: API-only; locale strings are the manifest-sync mirror — no rendered pixel changes.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)