Skip to content

Commit d1d62a1

Browse files
CyPackclaude
authored andcommitted
fix(doctor): address code review feedback on PR aaddrick#324
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>
1 parent 36ef3f8 commit d1d62a1

2 files changed

Lines changed: 62 additions & 24 deletions

File tree

docs/TROUBLESHOOTING.md

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,9 +97,12 @@ See [CONFIGURATION.md](CONFIGURATION.md#cowork-backend) for how to make this per
9797

9898
### Cowork: virtiofsd not found (Fedora/RHEL)
9999

100-
On Fedora and RHEL, `virtiofsd` installs to `/usr/libexec/virtiofsd` which is outside `$PATH`. The `--doctor` check may report `[WARN] virtiofsd: not found` even though the package is installed.
100+
On Fedora and RHEL, `virtiofsd` installs to `/usr/libexec/virtiofsd` which is
101+
outside `$PATH`. The `--doctor` check detects it there automatically and will
102+
show `[PASS]`, but the KVM backend spawns `virtiofsd` by name at runtime and
103+
resolves it through `$PATH` only.
101104

102-
**Fix:** Create a symlink:
105+
**Fix:** Create a symlink so the KVM backend can find it at runtime:
103106

104107
```bash
105108
sudo ln -s /usr/libexec/virtiofsd /usr/local/bin/virtiofsd

scripts/launcher-common.sh

Lines changed: 57 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -483,11 +483,6 @@ print(len(servers))
483483
local _distro_id
484484
_distro_id=$(_cowork_distro_id)
485485

486-
# COWORK_VM_BACKEND override
487-
if [[ -n "${COWORK_VM_BACKEND:-}" ]]; then
488-
_info "Backend override: COWORK_VM_BACKEND=${COWORK_VM_BACKEND}"
489-
fi
490-
491486
# KVM access
492487
if [[ -e /dev/kvm ]]; then
493488
if [[ -r /dev/kvm && -w /dev/kvm ]]; then
@@ -510,6 +505,19 @@ print(len(servers))
510505
_info 'Fix: sudo modprobe vhost_vsock'
511506
fi
512507

508+
# KVM-specific tools (QEMU, socat, virtiofsd) are only
509+
# actionable when the KVM backend is active or intended.
510+
# Show them as [WARN] with fix hints when KVM is relevant,
511+
# or as plain [INFO] lines when it is not.
512+
local _kvm_active=false _kvm_issue=_info
513+
if [[ -n "${COWORK_VM_BACKEND:-}" ]]; then
514+
[[ "${COWORK_VM_BACKEND,,}" == 'kvm' ]] \
515+
&& _kvm_active=true
516+
elif [[ -e /dev/kvm && -r /dev/kvm && -w /dev/kvm ]]; then
517+
_kvm_active=true
518+
fi
519+
$_kvm_active && _kvm_issue=_warn
520+
513521
# Check required tools: label, binary, pkg-hint name
514522
local _tool_label _tool_bin _tool_pkg
515523
for _tool_label in \
@@ -525,21 +533,32 @@ print(len(servers))
525533

526534
if command -v "$_tool_bin" &>/dev/null; then
527535
_pass "$_tool_label: found"
528-
elif [[ $_tool_bin == 'virtiofsd' ]]; then
536+
continue
537+
fi
538+
539+
# virtiofsd: also check non-PATH locations
540+
if [[ $_tool_bin == 'virtiofsd' ]]; then
529541
local _vfsd_path
530542
_vfsd_path=$(_doctor_find_virtiofsd) || true
531543
if [[ -n $_vfsd_path ]]; then
532544
_pass "$_tool_label: found ($_vfsd_path, not in PATH)"
533-
else
534-
_warn "$_tool_label: not found"
545+
continue
546+
fi
547+
fi
548+
549+
# Not found — severity depends on backend relevance
550+
if [[ $_tool_bin == 'bwrap' ]]; then
551+
_warn "$_tool_label: not found"
552+
_info \
553+
"Fix: $(_cowork_pkg_hint \
554+
"$_distro_id" "$_tool_pkg")"
555+
else
556+
"$_kvm_issue" "$_tool_label: not found"
557+
if $_kvm_active; then
535558
_info \
536559
"Fix: $(_cowork_pkg_hint \
537560
"$_distro_id" "$_tool_pkg")"
538561
fi
539-
else
540-
_warn "$_tool_label: not found"
541-
_info \
542-
"Fix: $(_cowork_pkg_hint "$_distro_id" "$_tool_pkg")"
543562
fi
544563
done
545564

@@ -556,16 +575,32 @@ print(len(servers))
556575
fi
557576

558577
# Determine active backend (matches daemon's detectBackend())
559-
local cowork_backend='none (host-direct, no isolation)'
560-
if [[ -e /dev/kvm ]] \
561-
&& [[ -r /dev/kvm && -w /dev/kvm ]] \
562-
&& command -v qemu-system-x86_64 &>/dev/null \
563-
&& [[ -e /dev/vhost-vsock ]] \
564-
&& [[ -f $vm_image ]]; then
565-
cowork_backend='KVM (full VM isolation)'
566-
elif command -v bwrap &>/dev/null \
567-
&& bwrap --ro-bind / / true &>/dev/null; then
568-
cowork_backend='bubblewrap (namespace sandbox)'
578+
# When COWORK_VM_BACKEND is set, the daemon skips auto-detection
579+
# and uses the override directly — mirror that here.
580+
local cowork_backend
581+
if [[ -n "${COWORK_VM_BACKEND:-}" ]]; then
582+
case "${COWORK_VM_BACKEND,,}" in
583+
kvm) cowork_backend='KVM (full VM isolation) [override]' ;;
584+
bwrap) cowork_backend='bubblewrap (namespace sandbox) [override]' ;;
585+
host) cowork_backend='none (host-direct, no isolation) [override]' ;;
586+
*)
587+
_warn "Unknown COWORK_VM_BACKEND: '${COWORK_VM_BACKEND}'"
588+
_info 'Valid values: kvm, bwrap, host'
589+
cowork_backend="unknown override: '${COWORK_VM_BACKEND}'"
590+
;;
591+
esac
592+
else
593+
cowork_backend='none (host-direct, no isolation)'
594+
if [[ -e /dev/kvm ]] \
595+
&& [[ -r /dev/kvm && -w /dev/kvm ]] \
596+
&& command -v qemu-system-x86_64 &>/dev/null \
597+
&& [[ -e /dev/vhost-vsock ]] \
598+
&& [[ -f $vm_image ]]; then
599+
cowork_backend='KVM (full VM isolation)'
600+
elif command -v bwrap &>/dev/null \
601+
&& bwrap --ro-bind / / true &>/dev/null; then
602+
cowork_backend='bubblewrap (namespace sandbox)'
603+
fi
569604
fi
570605
_info "Cowork isolation: $cowork_backend"
571606

0 commit comments

Comments
 (0)