Skip to content

feat(meetings): import a recording into a meeting - #5741

Merged
iamwhatever merged 1 commit into
kirodotdev:mainfrom
kaizawa97:pr/meetings-audio-import
Sep 7, 2026
Merged

feat(meetings): import a recording into a meeting#5741
iamwhatever merged 1 commit into
kirodotdev:mainfrom
kaizawa97:pr/meetings-audio-import

Conversation

@kaizawa97

@kaizawa97 kaizawa97 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

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.
  • Every line goes through _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 to transcript.jsonl BEFORE 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.
  • Every dispatched line requires the SESSION OBJECT admitted at import start (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 410 meeting_session_replaced instead. The identity check runs before the expiry branch so a producer holding a stale session cannot trigger another session's teardown.
  • Size ceiling enforced INSIDE the snapshot copy (copy_file_pinned gained max_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 before rmtree (_remove_snapshot_dir), closing the Windows cancellation race where an open handle made the removal silently fail and stale recordings accumulate.
  • Owner-only: the handler refuses any caller that is not the dashboard owner (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.
  • The client-supplied path goes through 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 .mp3 cannot smuggle in its target. transcribe_audio re-checks the path itself; rejections are SEL-audited.
  • error_response fix folded in (needed by this route): statuses with no literal branch fell through to 400, so store.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_transcript turns 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 than MAX_IMPORT_LINES lines is refused whole with 413 recording_too_long — never silently truncated, because a capped result is indistinguishable from a complete one and the tail would be lost without any signal. lines vs dispatched are 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.
  • No UI yet — this is an API surface; a host-path picker is a separate UI decision. The App Store highlight advertising the feature is deliberately deferred to the picker PR (maintainer ruling: nothing is advertised before a user can find it in the UI), so the manifest and all locale catalogs are untouched; spec updated in the same commit (Importing a recording section, producer-sharing paragraph, error_response paragraph, security bullets).
  • Resource ceilings and concurrency — a recording file above MAX_IMPORT_AUDIO_BYTES (512 MiB) is refused 413 audio_file_too_large in 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 413 recording_too_long BEFORE 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 from transcribe.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 409 import_in_progress, because both would dispatch line-by-line into the same transcript and interleave.
  • Pinned snapshot + one config snapshot (GPT round 6): the vetted path is a NAME, and anything running as this user could swap it between validation and the transcriber's open. The import now copies the file via the repo's central kiro_crew.pinned_fs.copy_file_pinned (O_NOFOLLOW open, verdict on the descriptor's fstat, exclusive-create destination) into a request-private 0700 temp 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, one O_NOFOLLOW openat per component, the same shape as the app-art reader in apps/routes.py) with the pinned dir_fd+name pair handed to copy_file_pinned, and a source that vanishes between validation and the copy is tolerated per that helper's documented FileNotFoundError contract — it answers the same 403 as any other unreadable path instead of a 500. A replacement session still initializing now answers the permanent 410 meeting_session_replaced rather than the retryable 409 (dispatch_line compares ACTIVE.get(meeting_id) with require_session before 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_subprocess helper 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_secs is 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_recording maps every OSError out of the pinned copy — permission denied, a component swapped mid-walk, a vanished file — to the same 403 audio_path_denied instead of letting non-ENOENT errnos escape as a 500.
  • Riders, declared: routes/__init__.py and _common.py become 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.md gains 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.
  • Platform support: importing requires kernel pinned traversal (dir_fd + O_NOFOLLOW), so Windows answers 501 import_unsupported_on_platform before any filesystem step. Every by-name variant of the no-dir_fd fallback (name sweep, post-open fd_real_path witness, vet-identity check) conceded a residual window: on Windows the os.open itself 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 in snapshot.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 410 meeting_session_replaced and 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 with source == "speech"), and test_both_producers_share_the_dispatch_transaction (source-pins that both handlers go through dispatch_line and the expiry side effects live in dispatch_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.
  • Duration gate: 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, and test_an_unanswerable_duration_probe_proceeds_to_transcription (route); test/test_transcribe.py gains TestBatchDurationCap + TestAudioExceedsSecs (provider-aware cap answers, exact WAV-header math incl. rate mismatches and the exactly-at-cap boundary, honest None without ffmpeg, ffmpeg -progress parsing). 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), and TestSnapshotRecording (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 probe stt_config.timeout_secs, pinning the aligned-budget invariant) and test_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.py allowlists transcribe.py::audio_exceeds_secs beside 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): backend isort/flake8/mypy (1284 files) and the black ratchet green; 308 meetings-suite + 379 transcribe/contract/manifest/spawn-audit tests green; website tsc -b and 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/meetnote branch. 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

  • 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

@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 pepmach 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 labels Aug 25, 2026
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of d71d11780674cbc45727b343610fb06fca861252 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound single-pipeline design; ships a dead src_fd parameter on the security-critical copy primitive and a minutes-long synchronous request contract about to be baked in.

Watch

  • copy_file_pinned(src_fd=...) (pinned_fs.py) has no caller and no test in this PR — it is the residue of the abandoned Windows fd-witness approach, and its docstring still advertises it as "on Windows … the only pinned source form available," the exact pattern the PR's own r31 ruling adjudicated unsafe (the by-name open a caller needs to obtain that fd already follows the junction). A future caller following that guidance reintroduces the hole the 501 gate closed. Delete the parameter and its doc paragraph, or land it with its caller.
  • The import is one synchronous HTTP request spanning snapshot + transcription + line-by-line dispatch — minutes for a long recording. A client/proxy timeout or a session that expires mid-transcription cancels or 410s after the expensive work, and a mid-loop failure leaves a partial transcript with no resume or idempotency (retry duplicates the head). The repo already has an async-job shape (apps/job_routes.py). Since there is no UI yet, this is the last cheap moment to decide sync-response vs job-handle before the picker PR freezes the contract.

Suggestions

  • If the sync shape stays, document the partial-import/retry semantics as part of the API contract in the spec's "Importing a recording" section, not only in code comments.

[DESIGN-REVIEWED] d71d117

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — ⏭️ skipped

Revision d71d11780674cbc45727b343610fb06fca861252 touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed d71d11780674cbc45727b343610fb06fca861252 via the fork AI-review pipeline; updated in place on each push.

Review details

FINDING -- src/kiro_crew/apps/builtins/meetings/backend/routes/audio_import.py:152 -- function-local "from ... import" statements violate top-level-imports -> Fix: move them to module scope.
FINDING -- src/kiro_crew/transcribe.py:1388 -- function-local imports here and at line 2021 violate top-level-imports -> Fix: move both to module scope.
[GPT-REVIEWED] d71d117

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of d71d11780674cbc45727b343610fb06fca861252 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered. Producing the review.

First-Principles-Verdict: CONCERNS

The {audio_path} input re-spells the shipped upload ingestion (api_stt_transcribe), and it alone forces the snapshot/pinning fortress and the Windows 501.

What this change ships

Intent: get notes for a meeting that was recorded outside the app, by feeding an existing recording into a live meeting. ADDITION.

  1. New API: import a host audio file into a live meeting as spoken lines — justified; see Watch
  2. Imported lines ride the exact live-speech pipeline via extracted shared dispatch_line — justified (deletes drift, not adds)
  3. Refusals: owner-only, sensitive-path gate, 512 MiB, 2000 lines, duration cap, one import per meeting — justified
  4. Import refused outright on Windows (501) — cost of the host-path shape only
  5. Data-root escape now answers 403 not 400 (+501/502/503 mappings) — fix rides along, real, consumed
  6. Every ffmpeg run (live voice paths too) pinned to local protocols + named demuxer — justified (untrusted content)
  7. Over-ceiling / unverifiable-duration recordings now refused loudly instead of silently truncated, product-wide — justified
  8. Multichannel WAV transcription now bounded-memory — mechanism fix riding along, justified
  9. copy_file_pinned gains max_bytes/identity pins (1 consumer each) and src_fd — zero consumers
  10. Snapshot staging under voice-runtime root, exempted from the sensitive-path guard — exists only for the host-path shape

More than 10 differences exist; ATTRIBUTION section, spec update, and black-baseline reformats ride along mechanically.

Watch

  • The bytes-ingestion half already exists: api_stt_transcribe (dashboard/handlers/core.py:1271) streams a multipart upload via part_stream.stream_part_to_file(max_bytes=…) into a process-private temp file and calls transcribe_audio — no name to race. The description weighs eight review rounds of TOCTOU hardening (r19–r34) but never the upload alternative that makes them unnecessary; uploaded bytes need no vet gate, no pinned snapshot, no /dev/fd plumbing, no _under_voice_runtime_root exemption, no owner gate for the "arbitrary host file" class — and work on Windows, deleting item 4.
  • "No UI yet — this is an API surface": the named harm is removed today only for a curl-capable owner. Declared, and the highlight deferral cites a maintainer ruling, so noted rather than contested — but the deferred picker is the moment the input shape above gets locked in.

Subtractions

  • Drop src_fd from copy_file_pinned — grepped src_fd across the diff and src/: zero call sites pass it (the import uses dir_fd+name; its motivating Windows consumer became the 501 refusal).
  • Replace {audio_path} with a multipart upload reusing part_stream.stream_part_to_file; this deletes _vet_audio_file, _snapshot_recording, _open_snapshot_pinned, _pinned_consumer_path, the /dev/fd and pass_fds plumbing in transcribe.py, the sensitive-path exemption, and the 501 branch.

[FIRST-PRINCIPLES-REVIEWED] d71d117

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed d71d11780674cbc45727b343610fb06fca861252 via the fork AI-review pipeline; updated in place on each push.

Review details

I've now read the full non-test source diff: the new audio_import.py route, the extracted dispatch_admission/dispatch_line transaction in _common.py, the agents.py refactor, pinned_fs.copy_file_pinned additions (src_fd, expected_src_ident, max_bytes, on_created), security_posture.py, the domain audio.split_transcript, and the transcribe.py/apple_speech duration-gate + demuxer/protocol hardening.

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:

  • Path/TOCTOU: owner gate → validate_file_path (canonicalize + sensitivity) → pin_parent + copy_file_pinned(O_NOFOLLOW, expected_src_ident, max_bytes) → single pinned fd reused by probe and decoder, identity-checked on reopen. No name-trusting step survives; non-pinned platforms refuse (501).
  • SSRF/local-read via ffmpeg: -protocol_whitelist file,pipe prepended to every spawn plus forced -f <demuxer> per validated suffix; descriptor inputs whose suffix can't be resolved raise OSError → loud refusal, never content sniffing.
  • Silent truncation / data loss: size gate before decode; audio_exceeds_secs refuses over-cap AND None under an aligned timeout budget; TranscriptTooLong/413 refuses whole; apple lane mirrors it with its own gate; batch_duration_cap_secs correctly returns None only for AWS (which refuses loudly).
  • Memory: in-loop byte ceiling in copy_file_pinned, byte-bounded multichannel WAV fold (mono keeps prior whole-read behavior; chunks are frame-aligned via readframes; truncated reads terminate on empty).
  • Races: DISPATCH_LOCK transaction, require_session identity pin distinct from meeting-id name, _imports_in_flight check-and-set with no await between test and add, join-then-close-then-rmtree cleanup as a shielded task.
  • Refactor parity: handle_dispatch_text 409/410/413 shapes and hold-during-init semantics match the removed inline logic; redaction still occurs once at the dispatch_line boundary, reflected in NON_EGRESS_REDACTION_MODULES; error mapping for FORBIDDEN/502/503/501 is consistent.

No behavioral defect grounds to (a)/(b)/(c) at 80+ on the changed lines.

No findings.

[OPUS-REVIEWED] d71d117

@github-actions github-actions Bot added 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
@NicholasRBowers NicholasRBowers added drive-to-green PR claimed by drive-to-green pipeline and removed needs-pr-triage PR scanner: awaiting automated triage labels Aug 29, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor

🤖 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 pr-no-autofix label.

@NicholasRBowers
NicholasRBowers force-pushed the pr/meetings-audio-import branch from d44de74 to ffe590e Compare August 29, 2026 04:08
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

Drive-to-green update — head is now ffe590efd (rebased onto current main, single commit, Kai Mitsuzawa's authorship preserved with a Kiro Crew co-author trailer). Changes made, each with its reason:

1. Rebase + locale conflict resolution. The branch conflicted with main in 7 locale catalogs (de, en-XA, en, es, fr, it, pt): this PR renumbers the meetings manifest highlights (inserting the new import highlight as highlight_2), while main added use_case_1/configuration_1 keys to the same block. Resolved union-style — the PR's highlight_2..6 numbering plus main's two new keys — in every affected catalog; the other locales auto-merged the same shape. JSON validity and catalog parity verified.

2. Session-identity fix (the GPT 5.6 blocking finding at audio_import.py:113). A meeting stopped, deleted, and recreated under the same id while the audio was transcribing previously got contaminated with the old recording's lines, because per-line re-admission resolved the session by meeting id alone. Fix as prescribed by the reviewer: the import captures the SESSION OBJECT admitted at its start, and every dispatched line now requires that same object (require_session, compared by identity is — sessions are dataclasses, so equality would not distinguish a same-id replacement). The replacement session gets a 410 meeting_session_replaced and receives nothing. The identity check runs before the expiry branch on purpose, so a producer holding a stale identity cannot trigger another session's lifecycle teardown. Pinned by a red-before-green test (test_a_recreated_meeting_does_not_receive_the_old_recording: stops + restarts the meeting mid-transcription, asserts 410, empty agent queues, and a transcript free of the old recording's lines — fails with 200 + contamination without the fix).

3. Re-verified against main's new STT stack (#6232 rewrote transcribe.py/stt/): transcribe_audio(audio_path) -> str | None and is_available() kept compatible signatures, so no call-site changes were needed; the PR's full test suite (now 36 tests) passes against the rebased tree.

4. .github/black-baseline.txt prune kept. The PR makes routes/__init__.py and _common.py black-clean, and the ratchet fails when a baselined file graduates without being removed (::error::N baselined file(s) are now black-clean), so dropping this hunk would turn Backend Lint red. This means the Fork Workflow-Change Guard will flag the .github/ edit — a maintainer's allow-fork-workflow-change label will be needed (the same resolution used on #4270).

5. PR body: added the <!-- no-visual-delta --> marker + justification (locale/manifest strings trip the frontend detector but nothing renders differently), the session-identity paragraph, and refreshed test counts/gate notes.

Local gates on ffe590efd: isort / flake8 / mypy (1168 files) / black ratchet green; meetings + error-code-contract suites (229 tests) green; website tsc -b + full vitest (1635 test files) green. Two model-pinned local reviewers (GPT gpt-5.6-sol, Opus claude-opus-5) mirroring the CI gates: no blocking findings from either.

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.

@NicholasRBowers
NicholasRBowers force-pushed the pr/meetings-audio-import branch from ffe590e to db15e70 Compare August 29, 2026 04:13
@NicholasRBowers

NicholasRBowers commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

Disposition of one Design Review (fork) 🟡 CONCERNS finding on d44de7457 (the review's other finding has its own comment):

  • fixed — synchronous, non-idempotent POST wrapping a minutes-long transcription, no job model / in-flight guard, duplicating retry path.

    a per-meeting in-flight guard or a documented dedupe story is needed before a UI sits on this

    Initially put to the maintainer as needs-a-decision (option (a): a per-meeting in-flight guard returning 409). Maintainer ruled: add the guard. Implemented at ca3136a10: one import per meeting at a time via a module-level in-flight set held from vetting through dispatch and released in finally on every exit path; a second concurrent request answers 409 import_in_progress. Pinned by test_a_concurrent_import_into_the_same_meeting_is_409 (real concurrency: second request refused while the first is gated mid-transcription; guard released on completion). The larger job-model question stays with the UI/picker PR, as the review's own "before a UI sits on this" framing places it.

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.

@NicholasRBowers

NicholasRBowers commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

Disposition of one UX Review (fork) 🟡 CONCERNS finding on d44de7457 (the review's other findings have their own comments):

  • fixed — phantom capability: the store highlight advertises "Import a recording" in every locale while the PR ships no UI for it.

    Smallest fix: hold this manifest/locale hunk for the UI PR, or reword to name the API explicitly

    Initially put to the maintainer as needs-a-decision (First Principles raised the identical subtraction). Maintainer ruled: defer — nothing is advertised before a user can find it in the UI. Implemented at ca3136a10: highlight_2 removed from the manifest and all locale catalogs; website/ is now byte-identical to the branch base. The highlight moves to the host-path-picker PR alongside the UI it describes.

@NicholasRBowers

NicholasRBowers commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

Disposition of one First Principles Review (fork) 🟡 CONCERNS finding on d44de7457 (the review's other findings have their own comments):

  • fixed — the manifest highlight advertises a UI-less capability (the same subtraction UX Review proposed: defer highlight_2's promotion to the picker PR).

    Put to the maintainer once for both lanes; maintainer ruled: defer. Implemented at ca3136a10: the highlight and its 14 locale mirrors are removed and website/ is byte-identical to the branch base, so the store page no longer advertises what a dashboard user cannot reach. The highlight ships with the picker PR.

@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 29, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

Applied allow-fork-workflow-change: the Fork workflow-change guard fired solely on the .github/black-baseline.txt prune, which this PR cannot drop — it makes routes/__init__.py and _common.py black-clean, and the black ratchet fails when a baselined file graduates without being removed from the baseline. No workflow logic is touched (same resolution as #4270). Also re-ran the Backend Tests (3.10, 2) shard + Coverage Gate: the shard was killed by a runner shutdown signal mid-run (infra churn, no test summary printed), not a test verdict.

@NicholasRBowers
NicholasRBowers force-pushed the pr/meetings-audio-import branch from db15e70 to 539df52 Compare August 29, 2026 05:10
@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=285b6165ed0e

The Windows fallback now opens the source once, requires fd_real_path(src_fd) — the kernel's final path for the inode actually held open — to equal the validated canonical name (fail-closed on an unreadable witness), and hands that same descriptor to copy_file_pinned via a new src_fd parameter, so the validated inode is the copied inode and no name is re-resolved. Same #6794 shape as the UI-file reader in apps/routes.py. Three new tests pin the redirected-open refusal, the fail-closed witness, and the honest path. Fixed in fd19114.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=e851b460434d

Every FFmpeg spawn is now pinned to local protocols: _create_ffmpeg_subprocess — the single seam all three invocations (webm remux, duration probe, transcode) go through — prepends -protocol_whitelist file,pipe ahead of the caller's args, so it precedes every -i and applies to nested playlist-segment opens. A crafted allowed-suffix HLS/ffconcat file can no longer make the demuxer fetch URLs. Pinned by a new test asserting the whitelist leads the argv and precedes -i. Fixed in bc7dd7a.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=e851b460434d

Took the suggested second half: every FFmpeg input is now decoded by the demuxer its validated suffix promises — _forced_demuxer_args maps each import-admissible suffix to its demuxer and every -i site prepends -f <demuxer>, so playlist text in an allowed-suffix file is a decode error instead of a set of local paths to open. A new test asserts every IMPORT_AUDIO_EXTENSIONS entry has a forced demuxer (no attacker-influenced input falls back to content sniffing). Fixed in 5f0e2df.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • rebutted span=f8992237b312

Repeat of the round-17-adjudicated finding: the function-local STT imports are a documented top-level-imports deviation recorded at the site — the STT stack pulls optional heavy dependencies (faster-whisper is not a declared extra) and a gateway that registers this app must not import a decoder at startup — and the laziness is pinned by TestWiring.test_transcribe_is_imported_lazily, so moving them to module scope fails the suite. Sixth round of this span; rationale unchanged.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=285b6165ed0e

Took the suggested fix: the route now opens the snapshot ONCE (_open_snapshot_pinned: O_NOFOLLOW + fstat regular/nlink==1) and hands both the duration probe and the transcriber a descriptor-pinned input — /dev/fd/N on POSIX (the spawn seam inherits N into FFmpeg children via pass_fds; format decisions resolve the real suffix through fd_real_path, refusing when unresolvable), the name on Windows where the held handle blocks the rename/delete a swap needs. A new test replays the attack — swapping the file at its name between probe and transcode — and asserts the transcriber still reads the probed bytes. Fixed in 5506589.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=285b6165ed0e

Took the suggested fix: the pinned descriptor now rides the shielded cleanup task (_remove_snapshot_dir(copy_task, snapshot_dir, snap_fd)), and _close_then_rmtree closes it in the same worker thread BEFORE rmtree — off the event loop per the no-blocking-call rule, and ahead of the removal because the held handle would block the directory delete on Windows. A new test drives the cleanup task directly and asserts the directory is gone and the descriptor is closed (not leaked). Fixed in 81202c0.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=3092b380c7e5

Took the bounded-decode fix: _pcm_from_wav now folds multichannel WAVs to mono in 60-second slices, so the interleaved int16 buffer and its float32 conversion are never held whole — peak transient drops from ~1.5 GiB (four-channel hour) to slice x channels, with the per-frame mean unchanged (readframes counts whole frames, so no frame splits across slices). A test writes a four-channel file spanning the slice boundary and asserts exact equality with a whole-file fold. Fixed in 2a46abc.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=e851b460434d

Took the suggested fix, both halves: the Apple lane's to-native remux is now bounded with -t _MAX_AUDIO_SECS (caps the temp WAV at ~110 MB instead of tracking an unbounded low-bitrate input), and batch_duration_cap_secs now reports that ceiling for the apple provider, so the meetings import refuses over-cap recordings loudly (413) BEFORE the truncation could ever bite. Tests pin the -t argv and the provider's ceiling. Fixed in 2a46abc.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=b79987e2fc43

Took the suggested fix: _to_native_audio skips the native fast path for descriptor paths (_dev_fd_number(audio_path) is not None), so a /dev/fd/N input always reaches the remux — whose FFmpeg child inherits the descriptor via the spawn seam — and the Swift helper receives a NAMED temp WAV it can open. A test opens a real descriptor and asserts the remux branch is reached instead of the fast return. Fixed in 2a46abc.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=285b6165ed0e

Took the identity-verification fix: copy_file_pinned gained an on_created witness (the destination descriptor's publish-time fstat), _snapshot_recording returns the snapshot path WITH that (st_dev, st_ino), and _open_snapshot_pinned accepts the reopen only when the descriptor matches it — a replacement swapped in at the name between copy and pin is refused (403), and a test replays exactly that swap. Third finding on this span; the invariant now: no name-trusting step remains anywhere from validated source → pinned copy → identity-verified pin → descriptor-consumed probe+transcode. Fixed in bcbafd2.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=e851b460434d

Took the suggested fix at the seam: _forced_demuxer_args now REFUSES (OSError -> the caller's loud "could not decode" answer) when a descriptor input's resolved suffix is unmapped — a validly-named import always maps (the coverage test pins every IMPORT_AUDIO_EXTENSIONS entry), so an unmapped answer means a post-pin rename stripped the suffix to re-enable sniffing. The plain-path fallback stays for internal temps. A test replays the rename and asserts the refusal. Fixed in 709b6dd.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=f8992237b312 (audio_import.py:174 instance)

Moved the kiro_crew.pinned_fs import to module scope — unlike the STT-stack imports (which stay lazy per the round-17 rationale: optional heavy dependencies, pinned by the wiring test), pinned_fs is a light non-optional module, so the top-level-imports rule applies. Fixed in 709b6dd.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=f8992237b312 (audio_import.py:559 instance)

Updated the stale provider-cap comment: it now states that the local decoder AND the Apple lane (whose to-native remux is -t-bounded the same way) share the ceiling, while AWS refuses oversized payloads loudly. Fixed in 709b6dd.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=538eb5287174

Took the suggested fix: _to_native_audio now probes duration (audio_exceeds_secs, same decoder + budget as the conversion) BEFORE the remux and raises RecordingTooLongError for over-cap inputs; transcribe() surfaces it as a loud, user-actionable error ("trim it rather than transcribing a truncated prefix") instead of a transcript that silently omits everything past the ceiling. The -t bound stays as the temp-disk guard only. A None probe answer proceeds — it does a strict subset of the remux's decode under the same budget, so the remux fails the same way, loudly. Tests pin the refusal at both layers. Fixed in 4370bcf.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=538eb5287174

Second finding on this span, and correct: my r26 None-proceeds reasoning only covers PERSISTENT probe causes — a transient timeout that clears before the remux would let the -t-bounded conversion succeed on a truncated prefix. None now refuses loudly and retryably (DurationUnverifiedError, "retry, or trim it under 60 minutes"), matching the import gate's r14 rule. The gate sits AFTER the decoder resolve on purpose: the no-ffmpeg degrade path runs no remux, so it has no truncation to guard, while any path that would remux must now prove the duration first. Tests pin both refusals and the degrade exemption. Fixed in a2373b6.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=285b6165ed0e (4th hit; adjudicator-upheld) — in-place rewrite of the snapshot bytes under world-listable system tmp defeated the inode-only pin.

The identity witness proves WHICH inode is read, not that its bytes are
untouched — a same-uid actor reaching the file can rewrite it in place and
pass the pin. Fixed in 6a8daee by removing reachability: snapshots now
stage beneath the agent-denied voice-runtime root (_new_snapshot_dir calls
the central prime_voice_runtime_sandbox_paths, the same root the
transcriber uses for decoder images; every agent sandbox mode denies
read/write/hardlink there). Root validated real-dir/non-link and re-tightened
to 0700 before each request's own mkdtemp. Tests: staged-under-root +
symlinked-root-refused.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=285b6165ed0e (5th hit; adjudicator-upheld) — the vet→copy window: the descriptor pin proved the copied inode was the OPENED inode, not the inode the vet judged.

Fixed in e53e003 at the central seam: copy_file_pinned gains
expected_src_ident (refuses with SKIP_IDENTITY_CHANGED when the pinned
source descriptor fstats to any other inode); _vet_audio_file now returns
the (st_dev, st_ino) it judged and _snapshot_recording passes it to both
platform branches, so a hardlink/inode swapped in at the name after the vet
is never copied. Invariant now end-to-end: vet-identity → identity-checked
pinned copy → publish witness → identity-verified reopen → descriptor
consumption — no step trusts a name against an unverified inode. Tests:
swap-after-vet replay (route helper) + mismatch/match units on the helper.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • rebutted span=f8992237b312 — function-local STT imports are the recorded, maintainer-ruled deviation from top-level-imports (7th repeat of the adjudicated finding).

The maintainer ruling scopes the deviation to the STT stack only:
kiro_crew.transcribe pulls optional heavy decoders (faster-whisper is not
a declared extra), and a gateway that registers this app must not import a
decoder at startup. Each site carries the recorded-deviation comment, and
TestWiring.test_transcribe_is_imported_lazily pins the behavior. All
non-STT imports (pinned_fs, sandbox, platform_compat) are module-scope.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=eb8103c2acd8 — the guard refused the gateway's own staging: crew run/ is a sensitive leaf, so every relocated import snapshot was refused and imports failed unconditionally.

Fixed in fd146a9: _is_sensitive_audio_path exempts real paths under
the gateway's own voice-runtime staging root (new
_under_voice_runtime_root, realpath + commonpath, both POSIX dev-fd and
Windows by-name branches). A link planted under the root resolves outside
it and is judged as its target. Tests: exemption honored +
link-planted-still-refused.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=eb8103c2acd8 — function-local from kiro_crew import pinned_fs in transcribe.py violated top-level-imports.

Fixed in fd146a9: pinned_fs joins the module-scope import line
(from kiro_crew import aws_consent, pinned_fs, platform_compat, stt) and
both function-local imports are removed. Consistent with the maintainer
ruling that the lazy-import deviation covers ONLY the heavy STT stack —
pinned_fs is lightweight stdlib-only and loads at module scope.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=285b6165ed0e (6th hit; adjudicator-upheld) — the Windows by-name open could follow a post-sweep ancestor UNC junction, firing outbound SMB auth before any descriptor check.

Fixed in 7942bf1 by removing the class, not narrowing it again: the
import route now refuses platforms without kernel pinned traversal
(dir_fd + O_NOFOLLOW) with an actionable 501
import_unsupported_on_platform BEFORE any filesystem step, and the
no-pinned-walk copy branch is a loud fail-closed backstop — no by-name
open of a user-influenced path remains on any platform. This follows the
repo ruling recorded in snapshot.py's notification copy (decline the
ctypes walk; refuse loudly). The 3 prior narrowings of this branch are
deleted with their tests; new tests pin the 501 (vet never runs) and the
backstop.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=319e527f9bd2 — the IMPORT_AUDIO_EXTENSIONS comment still said the decoder "does its own format detection", contradicting the forced demuxer.

Fixed in 7942bf1: the comment now documents that the validated
suffix is load-bearing — transcribe._forced_demuxer_args maps each
allowlisted extension to a forced FFmpeg demuxer (-f), so imported
files are decoded as their validated name promises, never by content
sniffing.

@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=b854c21c763a — the tier-1 docstring claimed "Whisper-family models emit one line per segment", contradicting adapters that join segments before calling.

Fixed in 2eda67e: tier 1 is now described as preserving whatever line
structure the CALLER supplied (adapters may join per-segment output with
newlines before calling), which is what the code actually does.

@NicholasRBowers

Copy link
Copy Markdown
Contributor

Rebase fallout, both lanes agree: main's new spawn contract makes callers
own the close and this probe predated it. Fixed in 13273f3 by
mirroring _pcm_via_ffmpeg: the post-resolve body is wrapped in
try/finally with await _close_ffmpeg_for_execution(ffmpeg_bin, preserve_active_exception=exc-in-flight) — every path reaching the finally
has reaped the child (communicate / kill-and-reap) or never spawned one.
Parametrized test pins close-exactly-once, off the loop thread, on success
and failure exits. Also fixed this round: Windows shard-3 route tests
(self-inflicted by the r31 501 gate) — fixture stubs pinned-walk capable,
real-copy tests platform-skipped, backstop test hoisted to run everywhere.

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>
@NicholasRBowers

Copy link
Copy Markdown
Contributor
  • fixed span=e851b460434d — the mono-fold slices were bounded by DURATION, so channel count scaled the transient without limit (a 256-channel WAV under the size cap made one "60-second" slice allocate ~1.5 GiB).

Fixed in d71d117: slices are now bounded by BYTES —
chunk_frames = max(1, 8 MiB // (channels * 2)) — so the raw int16 read
per slice stays at 8 MiB for ANY channel count and the transient (raw +
float32 + fold) stays in the tens of MiB. Result unchanged (per-frame mean
is frame-local; readframes counts whole frames). Tests: the boundary
test crosses the byte-bounded edge, and a new 256-channel test pins
channel-count independence with end-to-end exactness.

@NicholasRBowers

Copy link
Copy Markdown
Contributor

Disposition: rebutted — span=f8992237b312 (8th round for this span)

Deliberate, recorded top-level-imports deviation: the STT stack (kiro_crew.transcribe and its optional heavy deps; faster-whisper is not a declared extra) must never be imported at gateway startup, so route modules import it function-locally. Pinned by TestWiring.test_transcribe_is_imported_lazily and documented at the import site. This ruling covers every function-local import of the STT stack in this PR, wherever it moves.

@NicholasRBowers

Copy link
Copy Markdown
Contributor

Disposition: rebutted — span=eb8103c2acd8

Same recorded top-level-imports deviation inside the STT stack itself: numpy is typing-only at module level and imported at the fold site only when a decode actually runs, and provider modules (boto3, amazon_transcribe, apple_speech) load per-call for the same reason. Non-STT helpers (pinned_fs, sandbox) were already hoisted to module scope in an earlier round; the remaining function-local imports are exactly the optional-dependency class this ruling covers.

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

Labels

allow-fork-workflow-change fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants