Bug fixes

[Jordonbc]

No description provided.

[github-actions]

Review

Minor

Frontend/src/scripts/main.ts:467 — hydrateBranches catch handler does not capture the rejection error

hydrateBranches().then(() => setRepoHeader()).catch(() => console.warn('Failed to hydrate branches on startup'));

The catch lambda ignores its argument (() =>), so the actual error object is discarded. Every other catch-handler change in this PR captures the error with (err) => console.warn(...). While this is still better than the original unconditional .then() (which left a potential unhandled rejection), the inconsistency means branch-loading failures will log a generic message without the underlying cause.

Fix: change to (err) => console.warn('Failed to hydrate branches on startup:', err).

Confidence: High — the inconsistency is unambiguous and the fix is trivial.

---

No blocking or important issues found

All other changes are sound:

• logging.rs, plugin_bundles.rs, updater.rs: Silent let _ = ... operations are correctly replaced with warn-level logging. No behavioral regression.
• validate.rs: .unwrap() → .expect() for hardcoded regexes. Equivalent behavior, better panic message.
• plugin_vcs_backends.rs: The OnceLock<RwLock<...>> cache correctly avoids re-syncing plugins on every list_plugin_vcs_backends call. Cache invalidation fires at every modification path (set_config, sync_configured_plugins, uninstall_plugin). set_plugin_enabled invalidates indirectly through state.set_config(). No stale-cache path visible.
• ssh.rs: resolve_ssh_askpass properly handles absolute paths, PATH lookup, executability checks on Unix, and graceful None fallback on all platforms. The is_executable helper has correct #[cfg] branches. No resource leaks or hanging paths.
• sshKeys.ts: Extended regex correctly catches the new askpass-not-found error patterns.
• diff.ts: Truncation now operates on hookData.summary (post-hook) instead of the pre-hook local, which was the actual bug. commitSummary.value update is correctly moved inside the block.
• settings.ts: The disabled theme select is now re-enabled alongside the theme-auto checkbox — a correct fix for a stale-DOM-ref bug.
• themes.ts: sanitizeThemeMarkup strips <script> elements and on* event attributes. The sanitizer runs before nodes are appended to the live DOM. Legitimate theme scripts are still injected via the separate applyScriptNodes path, so this does not break valid theme functionality.
• main.ts: All remaining 10 catch(() => {}) → catch((err) => console.warn(...)) changes are correct and consistently preserve the error.
• All test files: New/modified tests use proper Vitest patterns (beforeEach/afterEach cleanup, vi.resetModules(), vi.useFakeTimers()), colocate correctly, verify observable behavior, and import only exported symbols. No test pollution between files.
• All copyright headers follow the required // Copyright © 2025-2026 OpenVCS Contributors // SPDX-License-Identifier: GPL-3.0-or-later template on new files.
• Imports, scoping, and cfg gates are correct across all changed files.

opencode session  |  github run

[github-actions]

I've completed my analysis. Here is my review:

---

Review

Important

Backend/src/tauri_commands/plugins.rs:266-299 — set_plugin_approval does not invalidate the VCS backend cache

set_plugin_approval changes a plugin's approval state on disk (via store.approve_capabilities) and optionally starts/stops the runtime, but never calls invalidate_plugin_vcs_backend_cache(). Since list_plugin_vcs_backends in plugin_vcs_backends.rs:102-108 returns cached data and filters by approval state (lines 150-167), approving or denying a plugin will not be reflected in VCS backend availability until the next cache miss (cold start or explicit invalidation elsewhere).

Every other state mutation path (set_config, sync_configured_plugins, uninstall_plugin, set_plugin_enabled) correctly invalidates the cache. This path is missing.

Fix: Add crate::plugin_vcs_backends::invalidate_plugin_vcs_backend_cache(); in set_plugin_approval alongside the existing runtime-management calls.

Impact: After approving a plugin that provides VCS backends, those backends are invisible until app restart or another code path invalidates the cache.

Confidence: High

---

Frontend/src/scripts/features/settings.ts:452-458 — openSettings finally block overrides correct disabled state on theme select

The .finally() callback unconditionally sets #set-theme.disabled = false (line 457). This runs after loadGeneralSettingsIntoForm (settingsGeneral.ts:58) correctly sets themeSel.disabled = themePref === 'system'. When the saved theme is 'system', the select ends up enabled instead of disabled, creating an inconsistent UI state where the user can change the theme pack while auto mode is active.

This is a regression introduced by the PR — the old .finally() only touched #set-theme-auto, not the select.

Fix: Remove the setThemeSel.disabled = false assignment from the finally block, or make it conditional on the auto-checkbox state matching the theme preference.

Impact: Settings form shows wrong control state for system-theme users; theme selection can be changed but will be overridden on next auto toggle.

Confidence: High

---

Minor

Backend/src/tauri_commands/ssh.rs:135-144 — resolve_command cannot distinguish bare executable names from paths

candidate.parent().is_some() returns true for both absolute paths (/usr/bin/ksshaskpass) and bare filenames (ksshaskpass), because Path::new("ksshaskpass").parent() returns Some(Path::new("")). A bare filename from the SSH_ASKPASS environment variable would be checked in the current working directory instead of PATH.

In practice SSH_ASKPASS values are almost always absolute paths, so the impact is low.

Fix: Check candidate.is_absolute() or candidate.components().count() > 1 instead of candidate.parent().is_some().

Confidence: Medium

---

Frontend/src/scripts/main.ts:473,533 — Two remaining empty catch handlers on refreshPluginMenubarMenus

Lines 473 and 533 still use .catch(() => {}) on refreshPluginMenubarMenus(), even though the PR converted 10+ other silent catches to console.warn(...). While failures here are arguably benign during startup/transition, the inconsistency with the rest of the PR is noticeable.

Fix: Convert to .catch((err) => console.warn('Plugin menu refresh failed:', err)) for consistency.

Confidence: High

---

No blocking issues found. All other changes are sound: the VCS backend cache invalidation wiring in state.rs, plugins.rs, and plugin_vcs_backends.rs is correct for all paths except set_plugin_approval; the sanitizeThemeMarkup function correctly removes XSS vectors while allowing legitimate theme scripts through applyScriptNodes; diff.ts correctly uses hookData.summary after plugin hooks; ssh.rs askpass resolution, is_executable cfg gates, and ssh_add_key fallback are correct; validate.rs .unwrap() to .expect() are equivalent; updater.rs, logging.rs, plugin_bundles.rs warn-level replacements are correct; sshKeys.ts regex extension covers the new error patterns; main.ts catch conversions are correct; all test additions are valid and properly isolated.

opencode session  |  github run

[github-actions]

Review

No blocking or important issues found. All changes are sound.

The two "important" findings from an earlier review are not present in the current code:

• set_plugin_approval (Backend/src/tauri_commands/plugins.rs:283) does call invalidate_plugin_vcs_backend_cache() — already fixed.
• openSettings finally block (Frontend/src/scripts/features/settings.ts:452-456) only re-enables #set-theme-auto, not #set-theme. The theme-select disabled state is correctly managed by loadGeneralSettingsIntoForm. No regression.

The earlier "minor" findings about resolve_command using parent().is_some() and empty catches on refreshPluginMenubarMenus are also not applicable to the current code — resolve_command:136 correctly uses is_absolute() || components().count() > 1, and both call sites (lines 473, 533) correctly capture the error with console.warn.

All changes verified:

• plugin_vcs_backends.rs: Cache with OnceLock<RwLock<...>>, invalidation at every mutation path (set_config, sync_configured_plugins, uninstall_plugin, set_plugin_approval). No stale-cache path.
• ssh.rs: resolve_ssh_askpass with correct PATH lookup, resolve_command distinguishes paths from bare names, is_executable has correct #[cfg] branches, ssh_add_key graceful fallback. No resource leaks.
• diff.ts: Uses hookData.summary (post-hook) instead of the pre-hook local — correct bug fix.
• themes.ts: sanitizeThemeMarkup strips <script> and on* attributes before DOM append. Legitimate scripts still injected via separate applyScriptNodes path. No breakage.
• main.ts: All 11 catch conversions preserve the error. No remaining silent swallows on the changed lines.
• validate.rs: .unwrap() → .expect() for hardcoded regexes — equivalent behavior.
• logging.rs, plugin_bundles.rs, updater.rs: Silent let _ = → warn-level logging — correct.
• sshKeys.ts: Extended regex covers askpass-not-found error patterns — correct.
• All test files: Proper Vitest patterns, beforeEach/afterEach cleanup, vi.resetModules(), no test pollution.

opencode session  |  github run