perf(ui): use ListBox::remove_all() instead of per-child loop
Replaces five while-loop child removal patterns with the batched remove_all() method available since GTK 4.12. Avoids per-removal layout invalidation.
This commit is contained in:
142
docs/superpowers/specs/2026-03-26-codebase-hardening-design.md
Normal file
142
docs/superpowers/specs/2026-03-26-codebase-hardening-design.md
Normal file
@@ -0,0 +1,142 @@
|
||||
# Codebase Hardening: owlry + owlry-plugins
|
||||
|
||||
**Date:** 2026-03-26
|
||||
**Scope:** 15 fixes across 2 repositories, organized in 5 severity tiers
|
||||
**Approach:** Severity-ordered tiers, one commit per tier, core repo first
|
||||
|
||||
---
|
||||
|
||||
## Tier 1: Critical / Soundness (owlry core)
|
||||
|
||||
### 1a. Replace `static mut HOST_API` with `OnceLock`
|
||||
|
||||
**File:** `crates/owlry-plugin-api/src/lib.rs`
|
||||
**Problem:** `static mut` is unsound — concurrent reads during initialization are UB.
|
||||
**Fix:** Replace with `std::sync::OnceLock<&'static HostAPI>`. `init_host_api()` calls `HOST_API.set(api)`, `host_api()` calls `HOST_API.get().copied()`. No public API changes — convenience wrappers (`notify()`, `log_info()`, etc.) keep working. No ABI impact since `HOST_API` is internal.
|
||||
|
||||
### 1b. Add IPC message size limit
|
||||
|
||||
**File:** `crates/owlry-core/src/server.rs`
|
||||
**Problem:** `BufReader::lines()` reads unbounded lines. A malicious/buggy client can OOM the daemon.
|
||||
**Fix:** Replace the `lines()` iterator with a manual `read_line()` loop enforcing a 1 MB max. Lines exceeding the limit get an error response and the connection is dropped. Constant: `const MAX_REQUEST_SIZE: usize = 1_048_576`.
|
||||
|
||||
### 1c. Handle mutex poisoning gracefully
|
||||
|
||||
**File:** `crates/owlry-core/src/server.rs`
|
||||
**Problem:** All `lock().unwrap()` calls panic on poisoned mutex, crashing handler threads.
|
||||
**Fix:** Replace with `lock().unwrap_or_else(|e| e.into_inner())`. The ProviderManager and FrecencyStore don't have invariants that require abort-on-poison.
|
||||
|
||||
---
|
||||
|
||||
## Tier 2: Security (owlry core)
|
||||
|
||||
### 2a. Set socket permissions after bind
|
||||
|
||||
**File:** `crates/owlry-core/src/server.rs`
|
||||
**Problem:** Socket inherits process umask, may be readable by other local users.
|
||||
**Fix:** After `UnixListener::bind()`, call `std::fs::set_permissions(socket_path, Permissions::from_mode(0o600))`. Uses `std::os::unix::fs::PermissionsExt`.
|
||||
|
||||
### 2b. Log signal handler failure
|
||||
|
||||
**File:** `crates/owlry-core/src/main.rs`
|
||||
**Problem:** `ctrlc::set_handler(...).ok()` silently swallows errors. Failed handler means no socket cleanup on SIGINT.
|
||||
**Fix:** Replace `.ok()` with `if let Err(e) = ... { warn!("...") }`.
|
||||
|
||||
### 2c. Add client read timeout
|
||||
|
||||
**File:** `crates/owlry-core/src/server.rs`
|
||||
**Problem:** A client that connects but never sends data blocks a thread forever.
|
||||
**Fix:** Set `stream.set_read_timeout(Some(Duration::from_secs(30)))` on accepted connections before entering the read loop.
|
||||
|
||||
---
|
||||
|
||||
## Tier 3: Robustness / Quality (owlry core)
|
||||
|
||||
### 3a. Log malformed JSON requests
|
||||
|
||||
**File:** `crates/owlry-core/src/server.rs`
|
||||
**Problem:** JSON parse errors only sent as response to client, not visible in daemon logs.
|
||||
**Fix:** Add `warn!("Malformed request from client: {}", e)` before sending the error response.
|
||||
|
||||
### 3b. Replace Mutex with RwLock for concurrent reads
|
||||
|
||||
**File:** `crates/owlry-core/src/server.rs`
|
||||
**Problem:** `Mutex<ProviderManager>` blocks all concurrent queries even though they're read-only.
|
||||
**Fix:** Replace both `Arc<Mutex<ProviderManager>>` and `Arc<Mutex<FrecencyStore>>` with `Arc<RwLock<...>>`.
|
||||
|
||||
Lock usage per request type:
|
||||
|
||||
| Request | ProviderManager | FrecencyStore |
|
||||
|---------|----------------|---------------|
|
||||
| Query | `read()` | `read()` |
|
||||
| Launch | — | `write()` |
|
||||
| Providers | `read()` | — |
|
||||
| Refresh | `write()` | — |
|
||||
| Toggle | — | — |
|
||||
| Submenu | `read()` | — |
|
||||
| PluginAction | `read()` | — |
|
||||
|
||||
Poisoning recovery: `.unwrap_or_else(|e| e.into_inner())` applies to RwLock the same way.
|
||||
|
||||
---
|
||||
|
||||
## Tier 4: Critical fixes (owlry-plugins)
|
||||
|
||||
### 4a. Fix `Box::leak` memory leak in converter
|
||||
|
||||
**File:** `owlry-plugins/crates/owlry-plugin-converter/src/units.rs`
|
||||
**Problem:** `Box::leak(code.into_boxed_str())` leaks memory on every keystroke for currency queries.
|
||||
**Fix:** Currency codes are already `&'static str` in `CURRENCY_ALIASES`. Change `resolve_currency_code()` return type from `Option<String>` to `Option<&'static str>` so it returns the static str directly. This eliminates the `Box::leak`. Callers in `units.rs` (`find_unit`, `convert_currency`, `convert_currency_common`) and `currency.rs` (`is_currency_alias`) must be updated to work with `&'static str` — mostly removing `.to_string()` calls or adding them at the boundary where `String` is needed (e.g., HashMap lookups that need owned keys).
|
||||
|
||||
### 4b. Fix bookmarks temp file race condition
|
||||
|
||||
**File:** `owlry-plugins/crates/owlry-plugin-bookmarks/src/lib.rs`
|
||||
**Problem:** Predictable `/tmp/owlry_places_temp.sqlite` path — concurrent instances clobber, symlink attacks possible.
|
||||
**Fix:** Append PID and monotonic counter to filename: `owlry_places_{pid}.sqlite`. Uses `std::process::id()`. Each profile copy gets its own name via index. Cleanup on exit remains the same.
|
||||
|
||||
### 4c. Fix bookmarks background refresh never updating state
|
||||
|
||||
**File:** `owlry-plugins/crates/owlry-plugin-bookmarks/src/lib.rs`
|
||||
**Problem:** Background thread loads items and saves cache but never writes back to `self.items`. Current session keeps stale data.
|
||||
**Fix:** Replace `items: Vec<PluginItem>` with `items: Arc<Mutex<Vec<PluginItem>>>`. Background thread writes to the shared vec after completing. `provider_refresh` reads from it. The `loading` AtomicBool already prevents concurrent loads.
|
||||
|
||||
---
|
||||
|
||||
## Tier 5: Quality fixes (owlry-plugins)
|
||||
|
||||
### 5a. SSH plugin: read terminal from config
|
||||
|
||||
**File:** `owlry-plugins/crates/owlry-plugin-ssh/src/lib.rs`
|
||||
**Problem:** Hardcoded `kitty` as terminal fallback. Core already detects terminals.
|
||||
**Fix:** Read `terminal` from `[plugins.ssh]` in `~/.config/owlry/config.toml`. Fall back to `$TERMINAL` env var, then `xdg-terminal-exec`. Same config pattern as weather/pomodoro plugins.
|
||||
|
||||
### 5b. WebSearch plugin: read engine from config
|
||||
|
||||
**File:** `owlry-plugins/crates/owlry-plugin-websearch/src/lib.rs`
|
||||
**Problem:** TODO comment for config reading, never implemented. Engine is always duckduckgo.
|
||||
**Fix:** Read `engine` from `[plugins.websearch]` in config.toml. Supports named engines (`google`, `duckduckgo`, etc.) or custom URL templates with `{query}`. Falls back to duckduckgo.
|
||||
|
||||
### 5c. Emoji plugin: build items once at init
|
||||
|
||||
**File:** `owlry-plugins/crates/owlry-plugin-emoji/src/lib.rs`
|
||||
**Problem:** `load_emojis()` clears and rebuilds ~370 items on every `refresh()` call.
|
||||
**Fix:** Call `load_emojis()` in `EmojiState::new()`. `provider_refresh` returns `self.items.clone()` without rebuilding.
|
||||
|
||||
### 5d. Calculator/Converter: safer shell commands
|
||||
|
||||
**Files:** `owlry-plugin-calculator/src/lib.rs`, `owlry-plugin-converter/src/lib.rs`
|
||||
**Problem:** `sh -c 'echo -n "..."'` pattern with double-quote interpolation. Theoretically breakable by unexpected result formatting.
|
||||
**Fix:** Use `printf '%s' '...' | wl-copy` with single-quote escaping (`replace('\'', "'\\''")`) — the same safe pattern already used by bookmarks and clipboard plugins.
|
||||
|
||||
---
|
||||
|
||||
## Out of scope
|
||||
|
||||
These were identified but deferred:
|
||||
|
||||
- **Hardcoded emoji list** — replacing with a crate/data file is a feature, not a fix
|
||||
- **Plugin vtable-level tests** — valuable but a separate testing initiative
|
||||
- **IPC protocol versioning** — protocol change, not a bug fix
|
||||
- **Plugin sandbox enforcement** — large feature, not a point fix
|
||||
- **Desktop Exec field sanitization** — deep rabbit hole, needs separate design
|
||||
- **Config validation** — separate concern, deserves its own pass
|
||||
161
docs/superpowers/specs/2026-03-26-runtime-integration-design.md
Normal file
161
docs/superpowers/specs/2026-03-26-runtime-integration-design.md
Normal file
@@ -0,0 +1,161 @@
|
||||
# Script Runtime Integration for owlry-core Daemon
|
||||
|
||||
**Date:** 2026-03-26
|
||||
**Scope:** Wire up Lua/Rune script runtime loading in the daemon, fix ABI mismatch, add filesystem-watching hot-reload, update plugin documentation
|
||||
**Repos:** owlry (core), owlry-plugins (docs only)
|
||||
|
||||
---
|
||||
|
||||
## Problem
|
||||
|
||||
The daemon (`owlry-core`) only loads native plugins from `/usr/lib/owlry/plugins/`. User script plugins in `~/.config/owlry/plugins/` are never discovered because `ProviderManager::new_with_config()` never calls the `LoadedRuntime` infrastructure that already exists in `runtime_loader.rs`. Both Lua and Rune runtimes are installed at `/usr/lib/owlry/runtimes/` and functional, but never invoked.
|
||||
|
||||
Additionally, the Lua runtime's `RuntimeInfo` struct has 5 fields while the core expects 2, causing a SIGSEGV on cleanup.
|
||||
|
||||
---
|
||||
|
||||
## 1. Fix Lua RuntimeInfo ABI mismatch
|
||||
|
||||
**File:** `owlry/crates/owlry-lua/src/lib.rs`
|
||||
|
||||
Shrink Lua's `RuntimeInfo` from 5 fields to 2, matching core and Rune:
|
||||
|
||||
```rust
|
||||
// Before (5 fields — ABI mismatch with core):
|
||||
pub struct RuntimeInfo {
|
||||
pub id: RString,
|
||||
pub name: RString,
|
||||
pub version: RString,
|
||||
pub description: RString,
|
||||
pub api_version: u32,
|
||||
}
|
||||
|
||||
// After (2 fields — matches core/Rune):
|
||||
pub struct RuntimeInfo {
|
||||
pub name: RString,
|
||||
pub version: RString,
|
||||
}
|
||||
```
|
||||
|
||||
Update `runtime_info()` to return only 2 fields. Remove the `LUA_RUNTIME_API_VERSION` constant and `LuaRuntimeVTable` (use the core's `ScriptRuntimeVTable` layout — both already match). The extra metadata (`id`, `description`) was never consumed by the core.
|
||||
|
||||
### Vtable `init` signature change
|
||||
|
||||
Change the `init` function in the vtable to accept the owlry version as a second parameter:
|
||||
|
||||
```rust
|
||||
// Before:
|
||||
pub init: extern "C" fn(plugins_dir: RStr<'_>) -> RuntimeHandle,
|
||||
|
||||
// After:
|
||||
pub init: extern "C" fn(plugins_dir: RStr<'_>, owlry_version: RStr<'_>) -> RuntimeHandle,
|
||||
```
|
||||
|
||||
This applies to:
|
||||
- `owlry-core/src/plugins/runtime_loader.rs` — `ScriptRuntimeVTable.init`
|
||||
- `owlry-lua/src/lib.rs` — `LuaRuntimeVTable.init` and `runtime_init()` implementation
|
||||
- `owlry-rune/src/lib.rs` — `RuneRuntimeVTable.init` and `runtime_init()` implementation
|
||||
|
||||
The core passes its version (`env!("CARGO_PKG_VERSION")` from `owlry-core`) when calling `(vtable.init)(plugins_dir, version)`. Runtimes forward it to `discover_and_load()` instead of hardcoding a version string. This keeps compatibility checks future-proof — no code changes needed on version bumps.
|
||||
|
||||
---
|
||||
|
||||
## 2. Change default entry points to `main`
|
||||
|
||||
**Files:**
|
||||
- `owlry/crates/owlry-lua/src/manifest.rs` — change `default_entry()` from `"init.lua"` to `"main.lua"`
|
||||
- `owlry/crates/owlry-rune/src/manifest.rs` — change `default_entry()` from `"init.rn"` to `"main.rn"`
|
||||
|
||||
Add `#[serde(alias = "entry_point")]` to the `entry` field in both manifests so existing `plugin.toml` files using `entry_point` continue to work.
|
||||
|
||||
---
|
||||
|
||||
## 3. Wire runtime loading into ProviderManager
|
||||
|
||||
**File:** `owlry/crates/owlry-core/src/providers/mod.rs`
|
||||
|
||||
In `ProviderManager::new_with_config()`, after native plugin loading:
|
||||
|
||||
1. Get user plugins directory from `paths::plugins_dir()`
|
||||
2. Get owlry version: `env!("CARGO_PKG_VERSION")`
|
||||
3. Try `LoadedRuntime::load_lua(&plugins_dir, version)` — log at `info!` if unavailable, not error
|
||||
4. Try `LoadedRuntime::load_rune(&plugins_dir, version)` — same
|
||||
5. Call `create_providers()` on each loaded runtime
|
||||
6. Feed runtime providers into existing categorization (static/dynamic/widget)
|
||||
|
||||
`LoadedRuntime::load_lua`, `load_rune`, and `load_from_path` all gain an `owlry_version: &str` parameter, which is passed to `(vtable.init)(plugins_dir, owlry_version)`.
|
||||
|
||||
Store `LoadedRuntime` instances on `ProviderManager` in a new field `runtimes: Vec<LoadedRuntime>`. These must stay alive for the daemon's lifetime (they own the `Library` handle via `Arc`).
|
||||
|
||||
Remove `#![allow(dead_code)]` from `runtime_loader.rs` since it's now used.
|
||||
|
||||
---
|
||||
|
||||
## 4. Filesystem watcher for automatic hot-reload
|
||||
|
||||
**New file:** `owlry/crates/owlry-core/src/plugins/watcher.rs`
|
||||
**Modified:** `owlry/crates/owlry-core/src/providers/mod.rs`, `Cargo.toml`
|
||||
|
||||
### Dependencies
|
||||
|
||||
Add to `owlry-core/Cargo.toml`:
|
||||
```toml
|
||||
notify = "7"
|
||||
notify-debouncer-mini = "0.5"
|
||||
```
|
||||
|
||||
### Watcher design
|
||||
|
||||
After initializing runtimes, spawn a background watcher thread:
|
||||
|
||||
1. Watch `~/.config/owlry/plugins/` recursively using `notify-debouncer-mini` with 500ms debounce
|
||||
2. On debounced event (any file create/modify/delete):
|
||||
- Acquire write lock on `ProviderManager`
|
||||
- Remove all runtime-backed providers from the provider vecs
|
||||
- Drop old `LoadedRuntime` instances
|
||||
- Re-load runtimes from `/usr/lib/owlry/runtimes/` with fresh plugin discovery
|
||||
- Add new runtime providers to provider vecs
|
||||
- Refresh the new providers
|
||||
- Release write lock
|
||||
|
||||
### Provider tracking
|
||||
|
||||
`ProviderManager` needs to distinguish runtime providers from native/core providers for selective removal during reload. Options:
|
||||
|
||||
- **Tag-based:** Runtime providers already use `ProviderType::Plugin(type_id)`. Keep a `HashSet<String>` of type_ids that came from runtimes. On reload, remove providers whose type_id is in the set.
|
||||
- **Separate storage:** Store runtime providers in their own vec, separate from native providers. Query merges results from both.
|
||||
|
||||
**Chosen: Tag-based.** Simpler — runtime type_ids are tracked in a `runtime_type_ids: HashSet<String>` on `ProviderManager`. Reload clears the set, removes matching providers, then re-adds.
|
||||
|
||||
### Thread communication
|
||||
|
||||
The watcher thread needs access to `Arc<RwLock<ProviderManager>>`. The `Server` already holds this Arc. Pass a clone to the watcher thread at startup. The watcher acquires `write()` only during reload (~10ms), so read contention is minimal.
|
||||
|
||||
### Watcher lifecycle
|
||||
|
||||
- Started in `Server::run()` (or `Server::bind()`) before the accept loop
|
||||
- Runs until the daemon exits (watcher thread is detached or joined on drop)
|
||||
- Errors in the watcher (e.g., inotify limit exceeded) are logged and the watcher stops — daemon continues without hot-reload
|
||||
|
||||
---
|
||||
|
||||
## 5. Plugin development documentation
|
||||
|
||||
**File:** `owlry-plugins/docs/PLUGIN_DEVELOPMENT.md`
|
||||
|
||||
Cover:
|
||||
- **Plugin directory structure** — `~/.config/owlry/plugins/<name>/plugin.toml` + `main.lua`/`main.rn`
|
||||
- **Manifest reference** — all `plugin.toml` fields (`id`, `name`, `version`, `description`, `entry`/`entry_point`, `owlry_version`, `[[providers]]` section, `[permissions]` section)
|
||||
- **Lua plugin guide** — `owlry.provider.register()` API with `refresh` and `query` callbacks, item table format (`id`, `name`, `command`, `description`, `icon`, `terminal`, `tags`)
|
||||
- **Rune plugin guide** — `pub fn refresh()` and `pub fn query(q)` signatures, `Item::new()` builder, `use owlry::Item`
|
||||
- **Hot-reload** — changes are picked up automatically, no daemon restart needed
|
||||
- **Examples** — complete working examples for both Lua and Rune
|
||||
|
||||
---
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Config-gated runtime loading (runtimes self-skip if `.so` not installed)
|
||||
- Per-plugin selective reload (full runtime reload is fast enough)
|
||||
- Plugin registry/installation (already exists in the CLI)
|
||||
- Sandbox enforcement (separate concern, deferred from hardening spec)
|
||||
Reference in New Issue
Block a user