feat: implement policy-constrained YouTube import (#30) - #90
Merged
Conversation
- Add `yt-dlp` to Python engine dependencies and inventory - Implement robust audio extraction logic enforcing constraints (audio only, 50MB limit, 15 min limit) - Expose `import_youtube_url` Tauri IPC command - Add UI inputs for YouTube URL import and loading state - Ensure 100% test coverage
Contributor
|
Caution Review failedThe pull request is closed. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (11)
Cache: Disabled due to Reviews > Disable Cache setting Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit릴리스 노트
WalkthroughYouTube URL을 검증하고 yt-dlp 기반 Python 엔진을 서브프로세스로 실행해 오디오를 다운로드한 뒤 per‑import 프로젝트/캐시/임시 루트를 생성해 LocalAudio 기반 ProjectBootstrapSummaryPayload를 반환·저장하는 엔드투엔드 유입 경로를 추가했습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "사용자 (UI)"
participant App as "App.tsx"
participant Lib as "analysis.ts"
participant Tauri as "Tauri (main.rs)"
participant Engine as "Python 엔진 (youtube.py)"
User->>App: URL 입력 및 임포트 클릭
App->>Lib: importYoutubeUrl(url)
Lib->>Tauri: invoke("import_youtube_url",{url})
Tauri->>Tauri: URL 검증 및 per‑import 루트 생성
Tauri->>Engine: spawn subprocess (bandscope_analysis.youtube --url --out-dir) within timeout
Engine->>Engine: validate_url(), yt-dlp 메타조회/다운로드, 길이·크기 검사
Engine-->>Tauri: JSON 결과(stdout) (ok / error)
Tauri->>Tauri: stdout 파싱, 파일 메타확인, LocalAudio 생성·저장
Tauri-->>Lib: ProjectBootstrapSummaryPayload 또는 오류 문자열
Lib-->>App: 성공/실패 응답
App-->>User: 소스 렌더링 또는 에러 표시
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/main.rs`:
- Around line 767-771: The blocking Command::new(...).output() call inside
start_analysis_job blocks the Tauri event loop and lacks a timeout; replace this
synchronous .output() usage with non-blocking handling (either make
start_analysis_job async with #[tauri::command(async)] and await a tokio/async
process, or spawn a dedicated thread via thread::spawn and use channels to
notify the main thread) and enforce a timeout similar to
ANALYSIS_PROCESS_TIMEOUT; specifically, change the code path that currently
invokes Command::new(program).args(args).current_dir(working_dir).output() to
spawn the child process (use Command::spawn / stdout/stderr piping or an async
process API), poll or await its completion with a deadline, kill the child on
timeout, and return an appropriate error message so run_analysis_engine and
callers are not blocked indefinitely.
- Around line 742-744: The URL check is vulnerable because it only inspects
substrings of the input (the variable url) which allows hosts like
"evil.com/youtube.com/..." to pass; replace this heuristic in the validation
(the block using url.starts_with and url.contains) with proper URL parsing
(e.g., using url::Url::parse) and then validate the scheme is "https" and the
host is exactly one of the allowed hosts ("youtube.com", "www.youtube.com",
"youtu.be", and any other official YouTube host you accept), plus any
path-specific checks you need for youtu.be short links; ensure you check
host_str() equality rather than substring containment so malicious domains
cannot bypass the check.
In `@apps/desktop/src/App.tsx`:
- Around line 167-184: Replace hardcoded UI strings in the YouTube input/button
block with i18n keys using the existing translation function (e.g., t).
Specifically update the placeholder "YouTube URL...", the button label when
isImporting ("Importing...") and the default button label ("Import YouTube") to
use t('...') keys; keep the existing props and logic around youtubeUrl,
setYoutubeUrl, handleImportYoutube, isImporting, analysisInFlight and isStarting
intact. Add corresponding translation keys to the locale files and ensure the
component imports/uses the translation hook/provider already used elsewhere in
the app.
In `@services/analysis-engine/src/bandscope_analysis/youtube.py`:
- Around line 26-30: The current string containment checks allow host-poisoning
like "https://evil.com/youtube.com/"; update the URL validation to parse the URL
(e.g., with urllib.parse.urlparse) instead of using substring checks: ensure
parsed.scheme == "https", extract and normalize parsed.netloc (lowercase, strip
optional port), then validate the host is exactly "youtu.be" or "youtube.com" or
endswith ".youtube.com"; return False for anything else. Replace the existing
substring checks around the url variable with this host-based validation so the
function (the block using url.startswith and the "youtube.com"/"youtu.be"
checks) only accepts legitimate YouTube hosts.
- Around line 53-78: The code currently downloads audio via yt_dlp (see ydl_opts
and the with yt_dlp.YoutubeDL(...) block, variable info and actual_filepath) but
does not enforce the PR-specified limits; add checks after info/actual_filepath
are available: validate info.get("duration") <= 15*60 seconds and check the
downloaded file size via os.path.getsize(actual_filepath) <= 50*1024*1024 bytes,
and if either limit is exceeded delete the downloaded file and return a failure
result (e.g., {"ok": False, "reason": "duration_exceeded" / "size_exceeded"});
ensure these checks are applied before returning the success metadata from the
function.
In `@services/analysis-engine/tests/test_youtube.py`:
- Around line 108-126: The test may import a cached bandscope_analysis.youtube
before patches are applied; update test_main_block to ensure the module is
reloaded after setting patches by using importlib.reload on the
bandscope_analysis.youtube module (after patch.object(sys, "exit") and patching
download_youtube_audio) so the patched objects are used by
bandscope_analysis.youtube.main; specifically, reference test_main_block, the
mock_download patch for download_youtube_audio, the patch.object(sys, "exit")
and the bandscope_analysis.youtube.main invocation and call
importlib.reload(bandscope_analysis.youtube) before calling main(), and repeat
reload before the failure case to guarantee test isolation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06395d4d-ae6f-42b2-bbae-a5c3a720ed90
⛔ Files ignored due to path filters (1)
services/analysis-engine/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
apps/desktop/src-tauri/src/main.rsapps/desktop/src/App.test.tsxapps/desktop/src/App.tsxapps/desktop/src/lib/analysis.tsservices/analysis-engine/pyproject.tomlservices/analysis-engine/src/bandscope_analysis/youtube.pyservices/analysis-engine/tests/test_youtube.pysupply-chain/supplemental-component-inventory.json
- Add url crate for robust URL validation in Tauri. - Wrap synchronous CLI execution in spawn_blocking in Tauri. - Refactor YouTube URL validation in Python engine to limit URL length and handle unknown exceptions. - Ensure 100% test coverage for YouTube module. - Fix hardcoded UI strings by migrating to i18n locale files. - Resolve Ruff line-length formatting issues.
Collaborator
Author
|
@coderabbitai review I have addressed the review comments. |
seonghobae
enabled auto-merge (squash)
March 26, 2026 02:29
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
services/analysis-engine/src/bandscope_analysis/youtube.py (1)
68-84: 🧹 Nitpick | 🔵 Trivial다운로드 전 duration 검사로 대역폭 낭비를 방지할 수 있습니다.
현재 구현은
download=True로 먼저 다운로드한 후 duration을 검사합니다. yt-dlp는download=False로 메타데이터만 먼저 추출할 수 있어, 15분 초과 영상의 불필요한 다운로드를 방지할 수 있습니다.♻️ 메타데이터 사전 검사 제안
try: with yt_dlp.YoutubeDL(ydl_opts) as ydl: - info = ydl.extract_info(url, download=True) + # First extract metadata without downloading + info = ydl.extract_info(url, download=False) if info is None: raise Exception("Failed to extract info") - actual_filepath = ydl.prepare_filename(info) duration = info.get("duration") if duration is not None and duration > 15 * 60: - if os.path.exists(actual_filepath): - os.remove(actual_filepath) return { "ok": False, "error": { "code": "duration_exceeded", "message": "Video exceeds the 15-minute limit.", }, } + + # Now download after validation + info = ydl.extract_info(url, download=True) + if info is None: + raise Exception("Failed to extract info") + actual_filepath = ydl.prepare_filename(info) if ( os.path.exists(actual_filepath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/analysis-engine/src/bandscope_analysis/youtube.py` around lines 68 - 84, The current code calls ydl.extract_info(url, download=True) which downloads before checking duration; change to first call ydl.extract_info(url, download=False) (using the existing ydl_opts and ydl/YoutubeDL context) to obtain info and check info.get("duration") against 15*60, and only if within limit perform the actual download (e.g., call ydl.download([url]) or re-run extract_info with download=True) to avoid wasted bandwidth; preserve the existing handling of actual_filepath (derived from ydl.prepare_filename(info)), removal on duration exceedance, and existing error/return shapes.apps/desktop/src-tauri/src/main.rs (1)
775-783:⚠️ Potential issue | 🟠 MajorYouTube 다운로드에 타임아웃이 없습니다.
run_analysis_engine은ANALYSIS_PROCESS_TIMEOUT(30초)을 사용하지만, YouTube 다운로드는 네트워크 상태에 따라 훨씬 오래 걸릴 수 있습니다. 현재 구현은 무한 대기할 수 있어 리소스가 고갈될 위험이 있습니다.🛡️ 타임아웃 추가 제안
+const YOUTUBE_IMPORT_TIMEOUT: Duration = Duration::from_secs(120); // 2분 + #[tauri::command] async fn import_youtube_url( // ... ) -> Result<ProjectBootstrapSummaryPayload, String> { // ... validation code ... - let output = tauri::async_runtime::spawn_blocking(move || { + let output = tokio::time::timeout( + YOUTUBE_IMPORT_TIMEOUT, + tauri::async_runtime::spawn_blocking(move || { Command::new(program) .args(args) .current_dir(working_dir) .output() - }) + }) + ) .await + .map_err(|_| "YouTube import timed out. Please try again or use a local audio file.".to_string())? .map_err(|_| "Failed to execute YouTube import process.".to_string())? .map_err(|_| "Failed to start YouTube import process.".to_string())?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/main.rs` around lines 775 - 783, The YouTube download command spawned in run_analysis_engine currently blocks indefinitely (the closure with Command::new(program).args(args).current_dir(working_dir).output()), so wrap the process execution with a timeout using ANALYSIS_PROCESS_TIMEOUT: convert the blocking .output() invocation into a cancellable flow (use tokio::time::timeout(Duration::from_secs(ANALYSIS_PROCESS_TIMEOUT), ...) or spawn the child and await it with timeout), and on timeout ensure you kill the spawned child process and return a clear timeout error (map_err) instead of hanging; update the error mapping that currently follows the tauri::async_runtime::spawn_blocking call to handle the timeout case and include proper cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/main.rs`:
- Around line 797-802: The file_name is built directly from title via
format!("{}.{}", title, extension) which allows directory traversal and invalid
characters; update the code that constructs LocalAudioSourcePayload
(specifically the file_name field creation) to sanitize the title: remove or
replace path separators (/, \), dots that form parent references, and other
filesystem-invalid/control characters, truncate to a safe max length, and fall
back to a deterministic safe name if empty; then compose file_name from the
sanitized title + extension and use that value in LocalAudioSourcePayload so
source_path and file_name cannot be manipulated by unsafe title content.
In `@services/analysis-engine/tests/test_youtube.py`:
- Around line 13-20: Add test cases in test_validate_url to cover YouTube
subdomains: call validate_url with URLs like "https://m.youtube.com/watch?v=123"
and "https://music.youtube.com/watch?v=123" and assert they return True (and
optionally a non-YouTube subdomain like "https://evil.youtube.com/watch?v=123"
if your validation should reject unknown subdomains); update the test function
test_validate_url to include these assertions so the validate_url behavior for
*.youtube.com subdomains is covered.
---
Duplicate comments:
In `@apps/desktop/src-tauri/src/main.rs`:
- Around line 775-783: The YouTube download command spawned in
run_analysis_engine currently blocks indefinitely (the closure with
Command::new(program).args(args).current_dir(working_dir).output()), so wrap the
process execution with a timeout using ANALYSIS_PROCESS_TIMEOUT: convert the
blocking .output() invocation into a cancellable flow (use
tokio::time::timeout(Duration::from_secs(ANALYSIS_PROCESS_TIMEOUT), ...) or
spawn the child and await it with timeout), and on timeout ensure you kill the
spawned child process and return a clear timeout error (map_err) instead of
hanging; update the error mapping that currently follows the
tauri::async_runtime::spawn_blocking call to handle the timeout case and include
proper cleanup.
In `@services/analysis-engine/src/bandscope_analysis/youtube.py`:
- Around line 68-84: The current code calls ydl.extract_info(url, download=True)
which downloads before checking duration; change to first call
ydl.extract_info(url, download=False) (using the existing ydl_opts and
ydl/YoutubeDL context) to obtain info and check info.get("duration") against
15*60, and only if within limit perform the actual download (e.g., call
ydl.download([url]) or re-run extract_info with download=True) to avoid wasted
bandwidth; preserve the existing handling of actual_filepath (derived from
ydl.prepare_filename(info)), removal on duration exceedance, and existing
error/return shapes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d8f52d7-78f9-4d3b-a5a8-d628c2a16817
⛔ Files ignored due to path filters (1)
apps/desktop/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
apps/desktop/src-tauri/Cargo.tomlapps/desktop/src-tauri/src/main.rsapps/desktop/src/App.tsxapps/desktop/src/locales/en/common.jsonapps/desktop/src/locales/ko/common.jsonservices/analysis-engine/src/bandscope_analysis/youtube.pyservices/analysis-engine/tests/test_youtube.py
Adds 120s timeout and filename sanitization to rust main.rs. Fixes youtube.py duration checking by using two-pass extract_info logic. Adds missing test cases for 100% test coverage.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/main.rs (1)
803-812:⚠️ Potential issue | 🟡 Minor파일명 새니타이징이 Windows 불법 문자를 완전히 처리하지 않습니다.
현재 구현은 control chars,
/,\,.만 필터링합니다. Windows에서 불법인:,*,?,",<,>,|문자가 누락되어 Windows 환경에서 파일 생성 오류가 발생할 수 있습니다.🛡️ Windows 호환 새니타이징 제안
let safe_title: String = title .chars() - .filter(|c| !c.is_control() && *c != '/' && *c != '\\' && *c != '.') + .map(|c| match c { + '/' | '\\' | ':' | '*' | '?' | '"' | '<' | '>' | '|' => '_', + '.' => '_', // prevent path-like patterns + c if c.is_control() => '_', + c => c, + }) .take(100) .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/main.rs` around lines 803 - 812, The filename sanitization building safe_title currently only filters control chars plus '/', '\', and '.'; update the sanitization logic that creates safe_title (the block using title.chars().filter(...).take(100).collect()) to also remove the Windows-reserved characters ':', '*', '?', '"', '<', '>', and '|' (or replace them with a safe character like '_'), and keep the existing max-length and empty-string fallback ("youtube_audio") behavior; ensure the variable name safe_title and the same take(100) truncation are preserved so callers remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/analysis-engine/src/bandscope_analysis/youtube.py`:
- Around line 88-99: The current branch only handles the case where
actual_filepath exists and is too large but does not handle the case where the
file is missing after download; update the post-download logic in the function
that writes/returns actual_filepath (look for the block using actual_filepath
and os.path.getsize) to explicitly check that os.path.exists(actual_filepath) is
True before returning success and, if not, return a failure dict (e.g., code
"missing_file" with a clear message) so the caller/Rust side (which uses
std::fs::metadata) sees a consistent error; ensure this defensive existence
check occurs prior to any size checks or successful return.
- Around line 57-66: ydl_opts currently contains the unsupported key
"extract_audio": True; replace this with a "postprocessors" entry that uses the
FFmpegExtractAudio postprocessor (configure "key": "FFmpegExtractAudio", and set
"preferredcodec" and "preferredquality") so yt_dlp's Python API will perform
audio extraction; update the ydl_opts dictionary used in download logic
(reference: ydl_opts) and ensure ffmpeg is available at runtime.
In `@services/analysis-engine/tests/test_youtube.py`:
- Around line 31-51: The test test_download_youtube_audio_success currently
doesn't mock filesystem checks so the file-size branch inside
download_youtube_audio isn't exercised; update the test to patch os.path.exists
and os.path.getsize in the module where download_youtube_audio is defined (e.g.,
patch "bandscope_analysis.youtube.os.path.exists" and
"bandscope_analysis.youtube.os.path.getsize") to return True and a size under
the max threshold (e.g., <50MB) respectively, so the code path that checks file
existence and size is executed and the metadata["filepath"]/size logic is
validated.
---
Duplicate comments:
In `@apps/desktop/src-tauri/src/main.rs`:
- Around line 803-812: The filename sanitization building safe_title currently
only filters control chars plus '/', '\', and '.'; update the sanitization logic
that creates safe_title (the block using
title.chars().filter(...).take(100).collect()) to also remove the
Windows-reserved characters ':', '*', '?', '"', '<', '>', and '|' (or replace
them with a safe character like '_'), and keep the existing max-length and
empty-string fallback ("youtube_audio") behavior; ensure the variable name
safe_title and the same take(100) truncation are preserved so callers remain
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d51ba850-3ce2-434b-a4db-6f972111beab
📒 Files selected for processing (3)
apps/desktop/src-tauri/src/main.rsservices/analysis-engine/src/bandscope_analysis/youtube.pyservices/analysis-engine/tests/test_youtube.py
- Replace extract_audio with postprocessors.FFmpegExtractAudio - Add fallback extension checking when yt-dlp transcodes audio - Add 100% test coverage for file not found and extension changes
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/analysis-engine/tests/test_youtube.py`:
- Around line 177-194: The test test_download_youtube_audio_duration_exceeded
patches three filesystem-related mocks (mock_remove, mock_exists, mock_getsize)
that are never used because download_youtube_audio rejects by duration before
touching the filesystem; remove the unused `@patch` decorators and the
corresponding parameters from the test signature so only
`@patch`("bandscope_analysis.youtube.yt_dlp.YoutubeDL") remains and the test
function only accepts mock_ydl_class (and keep the existing setup of
mock_ydl_class.return_value.__enter__.return_value and extract_info return
value).
- Around line 243-268: The test_module_execution test patches os.path on the
bandscope_analysis.youtube module which won't affect runpy.run_path because
runpy creates a fresh module namespace; instead, mock the os module at the
sys.modules level or patch the global os.path functions: replace the current
patch("bandscope_analysis.youtube.os.path.exists") /
patch("bandscope_analysis.youtube.os.path.getsize") approach by either inserting
a mock_os into sys.modules via monkeypatch.setitem(sys.modules, "os", mock_os)
(ensuring mock_os.path.exists and mock_os.path.getsize return the desired
values) or patching the global functions with patch("os.path.exists") and
patch("os.path.getsize") before calling runpy.run_path in test_module_execution
so the run-time import inside runpy sees the mocked functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0f76bba-a1dc-41b8-aead-c8d69cc30581
📒 Files selected for processing (2)
services/analysis-engine/src/bandscope_analysis/youtube.pyservices/analysis-engine/tests/test_youtube.py
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/main.rs`:
- Around line 795-836: When parsed.get("ok") is Some(true) but metadata is
missing, return a clear, specific error instead of falling through; update the
block that checks parsed.get("ok") and then attempts to unwrap metadata (the
branch that currently looks up metadata, filepath, title, builds
LocalAudioSourcePayload and ProjectBootstrapSummaryPayload and calls
store_bootstrap_source) to detect metadata.is_none() and immediately return
Err(...) with a descriptive message like "YouTube import reported ok but missing
metadata" (include any available context such as parsed value), so callers don’t
hit the generic "YouTube import failed with an unknown error." later.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78001590-9f88-452f-bddc-c121b0a2de17
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/main.rsservices/analysis-engine/tests/test_youtube.py
…ing and add tokio time feature
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apps/desktop/src-tauri/src/main.rs (2)
776-789:⚠️ Potential issue | 🟠 Major타임아웃이 자식 프로세스를 실제로 멈추지 않습니다.
Line 776의
timeout(...)은JoinHandle대기만 끊습니다. 이미 시작된spawn_blocking내부의Command::output()는 계속 돌기 때문에, 호출자는 타임아웃을 받았어도yt-dlp다운로드와 파일 쓰기가 백그라운드에 남습니다.Child를 직접spawn()해서 deadline 초과 시kill()/wait()까지 보장해야 합니다.⏱️ 제안
- let spawn_result = tokio::time::timeout( - YOUTUBE_IMPORT_TIMEOUT, - tauri::async_runtime::spawn_blocking(move || { - Command::new(program) - .args(args) - .current_dir(working_dir) - .output() - }) - ).await; - - let output = spawn_result - .map_err(|_| "YouTube import timed out.".to_string())? - .map_err(|_| "Failed to execute YouTube import process.".to_string())? - .map_err(|_| "Failed to start YouTube import process.".to_string())?; + let mut child = Command::new(program) + .args(args) + .current_dir(working_dir) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .map_err(|_| "Failed to start YouTube import process.".to_string())?; + + let deadline = Instant::now() + YOUTUBE_IMPORT_TIMEOUT; + loop { + match child.try_wait() { + Ok(Some(_)) => break, + Ok(None) if Instant::now() >= deadline => { + let _ = child.kill(); + let _ = child.wait(); + return Err("YouTube import timed out.".to_string()); + } + Ok(None) => tokio::time::sleep(ANALYSIS_WAIT_POLL).await, + Err(_) => { + let _ = child.kill(); + let _ = child.wait(); + return Err("Failed to execute YouTube import process.".to_string()); + } + } + } + + let output = child + .wait_with_output() + .map_err(|_| "Failed to execute YouTube import process.".to_string())?;Does `tokio::time::timeout` cancel an already-started `spawn_blocking` task, and if not, does a `std::process::Command::output()` child process continue running after the timeout future resolves?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/main.rs` around lines 776 - 789, The timeout currently wraps tauri::async_runtime::spawn_blocking(...) which only cancels waiting on the JoinHandle while the underlying Command::output() child keeps running; change the logic in the YOUTUBE import sequence to spawn the child process with Command::new(program).args(args).current_dir(working_dir).spawn() so you obtain a std::process::Child, then await the child's completion with tokio::time::timeout(YOUTUBE_IMPORT_TIMEOUT, async { child.wait_with_output() }) (or use a blocking-to-async bridge), and on timeout call child.kill() and child.wait() to ensure the process is terminated and reaped; remove the current use of spawn_blocking + Command::output() and update the error handling around the former spawn_result/Command::... calls accordingly.
743-753:⚠️ Potential issue | 🟠 Major호스트만 검사하면 정책 allowlist가 너무 넓습니다.
Line 751은
https://www.youtube.com/,/feed/history,/account처럼 실제 import 대상이 아닌 페이지도 모두 통과시킵니다. 이 경계에서는 host뿐 아니라 지원하는 path/query 조합(watch?v=...,youtu.be/<id>, 필요 시shorts/live)까지 명시적으로 좁혀서 fail-fast 해야 합니다.🛡️ 제안
- if host != "youtu.be" && host != "youtube.com" && !host.ends_with(".youtube.com") { + let supported = match host.as_str() { + "youtu.be" => parsed_url + .path_segments() + .and_then(|mut segments| segments.next()) + .is_some_and(|id| !id.is_empty()), + "youtube.com" | "www.youtube.com" | "m.youtube.com" | "music.youtube.com" => { + match parsed_url.path() { + "/watch" => parsed_url.query_pairs().any(|(k, v)| k == "v" && !v.is_empty()), + path if path.starts_with("/shorts/") || path.starts_with("/live/") => true, + _ => false, + } + } + _ => false, + }; + if !supported { return Err("Only standard YouTube URLs are supported.".to_string()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/main.rs` around lines 743 - 753, The current URL validation (using parsed_url, scheme, host) allows many non-video YouTube pages; tighten validation by explicitly allowing only supported path/query patterns: for youtube.com require path "/watch" with a non-empty "v" query parameter, or paths starting with "/shorts/" or "/live" as needed; for youtu.be require a single-segment path (the video id). Implement these checks after parsed_url and host are validated (using parsed_url.path(), parsed_url.path_segments(), and parsed_url.query_pairs()) and return the same Err("Only standard YouTube URLs are supported.".to_string()) for any unsupported path/query combinations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/main.rs`:
- Around line 795-823: The code trusts the raw stdout "filepath" and stores it
into LocalAudioSourcePayload.source_path, which allows referencing files outside
the intended workspace; fix by canonicalizing the reported filepath (use
Path::new(filepath).canonicalize()) and canonicalizing the application's
expected download root, then verify the canonicalized file path starts_with the
canonicalized download root (reject with an error if not). Use the canonicalized
path for subsequent std::fs::metadata calls and set
LocalAudioSourcePayload.source_path to the canonicalized string; keep
extension/safe_title logic unchanged but ensure you return an error instead of
accepting paths outside the confined root.
In `@services/analysis-engine/tests/test_youtube.py`:
- Around line 213-232: The test test_main_block in
services/analysis-engine/tests/test_youtube.py only asserts exit codes and
ignores main()'s stdout JSON contract; update the test to capture stdout via
capsys after calling bandscope_analysis.youtube.main(), parse the printed JSON
(e.g., json.loads(capsys.readouterr().out)), and assert the returned
structure/fields when mock_download returns {"ok": True, "metadata": {"id":
"123"}} and when it returns {"ok": False} (assert expected error shape); apply
the same stdout-capturing and JSON-parsing assertions to the other failing test
block referenced (lines ~235-260) so both tests validate printed JSON as well as
exit codes and reference the bandscope_analysis.youtube.main and
download_youtube_audio mocks when making assertions.
- Around line 13-22: Add negative test cases to ensure validate_url rejects
non-video YouTube paths: update test_validate_url to assert False for URLs like
"https://www.youtube.com/", "https://www.youtube.com/feed/history",
"https://www.youtube.com/watch" (no v param), and any other YouTube URLs lacking
a video id; ensure the validate_url implementation enforces allowed path/query
patterns (e.g., requires a valid "v" query or a youtu.be path segment) rather
than relying only on hostname checks so these cases fail as expected.
---
Duplicate comments:
In `@apps/desktop/src-tauri/src/main.rs`:
- Around line 776-789: The timeout currently wraps
tauri::async_runtime::spawn_blocking(...) which only cancels waiting on the
JoinHandle while the underlying Command::output() child keeps running; change
the logic in the YOUTUBE import sequence to spawn the child process with
Command::new(program).args(args).current_dir(working_dir).spawn() so you obtain
a std::process::Child, then await the child's completion with
tokio::time::timeout(YOUTUBE_IMPORT_TIMEOUT, async { child.wait_with_output() })
(or use a blocking-to-async bridge), and on timeout call child.kill() and
child.wait() to ensure the process is terminated and reaped; remove the current
use of spawn_blocking + Command::output() and update the error handling around
the former spawn_result/Command::... calls accordingly.
- Around line 743-753: The current URL validation (using parsed_url, scheme,
host) allows many non-video YouTube pages; tighten validation by explicitly
allowing only supported path/query patterns: for youtube.com require path
"/watch" with a non-empty "v" query parameter, or paths starting with "/shorts/"
or "/live" as needed; for youtu.be require a single-segment path (the video id).
Implement these checks after parsed_url and host are validated (using
parsed_url.path(), parsed_url.path_segments(), and parsed_url.query_pairs()) and
return the same Err("Only standard YouTube URLs are supported.".to_string()) for
any unsupported path/query combinations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c1bc002-3b94-4e32-883a-5315e7718b85
⛔ Files ignored due to path filters (1)
apps/desktop/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
apps/desktop/src-tauri/Cargo.tomlapps/desktop/src-tauri/src/main.rsservices/analysis-engine/tests/test_youtube.py
…ube-import # Conflicts: # apps/desktop/src-tauri/src/main.rs # apps/desktop/src/App.test.tsx # apps/desktop/src/App.tsx # apps/desktop/src/lib/analysis.ts
Collaborator
Author
|
@coderabbitai review I have addressed the review comments. |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Collaborator
Author
|
@coderabbitai resolve |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
1 similar comment
Contributor
✅ Actions performedReview triggered.
|
Contributor
✅ Actions performedFull review triggered. |
Contributor
✅ Actions performedComments resolved and changes approved. |
Contributor
✅ Actions performedComments resolved and changes approved. |
Contributor
✅ Actions performedFull review triggered. |
This was referenced Apr 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #30
Summary
yt-dlpto Python engine dependencies and recorded it in the supplemental component inventory to comply with supply chain policies.youtube.pywith strict policy enforcement:import_youtube_urlvia Tauri IPC in Rust backend.Security Notes
yt-dlpwhich securely fetches the metadata and stream.yt-dlpis used as a library internally rather than a spawned subprocess if possible, or tightly controlled.📝 Walkthrough
Walkthrough
YouTube URL을 검증하고 yt-dlp로 오디오를 내려받아 프로젝트별 워크스페이스에 부트스트랩 페이로드를 생성·저장하는 전체 흐름(데스크톱 UI → 분석 래퍼 → Tauri 명령 → Python 엔진)을 추가했습니다.
Changes
apps/desktop/src-tauri/src/main.rsimport_youtube_url추가: HTTPS + 허용 호스트 검증, per-import 프로젝트/캐시/임시 루트 생성, 분석 엔진 서브프로세스 실행(120s 타임아웃), stdout JSON 파싱, 파일 메타 검사 후ProjectBootstrapSummaryPayload생성·저장, 에러 경로 처리 및 핸들러 등록.apps/desktop/src/App.tsx,apps/desktop/src/App.test.tsxyoutubeUrl,isImporting) 추가, 버튼 비활성화/레이블 업데이트, 성공·실패·거부 케이스 테스트 및 Tauri 모킹 확장.apps/desktop/src/lib/analysis.tsimportYoutubeUrl(url: string)공개 함수 추가: Tauri 호출 래핑, 성공 시 bootstrap 파싱 반환, 실패 시 표준화된 오류 객체 반환.services/analysis-engine/src/bandscope_analysis/youtube.py,services/analysis-engine/pyproject.tomlyoutube.py추가:validate_url,download_youtube_audio, CLImain()구현. yt-dlp 의존성(yt-dlp>=2026.3.17) 추가. 사전/사후 제약(재생시간 15분, 크기 50MB), 오류 코드 매핑, JSON 출력 및 종료 코드 처리.services/analysis-engine/tests/test_youtube.pyvalidate_url와download_youtube_audio의 다양한 성공/실패/제약 케이스(기간·크기 초과, 다운로드 오류, 파일 미발견 등) 및 CLI 흐름을 모킹해 검증하는 pytest 스위트 추가.supply-chain/supplemental-component-inventory.json,apps/desktop/src-tauri/Cargo.tomlyt-dlp번들 메타데이터 추가(버전 제약·소스·라이선스 등) 및 Tauri쪽 Rust 의존성(tokio+timefeature,url) 추가.apps/desktop/src/locales/en/common.json,apps/desktop/src/locales/ko/common.jsonSequence Diagram(s)
sequenceDiagram participant User as "사용자 (UI)" participant App as "App.tsx" participant AnalysisLib as "analysis.ts" participant TauriCmd as "Tauri main.rs" participant Engine as "Python 엔진 (youtube.py)" User->>App: URL 입력 및 임포트 클릭 App->>AnalysisLib: importYoutubeUrl(url) AnalysisLib->>TauriCmd: invoke("import_youtube_url", { url }) TauriCmd->>TauriCmd: URL 검증, per-import 루트 생성 TauriCmd->>Engine: spawn (--module bandscope_analysis.youtube --url --out-dir) (with timeout) Engine->>Engine: validate_url(), yt-dlp 메타조회/다운로드, 길이/크기 검사 Engine-->>TauriCmd: JSON 결과(stdout) (ok / error) TauriCmd->>TauriCmd: stdout 파싱, 파일 메타 확인, LocalAudio 소스 생성·저장 TauriCmd-->>AnalysisLib: ProjectBootstrapSummaryPayload 또는 오류 문자열 AnalysisLib-->>App: 성공/실패 응답 App-->>User: 소스 렌더링 또는 에러 표시Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem