fix(doctor): detect virtiofsd outside PATH + document COWORK_VM_BACKEND#324
fix(doctor): detect virtiofsd outside PATH + document COWORK_VM_BACKEND#324CyPack wants to merge 1 commit intoaaddrick:mainfrom
Conversation
aaddrick
left a comment
There was a problem hiding this comment.
Hey! This is solid work addressing a real pain point. The --doctor false warning on Fedora and Debian has been confusing users since #293, and the documentation additions for COWORK_VM_BACKEND fill a genuine gap. The _doctor_find_virtiofsd() helper is clean and well-structured.
I found two issues in the shell script logic and one potential improvement in how the override is reported. The docs look good overall with one small note.
scripts/launcher-common.sh
Issue 1: Severity mismatch in virtiofsd not-found branch
The existing tool loop uses "$_kvm_issue" to control whether missing tools show as [WARN] (KVM active) or [INFO] (KVM not active). The new elif branch hardcodes _warn and always shows the fix hint. This means when a user hasn't set COWORK_VM_BACKEND=kvm, virtiofsd will show [WARN] while QEMU and socat show as plain info lines. It also always prints the install hint, unlike the other tools which gate it behind $_kvm_active.
Recommendation: Match the existing pattern so all three KVM tools behave consistently.
elif [[ $_tool_bin == 'virtiofsd' ]]; then
local _vfsd_path
_vfsd_path=$(_doctor_find_virtiofsd) || true
if [[ -n $_vfsd_path ]]; then
_pass "$_tool_label: found ($_vfsd_path, not in PATH)"
else
"$_kvm_issue" "$_tool_label: not found"
if $_kvm_active; then
_info \
"Fix: $(_cowork_pkg_hint \
"$_distro_id" "$_tool_pkg")"
fi
fiIssue 2: Duplicate COWORK_VM_BACKEND reporting
The new block at the top of the Cowork section prints _info "Backend override: COWORK_VM_BACKEND=...". But the existing code already reports the override in the "Cowork isolation:" line with "(via override)" appended. Running --doctor with COWORK_VM_BACKEND=bwrap would show both. I'd suggest either dropping the early mention since "(via override)" already covers it, or shortening to just _info "COWORK_VM_BACKEND=${COWORK_VM_BACKEND}" so it reads more like a status line.
Issue 3 (minor): :- vs - inconsistency
The new code uses ${COWORK_VM_BACKEND:-} but the existing convention in this file uses ${COWORK_VM_BACKEND-} without the colon. Both work identically for -n tests, but keeping it consistent helps readability.
docs/TROUBLESHOOTING.md
The new sections are well-written. One note: with this PR's _doctor_find_virtiofsd() fix, --doctor will correctly detect virtiofsd at /usr/libexec/. The symlink workaround is still valid for the KVM backend runtime (which spawns virtiofsd by name via PATH), so the advice is correct — might be worth mentioning that distinction. Not blocking, especially since PR #337 just disabled VM downloads on Linux for now.
Summary
- Fix severity mismatch — Use
"$_kvm_issue"and gate the fix hint behind$_kvm_active(matches existing pattern). This one should be addressed before merge. - Consider deduplicating the COWORK_VM_BACKEND display (minor)
- Use
${COWORK_VM_BACKEND-}without colon for consistency (minor)
Written by Claude Opus 4.6 via Claude Code
Address all three review issues from @aaddrick: Issue 1 (severity mismatch): Implement _kvm_active/_kvm_issue pattern to vary severity of KVM-specific tool checks (QEMU, socat, virtiofsd). When KVM is not the active/intended backend, missing KVM tools show as plain info lines instead of warnings. bubblewrap always warns since it is an independent backend. Issue 2 (duplicate reporting): Remove the separate Backend override info line. Instead, make the Cowork isolation detection honor COWORK_VM_BACKEND the same way the daemon does, appending [override] to the label. Also warn on invalid override values. Issue 3 (colon convention): Keep :- (with colon) as-is. Audited all 21 parameter expansions in production code: 100% use :- consistently. The colon correctly treats empty string same as unset, matching the JS side (process.env.COWORK_VM_BACKEND || null). Also fix TROUBLESHOOTING.md to clarify that --doctor now detects virtiofsd at /usr/libexec/ automatically, but the symlink is still needed for the KVM backend at runtime (spawns by PATH only). Co-Authored-By: Claude <claude@anthropic.com>
|
Thanks for the thorough review! All three issues addressed in commit d1d62a1. Issue 1: Severity mismatch (MUST FIX) ✅Deep dive revealed that Implemented: Added
Applied consistently to all three KVM tools (QEMU, socat, virtiofsd) while keeping bubblewrap as always- Also restructured the tool loop to use Issue 2: Duplicate reporting ✅The Fixed: Made the backend detection mirror Removed the separate Also added a Issue 3: Colon convention ✅ (respectfully keeping
|
Add _doctor_find_virtiofsd() to probe /usr/libexec/virtiofsd (Fedora/RHEL) and /usr/lib/qemu/virtiofsd (Debian/Ubuntu) when virtiofsd is not in PATH. Shows [PASS] with the discovered path instead of a false [WARN] not-found. Add warning for invalid COWORK_VM_BACKEND values with valid-values hint. Document COWORK_VM_BACKEND in CONFIGURATION.md with priority table, override examples, and desktop entry persistence. Add three Cowork troubleshooting sections to TROUBLESHOOTING.md: - VM connection timeout (vsock/first-boot) with bwrap workaround - virtiofsd not found on Fedora (doctor vs runtime PATH distinction) - cross-device link error on Fedora tmpfs /tmp Co-Authored-By: Claude <claude@anthropic.com>
d1d62a1 to
de275ef
Compare
|
Rebased onto current What changed in the rebaseDropped everything upstream already implemented since the original PR:
What remains (all genuinely new)
Tested: shellcheck clean, Generated with Claude Code |
Summary
virtiofsdinstalled outside$PATH(Fedora/usr/libexec/, Debian/usr/lib/qemu/)COWORK_VM_BACKENDenvironment variable (host|bwrap|kvm)Problem
On Fedora/RHEL,
virtiofsdinstalls to/usr/libexec/virtiofsdwhich is outside$PATH. The--doctorcheck usescommand -vand reports[WARN] virtiofsd: not foundeven though the binary is installed. Same issue on Debian/Ubuntu with/usr/lib/qemu/virtiofsd.The
COWORK_VM_BACKENDenvironment variable exists incowork-vm-service.js(line 47) but is undocumented indocs/CONFIGURATION.md.docs/TROUBLESHOOTING.mdhas no Cowork section at all.Changes
scripts/launcher-common.sh_doctor_find_virtiofsd()helper probing/usr/libexec/virtiofsdand/usr/lib/qemu/virtiofsd[PASS] virtiofsd: found (/path, not in PATH)instead of false warningCOWORK_VM_BACKENDoverride when setdocs/CONFIGURATION.mdCOWORK_VM_BACKENDto env vars tabledocs/TROUBLESHOOTING.mdCOWORK_VM_BACKEND=bwrap)/tmp(TMPDIR fix)Test plan
bash -n scripts/launcher-common.shpassesclaude-desktop --doctorcorrectly detects virtiofsd at/usr/libexec/virtiofsdCOWORK_VM_BACKEND=bwrap claude-desktopconfirmed workingTMPDIR=~/.config/Claude/tmpprevents EXDEV on Fedora tmpfsRelated
Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
70% AI / 30% Human
Claude: virtiofsd PATH detection, documentation, troubleshooting
Human: testing, review, Fedora-specific validation