Add more tests

[Jordonbc]

No description provided.

[github-actions]

Now I have a comprehensive view of the PR. Here is my review:

---

Review: Add more tests (22 commits, 100 files)

No blocking issues found.

Important: Stash paths silently dropped in PluginVcsProxy

Backend/src/plugin_runtime/vcs_proxy.rs:348-364 — stash_push accepts a _paths: &[PathBuf] parameter (matching the Vcs trait at core/mod.rs:158) but ignores it. The _ prefix suppresses the unused-warning, but the paths are never forwarded to the plugin. The node runtime's vcs_stash_push at node_instance/vcs.rs:311-321 doesn't accept paths at all. Meanwhile tauri_commands/stash.rs:81-97 builds pathbufs from user input and passes them through the proxy, creating a silent data loss path: callers believe paths are stashed, but the full working tree is saved instead.

This is a pre-existing gap, not a PR regression, but the PR adds the command-layer path plumbing (in stash.rs) without addressing the proxy/plugin hole. Either the proxy should forward paths (and the plugin contract extended), or the command handler should make paths a no-op so callers aren't misled.

Confidence: High

Important: Tests for close_vcs_session exist but do not invoke the method

Backend/tests/plugin_runtime/node_instance/mod.rs:231-242 — close_vcs_session_calls_rpc_when_session_active sets up a runtime with a session id and a mock handler, but never calls close_vcs_session(). The adjacent close_vcs_session_noop_when_no_session test is similarly inert (no call, no assertion). These tests document intent but exercise no code path.

Confidence: High

Minor: host_api.rs globals bleed across tests

Backend/tests/plugin_runtime/host_api.rs:8-19 — set_status_event_emitter installs a global OnceLock-based emitter and set_status_text_unchecked writes to a global OnceLock<RwLock<String>>. No clear/reset helper exists for either global. Tests share these globals; if run in parallel or if a new test reads status text without setting it first, results are undefined (read stale value or miss events). Currently only one test file uses this module, so no breach occurs, but the design invites future breakage.

Confidence: Medium (minor because not currently broken, but an unresolved design smell)

opencode session  |  github run

[github-actions]

Review Findings

No blocking or important issues found.

After thorough review of all 100 changed files (source and test), I found no correctness bugs, regressions, unsafe assumptions, error handling problems, API misuse, race conditions, lifetime issues, or resource leaks introduced by this PR.

What this PR does:

• Extracts inline logic into named functions (build_commit_message, stash_message_or_default, backend_display_label, progress_message_for_event, resolve_default_backend_id, etc.) — all behavior-preserving.
• Adds test-only injection points (TEST_HOME_DIR, TEST_PLUGIN_DATA_ROOT, TEST_BACKEND_CACHE, TEST_APP_DIRS, set_test_home_dir, set_test_plugin_data_root, etc.) — all correctly gated behind #[cfg(test)].
• Adds test isolation guards (assert_test_isolation()) in AppConfig::save() and save_recents_to_disk().
• Adds test module includes (include!(...)) throughout every changed source module.
• Adds ~32,500 lines of new tests (Rust and TypeScript).
• Bumps coverage tooling and config (95% thresholds in vitest.config.ts, cargo-llvm-cov recipes in Justfile).
• Minor Frontend fixes: markup field accepts null in types.d.ts, formatTimeAgo catch is type-safe, context menu errors are logged.

One minor observation (not actionable in this PR):

The backend_display_label function in Backend/src/tauri_commands/backends.rs now filters out empty/whitespace-only backend names before falling through to plugin_name/backend_id, whereas the original inline code passed empty names through. This is arguably an improvement — no regression risk since an empty backend name would render an invisible label in the UI.

All test infrastructure patterns (OnceLock<RwLock<Option<T>>>, thread-locals, override setter/clearer pairs) follow standard Rust testing conventions and are correctly implemented. The Drop for PluginVcsProxy correctly stops the runtime. The Drop for NodePluginRuntimeInstance correctly cleans up the child process. The with_process retry-on-disconnect logic is sound.

No further findings.

opencode session  |  github run

[github-actions]

Review: Add more tests (26 commits, 100 files)

No blocking or important issues found.

All refactorings (extracting inline logic into named functions, adding test-only injection points, adding #[cfg(test)] test module includes) are behavior-preserving. The test infrastructure (OnceLock<RwLock<>> globals with #[cfg(test)] reset helpers, thread-local overrides, assert_test_isolation() guards, MockRpcHandler for runtime tests) follows standard Rust testing conventions and is correctly implemented.

Specific items verified:

• Stash paths in vcs_proxy.rs: paths are properly collected into Vec<String> and forwarded through vcs_stash_push() — the earlier review comment about silent dropping is no longer applicable to the current diff.
• close_vcs_session tests: both tests call runtime.close_vcs_session() and assert on mock handler calls — correct and complete.
• host_api.rs globals: reset_host_api_state_for_tests() is called at both start and end of the test, preventing cross-test bleed.
• resolve_preferred_backend_id in lib.rs vs resolve_default_backend_id in general.rs: these are exact function duplicates. Correct but a minor maintainability smell if the logic diverges in the future.
• Frontend types.d.ts markup null widening, formatTimeAgo catch safety, context menu error logging: all correct defensive improvements.
• CodeQL findings (unused imports/variables): the current state of the changed files has already addressed each one.

No further findings.

opencode session  |  github run

[github-actions]

The PR is a large test addition with behavior-preserving refactors and test infrastructure. After thorough review of all source file changes, I found no blocking or important issues in the current state.

What was verified:

• lib.rs — resolve_preferred_backend_id and first_existing_recent_repo are semantics-preserving extractions. The extra has_plugin_vcs_backend call in the new preferred_vcs_backend_id is redundant (it always returns true for backends in the list) but harmless.

• general.rs — resolve_default_backend_id is behaviorally identical to resolve_preferred_backend_id in lib.rs. This is a code duplication smell (general.rs:36 vs lib.rs:45), not a bug — both function bodies match exactly.

• app_identity.rs — project_dirs() return type changed from Option<ProjectDirs> to Option<AppDirs>. All 6 callers use only .config_dir() or .data_dir(), both present on the new type. assert_test_isolation correctly gates writes behind #[cfg(test)], and setup_test_isolation uses temp paths recreated by create_dir_all on first write.

• vcs_proxy.rs — paths are now forwarded to vcs_stash_push (gap mentioned in earlier reviews is fixed).

• host_api.rs — global emitter wrapped in RwLock<Option<>>. reset_host_api_state_for_tests() clears both globals, preventing cross-test bleed.

• node_instance/mod.rs — close_vcs_session tests call runtime.close_vcs_session() and assert on mock handler calls (gap from earlier review is fixed).

• ssh.rs — home_dir_for_paths() supports #[cfg(test)] override via TEST_HOME_DIR. All three callers (known_hosts_path, ssh_dir_path, ensure_ssh_dir) are updated.

• themes.rs — load_theme logic in the extracted theme_allowed_for_enabled_plugins matches the original inline code exactly (verified the boolean algebra).

• Frontend source changes — types.d.ts markup field widened to | null, formatTimeAgo catch block is safer, menu.ts context menu errors are logged, dom.ts on() type widened from HTMLElement to Element. All are correct defensive improvements.

• CodeQL findings from both prior CI reviews — unused variables/imports have been removed in the latest commits (More issues fixed, Fixed issues). The current state is clean.

• Test coverage — new tests cover the extracted functions, edge cases, error paths, and mock-based RPC call verification. Patterns follow standard Rust/TypeScript testing conventions.

No further findings.

opencode session  |  github run