Skip to content

fix(desktop): fix yt-dlp PATH resolution and surface transcribe errors - #10

Merged
seungwonme merged 1 commit into
mainfrom
worktree-fix-transcribe-path
Jul 4, 2026
Merged

fix(desktop): fix yt-dlp PATH resolution and surface transcribe errors#10
seungwonme merged 1 commit into
mainfrom
worktree-fix-transcribe-path

Conversation

@seungwonme

Copy link
Copy Markdown
Owner

문제

Skim.app을 Finder//Applications에서 실행하면(터미널이 아니라 LaunchServices로 띄우면) 프로세스가 셸 프로필(PATH)을 물려받지 않는다. youtube-transcribe가 내부적으로 subprocess.run(["yt-dlp", ...])를 bare 명령어로 호출하는데, yt-dlp~/.local/bin에 있어 기본 LaunchServices PATH(/usr/bin:/bin:/usr/sbin:/sbin)에 없어 실행이 실패했다.

거기에 더해 실패 시 에러가 sourceMessage에 담기는데, 이 값은 "소스/크레덴셜 관리" 시트 안에서만 렌더링돼 리더 화면에서는 아무 반응도 안 보였다 - 버튼이 hourglass로 바뀌었다가 조용히 원상복구되어 "동작하는 척 하다가 안 됨"처럼 보였음.

수정

  • runSkimProcess.environment/opt/homebrew/bin, /usr/local/bin, ~/.local/bin을 PATH 앞에 추가해 GUI 실행 여부와 무관하게 yt-dlp/ffmpeg 등을 찾도록 함
  • 전사 실패 메시지를 sourceMessage 대신 별도 transcribeError 상태로 분리해 리더 상단바에 빨간 텍스트로 직접 표시(텍스트 선택 가능 - 사실상 앱 로그 역할)

테스트

  • swift build --package-path apps/desktop 성공
  • 기존 Python 테스트 214개, Swift 유닛 테스트 14개 통과 (pre-commit hook)

Finder-launched (LaunchServices) app processes don't inherit the
shell PATH, so yt-dlp/ffmpeg in ~/.local/bin or Homebrew were never
found and youtube-transcribe failed silently. Also, transcribe
failures were written to sourceMessage, which only renders inside
the source manager sheet, so the reader pane showed nothing.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces error handling for YouTube transcription failures by displaying errors directly in the reader top bar, and configures the environment PATH for the external skim process to ensure tools like yt-dlp and ffmpeg are correctly located. The review feedback suggests isolating transcription errors per post using a dictionary to avoid UX bugs when switching posts, and addresses a potential security vulnerability (CWE-426) regarding empty PATH components alongside a fallback mechanism for executing uv safely.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@State private var channelBusyMessage: String?
@State private var exhaustedChannels: Set<String> = []
@State private var transcribingPostID: DashboardPost.ID?
@State private var transcribeError: String?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

현재 transcribeError는 단일 글로벌 @State 변수로 선언되어 있습니다. 이 경우 특정 포스트의 전사가 실패한 후 다른 포스트를 선택하더라도 이전 포스트의 에러 메시지가 상단바에 계속 표시되는 UX 버그가 발생합니다.

transcribeError[DashboardPost.ID: String] 딕셔너리 형태로 변경하여 에러 메시지를 각 포스트별로 격리하는 것을 권장합니다.

Suggested change
@State private var transcribeError: String?
@State private var transcribeError: [DashboardPost.ID: String] = [:]

Comment on lines +451 to +457
if let transcribeError {
Text("전사 실패: \(transcribeError)")
.font(.caption)
.foregroundStyle(Color.red)
.textSelection(.enabled)
.lineLimit(3)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

transcribeError가 딕셔너리로 변경됨에 따라, 현재 선택된 포스트(post.id)에 해당하는 에러 메시지만 표시하도록 수정합니다.

Suggested change
if let transcribeError {
Text("전사 실패: \(transcribeError)")
.font(.caption)
.foregroundStyle(Color.red)
.textSelection(.enabled)
.lineLimit(3)
}
if let error = transcribeError[post.id] {
Text("전사 실패: \(error)")
.font(.caption)
.foregroundStyle(Color.red)
.textSelection(.enabled)
.lineLimit(3)
}

Comment on lines +1271 to 1279
transcribeError = nil
Task { @MainActor in
do {
_ = try await runSkim(["youtube-transcribe", url.absoluteString])
reloadChannelPosts()
loadDashboard()
} catch {
sourceMessage = Notice(text: localizedError(error), isError: true)
transcribeError = localizedError(error)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

전사 시작 시 해당 포스트의 에러를 초기화하고, 실패 시 해당 포스트 ID를 키로 하여 에러 메시지를 저장하도록 수정합니다.

Suggested change
transcribeError = nil
Task { @MainActor in
do {
_ = try await runSkim(["youtube-transcribe", url.absoluteString])
reloadChannelPosts()
loadDashboard()
} catch {
sourceMessage = Notice(text: localizedError(error), isError: true)
transcribeError = localizedError(error)
}
transcribeError[post.id] = nil
Task { @MainActor in
do {
_ = try await runSkim(["youtube-transcribe", url.absoluteString])
reloadChannelPosts()
loadDashboard()
} catch {
transcribeError[post.id] = localizedError(error)
}

Comment on lines +1301 to +1305
var environment = ProcessInfo.processInfo.environment
let extraPaths = ["/opt/homebrew/bin", "/usr/local/bin", "\(NSHomeDirectory())/.local/bin"]
let existingPath = environment["PATH"] ?? "/usr/bin:/bin:/usr/sbin:/sbin"
environment["PATH"] = (extraPaths + [existingPath]).joined(separator: ":")
process.environment = environment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-high high

여기에는 두 가지 중요한 개선 사항이 있습니다:

  1. 빈 PATH 컴포넌트 취약점 (CWE-426): 만약 environment["PATH"]가 빈 문자열("")인 경우, existingPath""가 되어 최종 PATH에 빈 컴포넌트(예: 끝에 붙는 콜론 :)가 포함됩니다. Unix 계열 시스템에서 PATH 내의 빈 컴포넌트는 현재 작업 디렉토리(.)를 의미하므로, 의도치 않게 현재 디렉토리의 바이너리가 실행될 수 있는 보안 취약점이 존재합니다. 따라서 빈 문자열을 필터링해야 합니다.
  2. 상대 경로 실행 실패 방지: 만약 uv가 지정된 후보 경로에 존재하지 않아 "uv"로 폴백되는 경우, ProcessexecutableURL에 절대 경로만 허용하므로 process.run() 실행 시 즉시 실패하게 됩니다. 이를 방지하기 위해 /usr/bin/env를 사용하여 새로 구성된 PATH에서 uv를 동적으로 찾아서 실행하도록 개선합니다.
Suggested change
var environment = ProcessInfo.processInfo.environment
let extraPaths = ["/opt/homebrew/bin", "/usr/local/bin", "\(NSHomeDirectory())/.local/bin"]
let existingPath = environment["PATH"] ?? "/usr/bin:/bin:/usr/sbin:/sbin"
environment["PATH"] = (extraPaths + [existingPath]).joined(separator: ":")
process.environment = environment
var environment = ProcessInfo.processInfo.environment
let extraPaths = ["/opt/homebrew/bin", "/usr/local/bin", "\(NSHomeDirectory())/.local/bin"]
let existingPath = environment["PATH"] ?? "/usr/bin:/bin:/usr/sbin:/sbin"
environment["PATH"] = (extraPaths + [existingPath]).filter { !$0.isEmpty }.joined(separator: ":")
process.environment = environment
if uv == "uv" {
process.executableURL = URL(fileURLWithPath: "/usr/bin/env")
process.arguments = ["uv", "run", "skim"] + arguments
}

@seungwonme
seungwonme merged commit c9600d6 into main Jul 4, 2026
3 checks passed
@seungwonme
seungwonme deleted the worktree-fix-transcribe-path branch July 4, 2026 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant