Phase 1 Wave 3: AppImage launcher + .deb ffprobe + Docker LAN + Gatekeeper probe (closes #54, #56, #76, #80)#93
Merged
Conversation
) WebKitGTK 2.44.x and 2.46.x have a compositing-path regression on Wayland that blanks the AppImage's first paint on Fedora 44 / Ubuntu 24.04. Setting WEBKIT_DISABLE_COMPOSITING_MODE=1 forces the software fallback that works, but blindly setting it on healthy WebKit versions (2.48+) regresses those. This wave adds a conditional AppRun launcher that detects the WebKit version via pkg-config and only sets the env var on the broken ranges (plus a fail-safe when pkg-config is absent or the version is unknown). The launcher is injected into Tauri's AppImage staging dir via a beforeBundleCommand hook — see .planning/decisions/apprun-strategy.md for the spike outcome and rationale (Strategy B chosen). Phase 1 Wave 3 — Plan 01-03 Task 1. Closes #56 frontend half. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#76) Prior versions placed the bundled ffprobe at /usr/bin/ffprobe via Tauri's externalBin, which overwrites the system ffprobe on Ubuntu 26.04 and collides with apt-installed media-package ffprobe. Relocate the .deb-bundled ffprobe to /usr/lib/omnivoice-studio/bin/ffprobe via bundle.linux.deb.files, plus defensive maintainer scripts: - preinst: ensure target dir exists for upgrade flows - postinst: remove legacy /usr/bin/ffprobe ONLY when dpkg confirms our package owns it (never touches a user's distro ffprobe) - postrm: clean up the relocated path tree on purge/remove Rust side (tools.rs::resolve_ffprobe) now probes the new path on Linux, and backend spawn (backend.rs) carries both FFPROBE_PATH (legacy alias) and OMNIVOICE_FFPROBE_PATH (canonical) into the backend env. Python side (ffmpeg_utils.resolve_ffprobe) reads OMNIVOICE_FFPROBE_PATH first, falls back to FFPROBE_PATH, then to shutil.which("ffprobe"). 6 new unit tests cover the env-cascade resolution. Phase 1 Wave 3 — Plan 01-03 Task 2. Closes #76. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Docker / LAN browser users hit the preview API at the LAN host's IP, not
their local machine — the prior frontend/src/utils/media.js:20 hardcoded
http://localhost:3900, which from a LAN client resolved to the client
machine itself.
Centralise via frontend/src/utils/apiBase.ts:
1. VITE_OMNIVOICE_API override (Docker compose / dev) always wins.
2. Tauri webview → http://localhost:3900 (unchanged behaviour).
3. Plain browser → ${window.location.protocol}//${window.location.hostname}:3900
(follows the page's origin — closes #80).
4. SSR / no-window → http://localhost:3900 (safe fallback).
Grep-sweep confirmed media.js:20 was the only hardcode site (Assumption
A4 in 01-RESEARCH.md verified). 6 new vitest cases cover the resolver.
Phase 1 Wave 3 — Plan 01-03 Task 3. Closes #80 frontend half.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds backend/core/gatekeeper_detect.py which walks up from sys.executable to find the .app bundle and runs `xattr -l` to check for the quarantine extended attribute (com.apple.quarantine). On detection, the lifespan startup probe logs a structured warning and emits a system_error event through the existing event bus with error_class="GATEKEEPER_QUARANTINE", which Wave 2's React ErrorBoundary turns into a docs deeplink. Detection is informational only — we never auto-run `xattr -cr` (the app itself is quarantined and cannot fix its own state per Anti-Pattern in 01-RESEARCH.md). Users get a clear pointer to the workaround docs. GET /system/quarantine-status exposes the structured payload so the frontend can poll on first load. INST-01 (setuptools>=75.0 pin from PR #62) gains a PR-time guard in tests/backend/test_pyproject.py + a user-observable smoke check in scripts/smoke-test.sh (pkg_resources + whisperx import). 7 gatekeeper tests + 1 pyproject test added — all pass. Phase 1 Wave 3 — Plan 01-03 Task 4. Closes #54 backend half (Wave 2 owns the docs page + ErrorBoundary deeplink wiring). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR ships Phase 1 Wave 3 completions: a custom AppImage AppRun launcher with WebKitGTK workarounds, Linux .deb ffprobe relocation with maintainer scripts, centralized frontend API base URL resolution, macOS Gatekeeper quarantine detection with REST endpoint and startup logging, plus comprehensive test coverage and phase documentation. ChangesPhase 1 Wave 3: Multi-feature implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
@.planning/phases/01-install-token-persistence-docs-scaffolding-error-ux/01-03-SUMMARY.md:
- Around line 152-156: The fenced code block containing the grep command
"(localhost|127\.0\.0\.1):3900' frontend/src/" needs a language tag to satisfy
markdownlint MD040—edit the opening fence to "```bash" (i.e., add bash after the
three backticks) for the block that includes the grep pipeline and the
frontend/src/utils/media.js comment.
In `@backend/core/gatekeeper_detect.py`:
- Around line 89-90: The subprocess.run call that invokes "xattr" in
backend/core/gatekeeper_detect.py is vulnerable to PATH hijack; change the
invocation to use an absolute path (e.g. "/usr/bin/xattr") or resolve and
validate the binary once with shutil.which before calling subprocess.run (update
the call that builds ["xattr", "-l", bundle] to use the resolved path), and
handle the case where the binary is not found by logging or raising an error.
In `@backend/services/ffmpeg_utils.py`:
- Line 99: The current assignment candidate = ffmpeg_path.replace("ffmpeg",
"ffprobe") can mangle directory names; instead compute the probe path using
path-aware operations: locate the ffmpeg executable name (ffmpeg) and replace
only that basename with "ffprobe" (e.g., use
pathlib.Path(ffmpeg_path).with_name("ffprobe") or
os.path.join(os.path.dirname(ffmpeg_path), "ffprobe")) so directories containing
"ffmpeg" are not modified; update the code where candidate is set to use this
path-safe replacement (ffmpeg_path variable).
In `@scripts/smoke-test.sh`:
- Around line 327-330: The INST-01 import check currently uses "uv run python"
which may target a different environment; change the probe to invoke the
app-managed Python binary (the runtime/venv created during bootstrap) instead of
"uv run python" so the import test runs inside the actual app runtime. Replace
the "uv run python -c \"import pkg_resources; import whisperx\"" call with the
bootstrap-created runtime Python variable (e.g., the script's APP_PYTHON or
VENV_PYTHON) and keep the same pass/fail messages ("INST-01: ..." and "INST-01
regression: ...") so the check validates the launched app environment rather
than uv's environment.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d5abd337-4e23-4734-b88f-89fc1be15159
📒 Files selected for processing (23)
.planning/decisions/apprun-strategy.md.planning/phases/01-install-token-persistence-docs-scaffolding-error-ux/01-03-SUMMARY.mdbackend/api/routers/system.pybackend/core/gatekeeper_detect.pybackend/main.pybackend/services/ffmpeg_utils.pyfrontend/src-tauri/appimage/AppRunfrontend/src-tauri/appimage/AppRun.test.shfrontend/src-tauri/debian/postinstfrontend/src-tauri/debian/postrmfrontend/src-tauri/debian/preinstfrontend/src-tauri/src/backend.rsfrontend/src-tauri/src/tools.rsfrontend/src-tauri/tauri.conf.jsonfrontend/src-tauri/tauri.linux.conf.jsonfrontend/src/utils/apiBase.test.tsfrontend/src/utils/apiBase.tsfrontend/src/utils/media.jsscripts/inject-apprun.shscripts/smoke-test.shtests/backend/core/test_gatekeeper_detect.pytests/backend/services/test_ffmpeg_utils.pytests/backend/test_pyproject.py
Comment on lines
+152
to
+156
| ``` | ||
| $ grep -rnE '(localhost|127\.0\.0\.1):3900' frontend/src/ \ | ||
| --include='*.{js,jsx,ts,tsx}' | grep -v apiBase.ts | grep -v apiBase.test.ts | ||
| frontend/src/utils/media.js:21:// (issue #80) get window.location.hostname:3900 instead of localhost:3900. | ||
| ``` |
Contributor
There was a problem hiding this comment.
Specify a language for the fenced code block.
Line 152 opens a fence without a language, which triggers markdownlint MD040. Mark it as bash since it contains shell commands.
Suggested fix
-```
+```bash
$ grep -rnE '(localhost|127\.0\.0\.1):3900' frontend/src/ \
--include='*.{js,jsx,ts,tsx}' | grep -v apiBase.ts | grep -v apiBase.test.ts
frontend/src/utils/media.js:21:// (issue `#80`) get window.location.hostname:3900 instead of localhost:3900.</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 152-152: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@.planning/phases/01-install-token-persistence-docs-scaffolding-error-ux/01-03-SUMMARY.md
around lines 152 - 156, The fenced code block containing the grep command
"(localhost|127\.0\.0\.1):3900' frontend/src/" needs a language tag to satisfy
markdownlint MD040—edit the opening fence to "```bash" (i.e., add bash after the
three backticks) for the block that includes the grep pipeline and the
frontend/src/utils/media.js comment.
Comment on lines
+89
to
+90
| result = subprocess.run( | ||
| ["xattr", "-l", bundle], |
Contributor
There was a problem hiding this comment.
Use an absolute xattr path to avoid PATH hijack risk.
At Line 89, spawning "xattr" via PATH can execute an unexpected binary if PATH is manipulated. Prefer /usr/bin/xattr (or resolve once via shutil.which and validate).
Suggested patch
@@
-import subprocess
+import shutil
+import subprocess
@@
- result = subprocess.run(
- ["xattr", "-l", bundle],
+ xattr_bin = shutil.which("xattr") or "/usr/bin/xattr"
+ result = subprocess.run(
+ [xattr_bin, "-l", bundle],
capture_output=True,
text=True,
timeout=5,
check=False,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| result = subprocess.run( | |
| ["xattr", "-l", bundle], | |
| import shutil | |
| import subprocess | |
| xattr_bin = shutil.which("xattr") or "/usr/bin/xattr" | |
| result = subprocess.run( | |
| [xattr_bin, "-l", bundle], | |
| capture_output=True, | |
| text=True, | |
| timeout=5, | |
| check=False, | |
| ) |
🧰 Tools
🪛 Ruff (0.15.13)
[error] 89-89: subprocess call: check for execution of untrusted input
(S603)
[error] 90-90: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/core/gatekeeper_detect.py` around lines 89 - 90, The subprocess.run
call that invokes "xattr" in backend/core/gatekeeper_detect.py is vulnerable to
PATH hijack; change the invocation to use an absolute path (e.g.
"/usr/bin/xattr") or resolve and validate the binary once with shutil.which
before calling subprocess.run (update the call that builds ["xattr", "-l",
bundle] to use the resolved path), and handle the case where the binary is not
found by logging or raising an error.
| if os.path.isfile(candidate): | ||
| return candidate | ||
| if ffmpeg_path: | ||
| candidate = ffmpeg_path.replace("ffmpeg", "ffprobe") |
Contributor
There was a problem hiding this comment.
Use path-aware replacement to avoid corrupting directory names.
The string replace("ffmpeg", "ffprobe") will incorrectly transform paths where "ffmpeg" appears in a directory name (e.g., /opt/ffmpeg/bin/ffmpeg → /opt/ffprobe/bin/ffprobe).
🛠️ Proposed fix using path manipulation
ffmpeg_path = find_ffmpeg()
if ffmpeg_path:
- candidate = ffmpeg_path.replace("ffmpeg", "ffprobe")
+ parent = os.path.dirname(ffmpeg_path)
+ basename = os.path.basename(ffmpeg_path)
+ candidate = os.path.join(parent, basename.replace("ffmpeg", "ffprobe"))
if os.path.isfile(candidate):
return candidate🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/services/ffmpeg_utils.py` at line 99, The current assignment
candidate = ffmpeg_path.replace("ffmpeg", "ffprobe") can mangle directory names;
instead compute the probe path using path-aware operations: locate the ffmpeg
executable name (ffmpeg) and replace only that basename with "ffprobe" (e.g.,
use pathlib.Path(ffmpeg_path).with_name("ffprobe") or
os.path.join(os.path.dirname(ffmpeg_path), "ffprobe")) so directories containing
"ffmpeg" are not modified; update the code where candidate is set to use this
path-safe replacement (ffmpeg_path variable).
Comment on lines
+327
to
+330
| if uv run python -c "import pkg_resources; import whisperx" 2>/dev/null; then | ||
| pass "INST-01: pkg_resources + whisperx import OK (setuptools pinned)" | ||
| else | ||
| fail "INST-01 regression: pkg_resources or whisperx import failed (setuptools pin?)" |
Contributor
There was a problem hiding this comment.
Run INST-01 import check against the launched app runtime, not uv run.
At Line 327, uv run can validate a different environment than the app process used in this smoke test, causing false confidence or false failures. Point the import check at the app-managed Python/venv path created during bootstrap.
Suggested patch
TESTS=$((TESTS + 1))
-if uv run python -c "import pkg_resources; import whisperx" 2>/dev/null; then
+APP_PY="${OV_DATA}/.venv/bin/python"
+if [ -x "$APP_PY" ] && "$APP_PY" -c "import pkg_resources; import whisperx" 2>/dev/null; then
pass "INST-01: pkg_resources + whisperx import OK (setuptools pinned)"
else
- fail "INST-01 regression: pkg_resources or whisperx import failed (setuptools pin?)"
+ fail "INST-01 regression: app runtime import failed (pkg_resources or whisperx)"
fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/smoke-test.sh` around lines 327 - 330, The INST-01 import check
currently uses "uv run python" which may target a different environment; change
the probe to invoke the app-managed Python binary (the runtime/venv created
during bootstrap) instead of "uv run python" so the import test runs inside the
actual app runtime. Replace the "uv run python -c \"import pkg_resources; import
whisperx\"" call with the bootstrap-created runtime Python variable (e.g., the
script's APP_PYTHON or VENV_PYTHON) and keep the same pass/fail messages
("INST-01: ..." and "INST-01 regression: ...") so the check validates the
launched app environment rather than uv's environment.
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.
Summary
Phase 1 Wave 3 of the v0.3.0 stabilization milestone. Lands four issue-bound fixes plus the macOS Gatekeeper backend detection wiring that Wave 2's React ErrorBoundary consumes.
frontend/src-tauri/appimage/AppRunconditionally setsWEBKIT_DISABLE_COMPOSITING_MODE=1only on broken WebKit ranges (2.44.x / 2.46.x / unknown), injected into Tauri's bundler via abeforeBundleCommandhook. Spike outcome documented in.planning/decisions/apprun-strategy.md./usr/lib/omnivoice-studio/bin/ffprobeviabundle.linux.deb.files. Defensivepostinstremoves any legacy/usr/bin/ffprobeonly whendpkg -Sconfirms our package owns it. Backend resolves throughOMNIVOICE_FFPROBE_PATH(canonical) withFFPROBE_PATHretained as a legacy alias.frontend/src/utils/apiBase.tsfollowswindow.location.hostname:3900in plain-browser contexts so LAN clients hit the OmniVoice host, not their own loopback. Tauri webview unchanged. Single hardcode site (media.js:20) confirmed by grep sweep.backend/core/gatekeeper_detect.pyrunsxattr -lagainst the resolved.appbundle at lifespan startup; on detection it logs a structured warning and emitserror_class="GATEKEEPER_QUARANTINE"via the existing event bus for the Wave 2 deeplink. Detection only — never auto-runsxattr -cr.tests/backend/test_pyproject.pyguards thesetuptools>=75.0pin from PR fix: setuptools pin, Linux/Russia troubleshooting (#58, #56, #60) #62. Smoke test gains an inlinepkg_resources + whisperximport probe.Test plan
uv run pytest tests/backend/services/test_ffmpeg_utils.py tests/backend/core/test_gatekeeper_detect.py tests/backend/test_pyproject.py -x -v→ 14/14 passbash frontend/src-tauri/appimage/AppRun.test.sh→ 4/4 pass (per checker W-1: 2.44 broken, 2.46 broken, 2.48 healthy, pkg-config absent)cd frontend && bun run test apiBase→ 6/6 passcd frontend && bun run test(all) → 30/30 pass (no regressions)grep -rnE '(localhost|127\.0\.0\.1):3900' frontend/src/→ only comment lines remainuv run pytest tests/ -q(full backend) → 292 passed, 6 skipped, 12 xfailed, 1 xpassed (no failures)/usr/lib/omnivoice-studio/bin/ffprobeexists and backend reads it viaOMNIVOICE_FFPROBE_PATH./Applications, confirm that withxattr -lshowing quarantine, the startup probe surfaces the error class.Closures
Decision docs
.planning/decisions/apprun-strategy.md— AppRun strategy spike outcome.planning/phases/01-install-token-persistence-docs-scaffolding-error-ux/01-03-SUMMARY.md— full wave summaryDeviations from plan
cargo tauri build(the build requires Linux). The decision doc calls this out — Phase 6 release.yml smoke validates the injected AppRun end-to-end.FFPROBE_PATHenv var kept alongside the canonicalOMNIVOICE_FFPROBE_PATHfor backward compat with prior backend builds and external scripts.apiBase.tsexposes a small_setEnvOverrideForTesting()hook because vitest 4.x does not propagatevi.stubEnvto dynamically imported modules'import.meta.env. Production code path unchanged.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests