diff --git a/.gitignore b/.gitignore index c9612bb..04b177c 100644 --- a/.gitignore +++ b/.gitignore @@ -28,6 +28,7 @@ src-tauri/target/ # Claude Code local configuration .claude/ +CLAUDE.local.md # Environment .env diff --git a/CHANGELOG.md b/CHANGELOG.md index 68d1e6e..bfc62a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 --- +## [1.0.2] - 2026-08-04 + +### Changed + +- **MCP: unified the environment-scope parameter name on `environment_id`** (issue #10). `crypt_env_inject_environment` and `crypt_env_generate_example_env` were the only two MCP tools naming the environment identifier `id` instead of `environment_id`, matching every other environment-scoped tool. This let an LLM caller that inferred the parameter name from the majority pass `environment_id` to these two tools and have it silently ignored — in the case where `project`+`environment` were also present, the resolver would fall through and inject a *different* environment's full decrypted variable set with no error. Both schemas now advertise `environment_id` with the canonical description used by every other scoped tool; the ambiguity error in `crypt_env_inject_env_by_name` now names `environment_id` instead of `id` as well. + - The bare `id` key is accepted as an unadvertised, deprecated alias for the whole 1.0.x line (`// DEPRECATED(remove in 1.1.0): environment 'id' alias, issue #10`). Calls using `id` still succeed, but the response text appends a fixed deprecation notice so the model is told to switch. Removed in 1.1.0. + - New in-crate tests (`src-tauri/src/bin/crypt-env-mcp.rs`, `#[cfg(test)] mod tests`) assert every environment-scoped tool declares `environment_id` and not a bare `id`, that the five item/category/workspace tools keep their bare `id`, and that the resolver prefers `environment_id` over the alias. + - `docs/reference.md` updated to drop the "inconsistent naming" note and describe the resolved state + deprecation window. + ## [1.0.1] - 2026-07-28 ### Fixed diff --git a/README.md b/README.md index 7e3dd32..5a9c026 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,13 @@ crypt-env workspace inject "my-node-project" Via UI: Settings → Workspaces → [select workspace] → **INJECT TO PATH** +### WSL paths (Windows only) + +If your project's source lives inside a WSL2 distro, its `.env` file is reachable from Windows at `\\wsl.localhost\\...` — the environment path picker has a **WSL** button (next to the regular browse button) that lists your installed distros and opens the file dialog right there, so you don't have to type the UNC path or know your distro's exact registered name. Two things to know: + +- Browsing (or injecting to) a WSL path **starts the distro** if it's stopped — this can take a few seconds and uses memory, the same as running `wsl` from a terminal would. +- A stopped distro must be running for **inject** to succeed too — if the distro isn't up, the write fails with an error instead of silently succeeding. + --- ## 💻 Interactive TUI @@ -169,6 +176,8 @@ Useful for SSH sessions, CI/CD scripts, or environments where a GUI is unavailab - **macOS**: Xcode Command Line Tools (`xcode-select --install`) - **Linux**: `libwebkit2gtk-4.1-dev`, `libgtk-3-dev`, `libayatana-appindicator3-dev`, `librsvg2-dev` +> **Known issue (Linux):** the mouse cursor may render oversized inside the app window on some Linux setups. Root cause is not yet confirmed — diagnosis is blocked on a native X11/Wayland desktop session (WSLg is not a valid repro/verification environment for this). No workaround is documented yet. Track status in [issue #6](https://github.com/maosuarez/crypt-env/issues/6). + ### Install & Run ```bash diff --git a/context.md b/context.md index 9d6e841..86c22c5 100644 --- a/context.md +++ b/context.md @@ -30,6 +30,7 @@ crypt-env/ │ │ ├── crypto/mod.rs # Argon2id KDF + AES-256-GCM encrypt/decrypt │ │ ├── vault/mod.rs # VaultState, Tauri commands for vault management │ │ ├── project/mod.rs # Project/environment business logic (shared by Tauri + HTTP API) +│ │ ├── wsl/mod.rs # WSL bridge (Phase 1): distro discovery + UNC seed dir for the env path picker │ │ ├── api/mod.rs # Axum server on 127.0.0.1:47821, dual token auth │ │ ├── share/mod.rs # Secure secret sharing (LAN bridge + encrypted packages) │ │ ├── share/relay.rs # Internet relay sharing (Supabase-based) @@ -54,6 +55,7 @@ crypt-env/ **New Tauri Commands** (Session 4+): - Projects/Environments: `project_list`, `project_save`, `project_delete`, `project_preview_delete`, `environment_save`, `environment_delete`, `environment_inject`, `vault_create_project_item`, `vault_set_item_global`, `vault_get_item_owners` - Internet Relay: `share_relay_send`, `share_relay_receive` +- WSL bridge (issue #3, Phase 1, Windows-only affordance): `wsl_list_distros() → Vec` (always `Ok`, empty on non-Windows/no-WSL), `wsl_distro_home(distro: String) → Result` (seed dir for the env path picker). `project_pick_env_path` gained an optional `start_dir: Option` argument to seed the native dialog at that directory ## Vault Item Types 1. **Secret / API Key**: name, encrypted value, category, notes. Export as `.env` / `export` / `$env:` @@ -872,6 +874,34 @@ CREATE TABLE settings ( --- +### 16. WSL Bridge for the Environment Path Picker (Issue #3, Phase 1) +**Context**: Windows developers who keep their project source inside a WSL2 distro (this repo itself, per `CLAUDE.local.md`) can already attach a WSL `.env` to an environment by typing its `\\wsl.localhost\\...` UNC path into the existing manual path input — but nothing in the UI suggests this is possible, the user must know their distro's exact *registered* name, and a typo or a stopped distro previously surfaced as a confusing later failure. This is discoverability and pre-flight validation, not new capability — see the plan's own accounting in `docs/plans/issue-3-wsl-bridge-env-paths.md` §2. + +**Decision**: Add a "Browse WSL" affordance next to the existing browse button in the environment editor's PATHS block, backed by two new Windows-only Tauri commands in a dedicated `wsl` module. Phase 2 (a CLI running inside WSL talking back to the vault) is explicitly deferred, uncommitted work (plan §6). + +1. **New module** `src-tauri/src/wsl/mod.rs` — decoupled from `db`/`vault`/`api`/`project`; only the frontend composes it with `project_pick_env_path`. + - `parse_distro_list(&[u8]) -> Vec` — pure. Decodes `wsl.exe --list --quiet` stdout: UTF-8 BOM, UTF-16LE BOM, BOM-less UTF-16LE (heuristic: every second byte of the first 32 is `0x00`), or UTF-8 lossy fallback. 11 unit tests in-file. + - `unc_root(distro)` / `unc_root_legacy(distro)` — pure, return `\\wsl.localhost\\` / `\\wsl$\\`. + - `wsl_list_distros() → Result, String>` (Tauri command) — spawns `wsl.exe --list --quiet` via `tokio::process::Command` + `kill_on_drop` under a 10 s `tokio::time::timeout`. Returns `Ok(vec![])` — never `Err` — when `wsl.exe` is absent, WSL has zero distros, or the target isn't Windows; `Err` only on timeout or a genuine non-`NotFound` spawn error. + - `wsl_distro_home(distro) → Result` (Tauri command) — validates the argument (no `\`, `/`, `..`, NUL), probes `\\wsl.localhost\\home\` then falls back once to `\\wsl$\\home\` (each probe `spawn_blocking` + 10 s timeout, since a UNC probe against a cold distro blocks). Descends into the single child directory if `read_dir` finds exactly one, else returns `...\home\`. Touching the UNC path starts a stopped distro — documented in the button's tooltip. +2. **`project_pick_env_path`** (`src-tauri/src/project/mod.rs`) gained an optional `start_dir: Option` argument; when given, calls `.set_directory()` on the `rfd::FileDialog` builder before `pick_file()`. The other four `rfd` call sites (`project_export`, `project_import`, the two in `share_commands.rs`) are untouched. +3. **Frontend** (`src/components/ProjectManager.tsx`): `isWindows` detected via `@tauri-apps/plugin-os` `platform()`, mirroring `WindowChrome.tsx`. `wsl_list_distros` fetched once when `isWindows` becomes true, held in local component state (not Zustand, not TanStack Query — ephemeral machine state). A "WSL" button (terminal icon) renders in the PATHS row only when `isWindows && wslDistros.length > 0`: one distro acts directly, several open a small inline dropdown (same panel styling as the existing tag filter). Click → `wsl_distro_home(distro)` → `project_pick_env_path({ startDir })`, reusing the same picked-path handling as the plain browse button (`applyPickedEnvPath`, extracted from the old `handlePickEnvPath`) → failure shows a toast and falls back to the manual input. +4. **Data-loss gate landed alongside this** (plan §3.6, cross-cutting with issues #7/#8): `inject_environment`'s read of each target path (`src-tauri/src/project/mod.rs`) previously did `std::fs::read_to_string(path).unwrap_or_default()`, treating *any* read failure — including a stopped WSL distro's dead 9p mount — as "file is empty," then silently overwriting it with only this environment's keys. Changed to a `match e.kind()` guard: `ErrorKind::NotFound` still starts from empty (new file); every other error kind aborts the write with `Err`. This is the single most likely real-world failure mode of attaching a WSL path, and the plan declared Phase 1 not closeable without it. + +**Rationale**: +- `--list --quiet` over the issue-specified `--list --verbose`: verbose output is column-aligned, carries a localized `Running`/`Stopped`/default-marker, and breaks on non-English Windows; quiet emits one bare name per line and needs no state (selecting a distro starts it anyway). +- `tokio::process::Command` (not `spawn_blocking` + `std::process::Command`, the pattern already used for `rfd` in this file) because a `timeout` around a blocking-pool `JoinHandle` doesn't cancel the blocked thread — it leaks a pool slot for as long as `wsl.exe` hangs. `tokio::process` cancels and kills the child on drop. +- No new Cargo dependency: hand-rolled UTF-16LE decode (~10 lines) instead of `encoding_rs`, since exactly two known encodings from one known producer don't justify a crate; `tauri-plugin-shell` (listed in this file's own stack-setup checklist, never actually added — see `src-tauri/Cargo.toml`) is deliberately not added either, since it would let the webview spawn processes for no gain when Rust can already do so natively. +- `\\wsl.localhost\` primary / `\\wsl$\` fallback (not build-number sniffing): two cheap filesystem probes beat silently guessing wrong on older Windows builds. + +**Consequences**: +- Windows-only feature; the command surface still registers on all targets (honest `Ok(vec![])` elsewhere) so there is one `invoke_handler!` list, not a per-platform one. +- Clicking "Browse WSL" can cold-boot a stopped WSL VM (several seconds, memory cost) — inherent to the UNC bridge, stated in the tooltip, not eagerly triggered (only on explicit click). +- `docker-desktop`/`docker-desktop-data` distros are not filtered out of the list (would hardcode a vendor's naming into the parser); users recognize their own distros. +- Requires manual verification on real Windows + WSL2 hardware (checklist M1–M10 in the plan) — not automatable in CI, since CI has no WSL. Automated coverage is limited to the pure `parse_distro_list`/`unc_root` functions and the `inject_environment` read-guard (both covered by `cargo test`). + +--- + ## Security Status (post-review 2026-04-24) A **comprehensive security review** was performed that identified **19 findings** (7 HIGH, 8 MEDIUM, 4 LOW). **All findings have been addressed**. diff --git a/docs/plans/issue-10-mcp-environment-id-param-naming.md b/docs/plans/issue-10-mcp-environment-id-param-naming.md new file mode 100644 index 0000000..5d6533f --- /dev/null +++ b/docs/plans/issue-10-mcp-environment-id-param-naming.md @@ -0,0 +1,383 @@ +# Issue #10 — MCP: unify the environment-scope parameter name on `environment_id` + +**Type:** bug (interface naming) · **Surface:** MCP server only (`src-tauri/src/bin/crypt-env-mcp.rs`) · **Status:** plan, not implemented + +--- + +## 1. Objective + +`crypt_env_inject_environment` and `crypt_env_generate_example_env` are the only two MCP +tools that name the environment identifier `id`. Every other environment-scoped tool names +it `environment_id`. This plan makes `environment_id` the single advertised name for +"an environment identifier" across the whole tool list, keeps `id` working as an +**unadvertised** alias for the 1.0.x line only, and adds a regression test over the static +tool-list JSON so the divergence cannot come back. + +### Definition of done + +**Schemas changed (exactly two).** In `tool_definitions()` (`src-tauri/src/bin/crypt-env-mcp.rs:195`): + +| Tool | Schema at | Change | +|---|---|---| +| `crypt_env_inject_environment` | `:519-531` | property `id` → `environment_id`; `project`, `environment`, top-level `description` re-worded to the canonical strings | +| `crypt_env_generate_example_env` | `:533-545` | identical change | + +No other tool's schema is touched. `crypt_env_get_item`, `crypt_env_update_item`, +`crypt_env_delete_item`, `crypt_env_update_category`, `crypt_env_delete_category` and +`crypt_env_share_workspace_send` keep their bare `id` — there it means an *item*, +*category* or *workspace* id and is correct. + +**Canonical strings** (already byte-identical across all 13 conforming tools — verified at +`:205/229/243/257/278/343/361/379/397/411/463/569/690`; the implementation copies them +verbatim, it does not paraphrase): + +``` +environment_id → "Environment ID (scope). Provide this, or both 'project' and 'environment'." +project → "Project name (case-insensitive). Used with 'environment' when 'environment_id' is not given." +environment → "Environment name within the project (case-insensitive), e.g. production, local, test. Used with 'project'." +``` + +**Resolver behaviour** (`resolve_environment_id`, `:1845`), in strict order: + +1. `args["environment_id"]` as `i64` → resolve, no deprecation flag. +2. else `args["id"]` as `i64` → resolve, **flag as deprecated** (not silent — see below). +3. else `project` + `environment` name pair → `fetch_projects()` + case-insensitive match (unchanged). +4. else → `Err(tool_err("required: 'environment_id' (environment id), or 'project' + 'environment' (names)"))`. + +The alias is **not silent**: the resolver returns a small struct rather than a bare `i64`, and +both call sites append a one-line deprecation notice to the *successful* tool response text. +The MCP protocol gives no other warning channel, and the response text is what the model +actually reads — a silent alias would keep teaching the wrong name indefinitely. + +```rust +/// Outcome of resolving an environment identifier from MCP tool args. +struct ResolvedEnvironment { + id: i64, + /// True when the caller used the deprecated `id` key instead of `environment_id`. + /// Callers surface this to the model; remove together with the alias in 1.1.0. + via_deprecated_id: bool, +} +``` + +Notice text (fixed string, contains no identifiers and no secret material): +`note: parameter 'id' is deprecated on this tool; use 'environment_id' instead.` + +**Deprecation window.** The alias is accepted for the whole **1.0.x** line and **removed in +1.1.0**. Marked in-source with `// DEPRECATED(remove in 1.1.0): environment `id` alias, issue #10` +on the branch, and recorded in `CHANGELOG.md` under both the 1.0.2 entry (alias added, +schema renamed) and — when it lands — the 1.1.0 entry (alias removed). Rationale for a short +window in §4. + +**Error strings changed (two).** +- `:1856` → `"required: 'environment_id' (environment id), or 'project' + 'environment' (names)"` +- `:2736` → `"... Use crypt_env_inject_environment with a specific 'environment_id'."` + +**Docs changed.** +- `docs/reference.md:245` and `:246` — parameter column `id` → `environment_id`; drop the + trailing "Note: uses `id`, not `environment_id` like every other scoped tool above — see Notes". +- `docs/reference.md:274` — the "**Scope-parameter naming is inconsistent across tools**" + note is replaced by a resolved note stating that all environment-scoped tools now use + `environment_id`, that `id` is a temporary undocumented alias, and when it is removed. +- `CHANGELOG.md` — new `## [1.0.2]` **Changed** entry. +- `context.md:742` needs **no** change (lists tool names only, no parameters). +- `CLAUDE.md` needs no change. + +**Tests (new file section — the MCP binary currently has zero tests).** A +`#[cfg(test)] mod tests` block at the end of `src-tauri/src/bin/crypt-env-mcp.rs`, run by +`cargo test --bin crypt-env-mcp` (and by a plain `cargo test`). Five assertions: + +1. `every_environment_scoped_tool_declares_environment_id` — iterate `tool_definitions()`; + treat a tool as environment-scoped iff its `inputSchema.properties` contains **both** + `project` and `environment` (structurally exact today: 15 tools — the 13 conforming plus + the 2 outliers; `crypt_env_inject_env_by_name` is excluded because it uses `project_path`, + not `project`). For each: assert `properties.environment_id` **exists** and + `properties.id` **does not exist**. This is the literal check the issue asks for. +2. `environment_scope_descriptions_are_canonical` — for the same set, assert the three + description strings equal the canonical strings above, byte for byte. +3. `id_tools_keep_bare_id` — assert `crypt_env_get_item`, `crypt_env_update_item`, + `crypt_env_delete_item`, `crypt_env_update_category`, `crypt_env_delete_category` still + declare `id` and do **not** declare `environment_id`. Guards against an over-eager + find-and-replace. +4. `resolve_environment_id_prefers_canonical_key` — three pure cases, **no network**: + `{"environment_id": 7}` → `Ok(id: 7, via_deprecated_id: false)`; + `{"id": 7}` → `Ok(id: 7, via_deprecated_id: true)`; + `{"environment_id": 7, "id": 9}` → `Ok(id: 7, via_deprecated_id: false)`. + Safe because `fetch_projects()` is only reached on the name-pair path; `token` is unused + on these branches, so `""` is passed. +5. `resolve_environment_id_missing_scope_names_the_canonical_key` — `{}` → `Err`, and the + error text contains `environment_id` and does **not** contain the substring `'id' (environment id)`. + +Done means: `cargo test --bin crypt-env-mcp` passes, `cargo clippy` clean, and +`grep -n "'id'" src-tauri/src/bin/crypt-env-mcp.rs` returns no hit whose subject is an +environment. + +--- + +## 2. What is being mitigated + +**Risk removed:** an LLM caller that infers the parameter name from the 13-tool majority +passes `environment_id` to the two outliers. Today that key is ignored, and one of two +things happens: + +- *Loud, recoverable:* no `project`/`environment` present → the call fails with + `required: 'id' ...`, which contradicts the name the model just learned from every other + tool. Wasted turns, model likely retries with the same wrong key. +- *Silent, not recoverable:* `project` and `environment` **are** also present → the resolver + ignores `environment_id` entirely and resolves a **different environment by name**. + `crypt_env_inject_environment` then writes that environment's full decrypted variable set + to disk. The caller is never told its explicit identifier was discarded. This is the case + that matters: a wrong-environment secret dump with no error. + +A second, narrower hazard is closed by the same change: because `id` is simultaneously the +*correct* name for item ids and category ids on five neighbouring tools, an id copied from +`crypt_env_get_item` and passed as `id` to `crypt_env_inject_environment` is accepted as an +environment id today. After this change, `id` is no longer advertised anywhere on these two +tools, so no model is taught the collision; during the 1.0.x alias window the behaviour is +unchanged but the response now says the key is deprecated, and from 1.1.0 the call fails loudly. + +**Checkable statement:** after this change, for every tool in the advertised +`tools/list` payload, the property named `id` never denotes an environment, and every tool +that accepts an environment scope declares `environment_id` with identical wording — asserted +by tests 1–3, which fail the build if either invariant is violated. + +**Explicitly not mitigated:** the pre-existing "one call can dump a whole environment to an +arbitrary path" concern documented at `docs/reference.md:266`. This plan does not widen or +narrow that surface; it only fixes which key selects the environment. + +--- + +## 3. Implementation steps + +Ordered. Each step is independently compilable; steps 1–3 are one commit, 4 is a second, 5 a third. + +### Step 1 — resolver (`src-tauri/src/bin/crypt-env-mcp.rs:1842-1884`) + +1.1 Add `struct ResolvedEnvironment { id: i64, via_deprecated_id: bool }` immediately above +`resolve_environment_id` (private to the binary; no `#[derive]` needed beyond `Debug` for +test assertions). + +1.2 Change the signature to +`fn resolve_environment_id(args: &serde_json::Value, token: &str) -> Result`. +Keep the name — it is accurate and renaming it churns two call sites plus the doc comment for +no gain. + +1.3 Replace the first branch (`:1846-1848`) with the two-key lookup, canonical key first, +alias second, alias flagged. Do **not** collapse them into +`.get("environment_id").or_else(|| args.get("id"))` — that form cannot distinguish which key +was used and therefore cannot drive the deprecation notice. + +1.4 Update the `Err` at `:1855-1858` to the new message. + +1.5 Update the doc comment at `:1842-1844`: it currently says args are "shaped like +`crypt_env_inject_environment`'s schema: `id` directly". Rewrite to name `environment_id` +as canonical, `id` as deprecated-until-1.1.0, and add the +`// DEPRECATED(remove in 1.1.0): environment 'id' alias, issue #10` marker on the alias branch. +`unwrap()` is not introduced anywhere; all lookups stay `Option`-chained per CLAUDE.md. + +### Step 2 — call sites + +2.1 `tool_inject_environment` (`:1886-1890`) — destructure the struct; keep the local named +`environment_id` so the `/environments/{environment_id}/inject` format string at `:1901` and +the 404 message at `:1916` are untouched. + +2.2 `tool_generate_example_env` (`:1929-1933`) — same, for `/environments/{id}/example` at `:1944`. + +2.3 In both, when `via_deprecated_id` is true, append the fixed notice line to the success +text. Both functions end in the same shape (`tool_ok(pretty)` / `tool_ok(text)`); append to +the string passed to `tool_ok`, not to the parsed JSON, so the API payload shape is unchanged. +Do **not** append on the error paths — error text is already terminal and the deprecation is +not the cause. The notice is a compile-time constant string: no argument interpolation, so no +path, key, or value can leak into it (CLAUDE.md security rule). + +### Step 3 — schemas and the stale error string + +3.1 `:519-531` (`crypt_env_inject_environment`) — rename the property, swap in the three +canonical description strings, and re-word the tool-level `description` at `:520` from +"Identify by environment id, or by 'project' + 'environment' names." to +"Requires scope: 'environment_id', or both 'project' and 'environment'." — matching the +phrasing already used at `:198`. Leave `output_path` / `output_dir` alone. Do not add a +`"required"` array (see §4). + +3.2 `:533-545` (`crypt_env_generate_example_env`) — identical treatment at `:534`; keep the +"Never reads or returns secret values" sentence, it is load-bearing. + +3.3 `:2736` — the ambiguity error inside the `crypt_env_inject_env_by_name` path still tells +the caller to use `crypt_env_inject_environment` "with a specific 'id'". Change to +`'environment_id'`. This string is why the bug survives even for callers who read errors carefully. + +3.4 Sweep: `grep -n "'id'" src-tauri/src/bin/crypt-env-mcp.rs` and confirm every remaining hit +refers to an item, category, or workspace id. + +### Step 4 — tests (`src-tauri/src/bin/crypt-env-mcp.rs`, new trailing `#[cfg(test)] mod tests`) + +4.1 Placement rationale: `crypt-env-mcp` is a `[[bin]]` (`src-tauri/Cargo.toml:106-107`), so +`tool_definitions()` and `resolve_environment_id()` are **not** reachable from +`src-tauri/tests/*.rs` — an integration test there can only spawn the binary. An in-file +`#[cfg(test)] mod tests` is the only zero-restructuring option and needs no new dev-dependency +(`serde_json` is already a normal dep; no `tokio`, no `tempfile`, no process spawn, no network). + +4.2 Write a `fn environment_scoped_tools() -> Vec<&serde_json::Value>` test helper applying +the `has "project" && has "environment"` predicate, and a +`const CANON_ENVIRONMENT_ID_DESC/CANON_PROJECT_DESC/CANON_ENVIRONMENT_DESC`. Assert the helper +returns exactly 15 tools — a bare count check that fails loudly if someone adds a scoped tool +without the canonical shape, which is the failure mode this whole issue is about. + +4.3 Implement tests 1–5 from §1. Failure messages must name the offending tool +(`assert!(..., "tool {name} declares a bare 'id' for an environment")`) — a bare +`assert_eq!` on a 15-element set is unactionable. + +4.4 **Dependency on issue #11.** #11 is planning a general test harness for this repo. Nothing +here blocks on it: these five tests are pure functions over a JSON literal, need no vault, no +`ApiState`, no temp dir, and no async runtime. If #11 lands first and establishes a shared +location for MCP tests, move the module there unchanged; if it lands second, it should adopt +this module as-is rather than duplicating it. Coordinate only to avoid two people writing the +same tool-list assertions. + +### Step 5 — documentation + +5.1 `docs/reference.md:245`, `:246` — parameter column and trailing note (see §1). + +5.2 `docs/reference.md:274` — replace the inconsistency note with: +what the canonical name is, that `id` is accepted-but-unadvertised through 1.0.x, that it is +removed in 1.1.0, and a pointer to the test that enforces it. Keeping a note here (rather than +deleting the paragraph) is deliberate: readers of older transcripts need to know why they saw `id`. + +5.3 `docs/reference.md:272` — the `crypt_env_inject_environment` note says "resolves environment +by ID or by project+environment names"; make "ID" read `environment_id` for consistency. + +5.4 `CHANGELOG.md` — new `## [1.0.2]` section, `### Changed`, describing the rename, the silent +wrong-environment failure it removes, the alias, and its removal target. Note that `1.0.1`'s +entries stay untouched. + +5.5 Do **not** create any additional `.md` file. CLAUDE.md forbids proliferating docs; this +plan file plus the two existing docs is the whole documentation footprint. + +### Verification + +```bash +cd src-tauri && cargo test --bin crypt-env-mcp && cargo clippy --bin crypt-env-mcp -- -D warnings +``` +Manual smoke (Windows side, per CLAUDE.local.md): with the vault unlocked, call +`crypt_env_generate_example_env` with `{"environment_id": }` (expect success, no notice), +then with `{"id": }` (expect success **plus** the deprecation line), then with +`{"environment_id": , "project": "X", "environment": "Y"}` where X/Y is a *different* +environment (expect the `environment_id` environment — the exact case that silently +mis-resolves today). Use `generate_example_env`, not `inject_environment`, for the manual +check: it writes placeholders only and never decrypts, so a mis-resolution during testing +cannot spill real secrets. + +--- + +## 4. Trade-offs and alternatives considered + +### 4.1 Deprecation window: alias for one release *(chosen)* vs. drop outright vs. keep forever + +**Chosen: accept `id` through 1.0.x, remove in 1.1.0, never advertise it.** + +- *Drop outright.* Genuinely attractive, and the strongest argument for it is real: while the + alias lives, an item id mistakenly passed as `id` still resolves to an unrelated environment. + Rejected because the cost of keeping it is one branch and one bool, and because the failure + it prevents (an agent mid-task whose next call suddenly errors) is invisible to us and + annoying to the user. The deprecation notice narrows the gap: the model is told the key is + wrong on the very call that used it. +- *Keep forever.* Rejected. A permanent alias means `id` permanently means two different things + in the same tool list, and dead compatibility branches never get removed once nobody remembers + why they exist. The dated `DEPRECATED(remove in 1.1.0)` marker plus the CHANGELOG entry is + the mechanism that makes removal actually happen. +- *Cost accepted:* one extra struct, one bool threaded through two call sites, and a mandatory + follow-up commit in 1.1.0. If that follow-up is skipped, we are back at "keep forever" by + neglect — that is the main risk of this choice and the reason the marker is dated rather than + a vague "TODO". + +Note the compat surface really is small: there is no MCP protocol versioning here, the tool list +is a static literal, and MCP clients re-fetch `tools/list` on connect. The only exposure is a +transcript already in flight — minutes, not releases. That is precisely why the window is one +minor version and not more. + +### 4.2 Unify `resolve_environment_id` with `append_scope_params` *(rejected — scope creep)* + +The two functions duplicate the case-insensitive project/environment fallback, and it is +tempting to extract one canonical scope parser. Rejected for this issue: + +- They have genuinely different shapes and different *costs*. `append_scope_params` (`:798`) + builds URL query params and is a pure string operation that pushes resolution to the API. + `resolve_environment_id` (`:1845`) must produce a concrete `i64` because its two callers hit + **path-param** routes (`POST /environments/:id/inject`, `POST /environments/:id/example` — + `src-tauri/src/api/mod.rs:3085-3086`), so it performs an HTTP `fetch_projects()` round-trip. + A single function returning "either an id or some query params" is a worse abstraction than + the two honest ones. +- The bug is a naming bug. Merging two resolution strategies inside the fix means the diff is + no longer reviewable as "one key renamed", and a regression in scope resolution is far more + expensive than the inconsistency it would clean up. +- The duplication is small (~15 lines) and now covered by tests 1–2 at the schema level, which + is where the divergence actually hurt. + +If unification is still wanted, it belongs in a separate issue *after* §4.3 is decided — because +if the two outliers stop resolving client-side, `resolve_environment_id` disappears entirely and +there is nothing left to unify. + +### 4.3 Push resolution server-side and delete the client-side resolver *(rejected here, worth a separate issue)* + +The API already resolves scope from `environment_id` **or** `project`+`environment` via +`resolve_scope` / `project::resolve_environment` (`src-tauri/src/api/mod.rs:214-245`), using the +shared `EnvScopeQuery` extractor. If the two outlier tools forwarded the raw scope args instead +of pre-resolving, `resolve_environment_id` and its `fetch_projects()` round-trip both vanish, and +scope semantics would live in exactly one place (the API) instead of two. + +**Cost, stated plainly:** `/environments/:id/inject` and `/environments/:id/example` are +**path-param** routes — `handle_inject_environment` and `handle_environment_example` take +`Path(id): Path` (`api/mod.rs:2100-2104`, `:2162-2166`). There is no id-less variant. Doing +this requires either (a) new routes (`POST /environments/inject` with `Query`), +leaving two ways to call the same operation, or (b) changing the existing routes, which breaks the +CLI and any external caller of the local REST API. Both are API-surface changes needing their own +review, their own tests, and their own CHANGELOG entry — for a bug whose entire content is a +misspelled JSON key. Rejected as out of scope; recommended as a follow-up issue, with (a) plus a +deprecation of the path-param form as the likelier shape. + +### 4.4 Add a `"required"` array to the two schemas *(rejected)* + +Neither outlier schema declares `required` — resolution is entirely runtime. Adding +`"required": ["environment_id"]` would let the client validate, but it is **wrong**: the +`project`+`environment` pair is an equally valid way to identify the environment, and JSON Schema +`oneOf`/`anyOf` across property groups is exactly the kind of cleverness CLAUDE.md's simplicity +rule pushes back on — MCP clients vary in how much of it they enforce. The 13 conforming tools +also omit `required` for the same reason. Consistency wins; the runtime error message (now naming +`environment_id`) is the contract. + +### 4.5 Test location: in-binary unit tests *(chosen)* vs. move `tool_definitions()` into `crypt_env_lib::mcp` + +`src-tauri/src/mcp/mod.rs` exists and is empty (already declared at `src-tauri/src/lib.rs:9`), so +moving `tool_definitions()` there would cost nothing structurally and would make the tool list a +first-class library artifact testable from `src-tauri/tests/`, alongside `vault_integration.rs`. + +Rejected for this pass: it moves ~600 lines of `json!` literal across a module boundary inside a +naming-bug fix, inflating the diff by an order of magnitude and burying the four lines that +actually matter. The in-binary `#[cfg(test)] mod tests` gets the identical assertions with a +~60-line diff and no restructuring. Recommended as a candidate for issue #11's harness work, +where a schema-move is on-topic and reviewable on its own merits. + +### 4.6 Also rename the CLI's `--id` flag *(out of scope)* + +`project inject --id` is the convention the MCP schema was derived from (see the comment at +`api/mod.rs:214-220`). It is **not** renamed here: a CLI flag is read by a human from `--help` +next to `--project`/`--environment`, not inferred by a model from twelve sibling schemas, so it +does not carry the failure mode this issue describes. Renaming it would be a user-visible breaking +change to an unrelated surface. + +### 4.7 Echo the resolved environment name in the response *(noted, not done)* + +A numeric id gives the caller no confirmation of *which* environment was touched. Echoing the +resolved project/environment name in the success payload would make a mis-resolution visible +immediately. It requires an API response-shape change on both endpoints, so it is a separate +issue — recorded here because it is the natural defence-in-depth companion to §2's silent +mis-resolution, and because it would make §4.1's residual risk near-zero. + +--- + +## 5. Rollback + +Single-file, additive-then-substitutive; revert the three commits in reverse order. Reverting +step 3 alone restores the old schemas while leaving the resolver accepting both keys — a safe +intermediate state, since the resolver is a strict superset of the old behaviour. No database +migration, no persisted state, no config, no frontend involvement. The only externally observable +artifact is the `tools/list` payload, which clients re-fetch on connect. diff --git a/docs/plans/issue-11-test-coverage-and-postman.md b/docs/plans/issue-11-test-coverage-and-postman.md new file mode 100644 index 0000000..6448982 --- /dev/null +++ b/docs/plans/issue-11-test-coverage-and-postman.md @@ -0,0 +1,482 @@ +# Issue #11 — Test coverage baseline, shared test harness, and the stale Postman collection + +**Label:** tech-debt +**Scope of this plan:** the *test harness*, the *coverage baseline*, and the *Postman decision*. +This plan deliberately does **not** define fix behaviour for issues #7, #8, #9, #10, #12, #13 — it +only provides the scaffolding their regression tests will consume. Where a sibling plan needs a +fixture, this document is the authority on its name and signature. + +--- + +## 0. Verified starting state + +Measured against `fix/updater-signing-pipeline` (parent commit `1641f97`), not assumed: + +| Fact | Value | +|---|---| +| Test functions in the whole crate | **17** — `src/vault/import.rs` (5 `#[test]`), `src/crypto/mod.rs` (4 `#[test]`), `src/db/mod.rs` (1 `#[tokio::test]`, legacy-workspace backfill only), `tests/vault_integration.rs` (7 `#[tokio::test]`) | +| Issue's "9 `#[test]`" figure | Correct if you count only `#[test]` inside `src/` (5 + 4). The 8 `#[tokio::test]` cases were not counted. | +| `src/api/mod.rs` | 3122 lines, **36** `async fn handle_*`, 0 tests | +| `src/project/mod.rs` | 525 lines, 0 tests | +| `src/vault/mod.rs` | 1149 lines, 0 tests | +| `src/share/mod.rs` | 882 lines, 0 tests | +| `src/bin/crypt-env/` | 4037 lines across 22 files, 0 tests | +| `src/bin/crypt-env-mcp.rs` | 3136 lines, 0 tests | +| Coverage tooling | none installed, none configured | +| CI | `.github/workflows/release.yml` only — triggers on tag push / `workflow_dispatch`, builds Windows + macOS bundles. **There is no test job and no PR trigger anywhere in the repo.** | +| Local toolchain | `cargo 1.97.0` in WSL; `pkg-config --exists webkit2gtk-4.1` succeeds, so the Tauri lib compiles and tests run natively in WSL. `CARGO_TARGET_DIR` is *not* exported in non-login shells despite `CLAUDE.local.md` — see step 1.6. | +| Postman collection | `src-tauri/tests/crypt-env-api.postman_collection.json`, 81 KB. Asserts `item_count` on `GET /health` at lines 213–216 (field removed). 0 of 15 request URLs pass `environment_id`, `project`, or `environment`. | + +Two structural facts drive every decision below: + +1. **`ApiState` fields are private, every `handle_*` is a private `async fn`, and the router is built + inline inside `start_server` (L3054–3091).** Nothing in `api/` is reachable from an external + integration-test crate today. +2. **`vault::migrate_literal_vars_to_items` (L134), `vault::decrypt_item`/`encrypt_item`, and + `share::import_plain_items_into_vault` (L656) are `pub(crate)`.** External integration tests in + `src-tauri/tests/` *cannot* see them at any visibility short of `pub`. + +--- + +## 1. Objective — measurable definition of done + +### 1.1 Tooling + +`cargo-llvm-cov` is installed, pinned in CI, and driven by a single committed command: + +``` +cargo llvm-cov --lib --bins --no-fail-fast --ignore-filename-regex '(^|/)(tests|test_support)/' --summary-only +``` + +Coverage is **line coverage of the lib + the two bin targets**. The frontend (`src/`, TypeScript) is +out of scope for this issue. + +### 1.2 Per-module line-coverage targets + +Baseline is recorded in Phase 0 by running the command above *before any test is written* and +committing the numbers into the PR description (not into a file — CLAUDE.md forbids extra `.md`s). +Expected baseline is ~0% for every row below; the target column is the merge gate. + +| File | Baseline (expected) | Target | Why not higher | +|---|---|---|---| +| `src-tauri/src/api/mod.rs` | 0% | **≥ 55%** | ~1200 of 3122 lines are `handle_share_*` / `handle_relay_*` / `handle_workspace_relay_*` (L1503–1910, L2245–3040), which do mDNS discovery, x25519 pairing and live Supabase HTTPS calls. Testing those needs a network double, which is out of scope here. | +| — non-network subset of `api/mod.rs`: `resolve_scope`, `validate_create`, `validate_update`, `redact_item`, `environment_item_ids`, `verify_token`, `handle_health`, `handle_unlock`, `handle_list_items`, `handle_get_item`, `handle_create_item`, `handle_update_item`, `handle_delete_item`, `handle_reveal_item`, `handle_fill`, `handle_list_categories`..`handle_delete_category`, `handle_list_commands`, `handle_get_command`, `handle_get_settings`, `handle_put_settings`, `handle_list_projects`, `handle_save_project`, `handle_delete_project`, `handle_preview_delete_project`, `handle_save_environment`, `handle_delete_environment`, `handle_inject_environment`, `handle_environment_example` | 0% | **≥ 80%** | This is the real gate. The whole-file number is diluted by the network handlers. | +| `src-tauri/src/project/mod.rs` | 0% | **≥ 75%** whole file, **≥ 90%** over L95–L360 | L369–434 are `#[tauri::command]` wrappers needing a Tauri runtime; L416 and L459–525 use `rfd` native dialogs. Neither is unit-testable without a window. | +| `src-tauri/src/vault/mod.rs` | 0% | **≥ 50%** | Same reason — the file mixes pure logic with `#[tauri::command]` wrappers. `migrate_literal_vars_to_items`, `create_project_item`, `set_item_global` fork logic must each be ≥ 90%. | +| `src-tauri/src/share/mod.rs` | 0% | **≥ 25%** whole file, **≥ 85%** on `import_plain_items_into_vault` | The other 800 lines are mDNS + x25519 session handshakes. | +| `src-tauri/src/db/mod.rs` | ~8% | **≥ 60%** | | +| `src-tauri/src/bin/crypt-env/commands/scope.rs` | 0% | **≥ 70%** | `fetch_projects` (L118) does a blocking HTTP call to the local API; excluded. | +| `src-tauri/src/bin/crypt-env-mcp.rs` | 0% | **≥ 15%** whole file; **100%** on `append_scope_params` (L798) and `is_safe_env_key` (L816) | The 14 tool handlers are thin `reqwest::blocking` wrappers over the REST API already covered above; duplicating them here buys nothing. | + +### 1.3 New test-case count + +**115 new test functions**, taking the crate from 17 to 132. Distribution is fixed per phase: + +| Phase | Location | Cases | +|---|---|---| +| 1 | `src/api/tests/units.rs` — `redact_item` 3, `validate_create` 8, `validate_update` 4, `environment_item_ids` 2 | 17 | +| 1 | `src/project/mod.rs` in-file — `resolve_environment` | 10 | +| 1 | `src/db/mod.rs` in-file — `upsert_environment_var` | 6 | +| 2 | `src/api/tests/scope.rs` | 12 | +| 2 | `src/api/tests/items.rs` | 10 | +| 2 | `src/api/tests/fill.rs` | 8 | +| 3 | `src/api/tests/projects.rs` | 8 | +| 3 | `src/project/mod.rs` in-file — `inject_environment` 6, `save_environment` multi-owner guard 4 | 10 | +| 3 | `src/vault/mod.rs` in-file — `migrate_literal_vars_to_items` 4, `create_project_item` 3, `set_item_global` fork 5 | 12 | +| 4 | `src/share/mod.rs` in-file — `import_plain_items_into_vault` | 6 | +| 4 | `src/bin/crypt-env/commands/scope.rs` in-file | 9 | +| 4 | `src/bin/crypt-env-mcp.rs` in-file | 7 | +| **Total** | | **115** | + +### 1.4 Non-numeric completion criteria + +- A shared harness exists at `src-tauri/src/test_support/mod.rs` with the exact API in §3.2, and at + least one test in each of `api/`, `project/`, `vault/`, `share/` consumes it. +- `.github/workflows/test.yml` runs on every `push` and `pull_request` and fails the build on any + failing test (Phase 1) and on any coverage regression (Phase 4). +- `src-tauri/tests/crypt-env-api.postman_collection.json` is deleted and `docs/reference.md` §REST API + carries the replacement examples (§5). +- Zero production-code behaviour changes. The only non-test edits are the `start_server` extraction + in §3.1 and the `resolve_environment_id` split in §3.6. Both are pure refactors; any behaviour + delta is a bug in this PR. + +--- + +## 2. What is being mitigated + +Stated so each item is checkable by pointing at a named test. + +**R1 — Silent scope-authorization drift.** `resolve_scope` (api/mod.rs L237) → `project::resolve_environment` +(project/mod.rs L214) is the single function deciding *which environment's secrets an authenticated +caller may read*. It has three input paths (`environment_id`, `project`+`environment`, project-only → +default env) and today nothing asserts that an unresolvable or mismatched scope produces 422 rather +than falling back to "all items". A regression here is not a 500 — it is a cross-project secret leak +that returns HTTP 200. Mitigated by `api/tests/scope.rs` (12 cases) + `project::resolve_environment` +(10 cases). + +**R2 — `ON CONFLICT` semantics flipping from repoint to insert.** `db::upsert_environment_var` +(db/mod.rs L1152) carries `ON CONFLICT(environment_id, key) DO UPDATE SET item_id = excluded.item_id`. +If that clause is ever dropped or the unique index changes, an environment silently accumulates two +rows for the same key and injection becomes order-dependent. Mitigated by the 6 `upsert_environment_var` +cases, which assert row *count* as well as `item_id`. + +**R3 — Regression of the four bugs already fixed in the projects/environments pass.** +Line-preservation in `handle_fill`, 409-on-duplicate-project, collision-skip in +`share::import_plain_items_into_vault`, and mandatory-scope enforcement all currently have zero +protection. Three of the four are shapes a `#[tokio::test]` against `handle_fill`, `resolve_scope` +and `import_plain_items_into_vault` catches directly. Mitigated by `api/tests/fill.rs`, +`api/tests/projects.rs`, and the `share` in-file cases. + +**R4 — Data loss on the one-shot literal migration.** `migrate_literal_vars_to_items` runs once per +install, gated by `settings['migrated_literals_v1']`, and rewrites user data with the master key held +in memory. It cannot be re-run to recover from a bug. Mitigated by 4 cases including an +already-migrated no-op case and a zero-owner-promotes-to-global case. + +**R5 — No PR gate.** Nothing runs `cargo test` before merge today. Even the 17 existing tests can rot +undetected. Mitigated by `.github/workflows/test.yml`. + +**R6 — A contract artifact that lies.** The Postman collection asserts a field that no longer exists +and issues 15 requests that now all 422. Anyone using it to learn the API is actively misled. +Mitigated by §5. + +--- + +## 3. Implementation steps + +### 3.1 Decision: how the API layer is made testable + +Three strategies were on the table. **Recommendation: (B), in-crate `#[cfg(test)]` tests driving the +plain `axum::Router` via `tower::ServiceExt::oneshot`.** + +**(A) Widen visibility — `pub fn build_router(state: Arc) -> Router` + `pub fn ApiState::new` — and test from `src-tauri/tests/api_*.rs`.** +Matches the existing `tests/vault_integration.rs` convention. Rejected on three counts: +- It widens the crate's public API purely for test access, and the widened items are the *entire REST + surface*. CLAUDE.md's decoupling rule exists to keep seams intentional; a `pub` router is a seam no + production caller wants. +- It still cannot reach `redact_item`, `validate_create`, `environment_item_ids`, + `vault::migrate_literal_vars_to_items` or `share::import_plain_items_into_vault` — all private or + `pub(crate)`. Those would need in-file tests *anyway*, splitting the suite across two conventions. +- Every file under `tests/` becomes its own binary that links the whole crate — Tauri, `libsqlite3-sys` + bundled C, rustls, ratatui. Eight such files means eight full links. Measured link cost dominates + this crate's test time. + +**(B) In-crate `#[cfg(test)] mod tests;` submodule under `src/api/tests/`, plus a `#[cfg(test)]`-gated +`src/test_support/mod.rs`. — CHOSEN.** +Zero visibility changes to production items. Full access to private handlers, private `ApiState` +fields and `pub(crate)` domain functions. One test binary for the whole lib. One import path +(`crate::test_support`) for every sibling issue's tests. The existing `tests/vault_integration.rs` +stays exactly as-is — it only touches genuinely public API and is still a useful smoke test that the +public surface compiles standalone. + +**(C) Spin the real HTTPS server on `127.0.0.1:47821` and drive it with `reqwest`.** Rejected: the +port is a fixed constant, so tests cannot run in parallel or alongside a running app; it needs +`tls::ensure_tls_config` to mint a self-signed cert and the client to trust it; and it converts unit +failures into timeouts. It tests axum's TCP stack, not our logic. + +**(D) Test only the pure helpers, skip handlers.** Rejected: R1 and R3 live *in* the handlers. + +**The single required refactor.** `api::start_server` (L3042) is split so the router construction is +separable, with the socket address and TLS staying where they are: + +``` +async fn start_server(vault, app_data_dir) // unchanged signature, still pub + ├── fn build_router(state: Arc) -> Router // private (not pub) — all 36 .route() calls + cors_guard layer + └── (unchanged) const ADDR = "127.0.0.1:47821"; tls::ensure_tls_config; axum_server::bind_rustls +``` + +`build_router` stays **private**; `#[cfg(test)] mod tests;` inside the same module reaches it. **The +CLAUDE.md rule that the API listens only on `127.0.0.1:47821` is untouched**: the address literal and +the only call to `bind_rustls` remain inside `start_server`, no test binds a socket at all, and +nothing test-only is compiled into a release build (`#[cfg(test)]` is absent from `cargo build`). + +`ApiState` gains a private `fn new(vault: SharedState) -> Self` so `start_server` and the harness +build it identically — no duplicated initialisation to drift. + +### 3.2 The shared harness — `src-tauri/src/test_support/mod.rs` + +**Sibling issue plans (#7, #8, #9, #10, #12, #13): this is the fixture API. Use it; do not invent +another.** Declared in `lib.rs` as `#[cfg(test)] mod test_support;`, so it never exists in a release +build. `unwrap()`/`expect()` are **permitted here and in all `#[cfg(test)]` modules** — CLAUDE.md's +ban applies to production code; a panicking fixture is a failing test, which is correct behaviour. + +```rust +pub struct TestVault { + pub dir: tempfile::TempDir, // MUST stay alive: dropping it deletes the sqlite file + pub state: crate::vault::SharedState, + pub token: String, // value for the `X-Vault-Token` header + pub master_password: String, // "test-master-password-1" + pub project_id: i64, // project "demo" + pub env_id: i64, // environment "production" of "demo", is_default = true + pub item_ids: Vec, // the seeded items, in insertion order +} + +/// Tempdir + VaultDb::open + init_vault_crypto + key installed in VaultState + +/// project "demo" with environments "production" (default) and "local" + +/// 3 items (DB_HOST, DB_PASSWORD, API_KEY) linked into "production" + +/// 1 global item (SHARED_TOKEN, is_global = true, linked to nothing) + +/// settings['mcp_token'] seeded so `token` authenticates immediately. +pub async fn unlocked_vault() -> TestVault; + +/// Same crypto setup, no projects / environments / items. For tests that need +/// to assert creation from empty, or 422-on-unresolvable-scope. +pub async fn unlocked_vault_empty() -> TestVault; + +/// Locked vault (`state.key == None`) for 403 VAULT_LOCKED assertions. +pub async fn locked_vault() -> TestVault; + +/// The plain axum Router with state — no TLS, no socket. +pub fn router(v: &TestVault) -> axum::Router; + +/// One request through `ServiceExt::oneshot`. `token: None` omits the header. +/// Returns the status plus the parsed JSON body (`Value::Null` for empty bodies). +pub async fn req( + app: &axum::Router, + method: &str, + uri: &str, + token: Option<&str>, + body: Option, +) -> (axum::http::StatusCode, serde_json::Value); + +/// Real POST /unlock round-trip returning a session token, for the tests that +/// need session semantics (expiry, rate limiting) rather than the static token. +pub async fn session_token(app: &axum::Router, v: &TestVault) -> String; + +pub async fn seed_item(v: &TestVault, name: &str, value: &str, is_global: bool) -> i64; +pub async fn seed_project(v: &TestVault, name: &str, envs: &[&str]) -> (i64, Vec); +pub async fn link_var(v: &TestVault, env_id: i64, key: &str, item_id: i64) -> i64; + +/// Reads an item straight out of the DB and decrypts it — for asserting what was +/// actually persisted, independent of what the handler echoed back. +pub async fn read_item(v: &TestVault, id: i64) -> crate::vault::VaultItem; +``` + +**Auth mechanism, and why.** `verify_token` (L110) falls back to the static `settings['mcp_token']` +when the session token doesn't match. Seeding that setting gives every test a working credential with +no `/unlock` round-trip, which keeps the unlock rate limiter (`unlock_rate`, 5-attempt window) out of +unrelated tests. Tests that specifically exercise session-token behaviour call `session_token()`. + +**`cors_guard` note for consumers:** the middleware (L84) allows a request with no `Origin` header, so +`req()` sends none. A test asserting the CORS guard must set `Origin` explicitly. + +**Dependency-direction note:** `test_support` sits *above* `db`, `vault`, `project` and `api` and is +imported by their test modules only. It does not create a `db → api` edge; `db` remains unaware of +`api` in all non-test builds, satisfying CLAUDE.md. + +### 3.3 `Cargo.toml` changes + +```toml +[dev-dependencies] +tempfile = "3" +tokio = { version = "1", features = ["rt", "macros"] } +tower = { version = "0.5", features = ["util"] } # NEW — ServiceExt::oneshot +``` + +Only `tower` is added. Version skew against axum 0.7's internal tower is a non-issue: `ServiceExt` is +a blanket extension over `tower_service::Service`, which is `0.3` in both trees. `http-body-util` is +not needed — axum 0.7 provides `axum::body::to_bytes`. `serde_json` and `axum` are already normal +dependencies, so in-crate test modules use them without further dev-deps (another concrete win of +strategy B). + +### 3.4 Files touched + +Production edits (refactor only, no behaviour change): + +- `src-tauri/src/api/mod.rs` — extract `build_router`, add `ApiState::new`, add `#[cfg(test)] mod tests;` +- `src-tauri/src/lib.rs` — add `#[cfg(test)] mod test_support;` +- `src-tauri/src/bin/crypt-env-mcp.rs` — split `resolve_environment_id` (§3.6) +- `src-tauri/Cargo.toml` — one dev-dependency + +New test files: + +- `src-tauri/src/test_support/mod.rs` +- `src-tauri/src/api/tests/mod.rs` (declares the four below) +- `src-tauri/src/api/tests/units.rs`, `scope.rs`, `items.rs`, `fill.rs`, `projects.rs` + +In-file `#[cfg(test)] mod tests` appended to: `src/project/mod.rs`, `src/vault/mod.rs`, +`src/share/mod.rs`, `src/bin/crypt-env/commands/scope.rs`, `src/bin/crypt-env-mcp.rs`. +Extended: the existing `mod tests` in `src/db/mod.rs` (L1209). + +Deleted: `src-tauri/tests/crypt-env-api.postman_collection.json`. +Untouched: `src-tauri/tests/vault_integration.rs`. + +### 3.5 Test-case specifications + +Behaviour is asserted **as it exists on `main` today**, except where a sibling issue is explicitly +fixing it — in those spots this plan writes the harness and the sibling plan writes the assertion. + +`api/tests/scope.rs` (12): resolves by `environment_id`; resolves by `project`+`environment`; +resolves by `project` alone → default environment; case-insensitive project name; +case-insensitive environment name; no scope params at all → 422 `VALIDATION_ERROR`; unknown +`environment_id` → 422; unknown project name → 422; known project + unknown environment → 422; +`environment_id` present *and* `project`/`environment` present → documented precedence; missing token +→ 401 `UNAUTHORIZED`; locked vault + valid token → 403 `VAULT_LOCKED`. + +`api/tests/items.rs` (10): `GET /items` returns only items linked into the scoped environment; +`GET /items` never returns `value`/`password`/`content` (asserts `redact_item` on the wire — this is +the CLAUDE.md "no plaintext in API responses" rule made executable); `GET /items?search=` filters +within scope; `GET /items/:id` for an out-of-scope id → 404; `POST /items` persists an encrypted blob +(assert via `read_item`, and assert the raw DB column is not the plaintext); `POST /items` with a +missing `name`/`value`/bad `type` → 422 with the offending field named; `PUT /items/:id` partial +update leaves other fields intact; `DELETE /items/:id` unlinks and removes; +`POST /items/:id/reveal` is the *only* endpoint returning a plaintext value; reveal on an +out-of-scope id → 404. + +`api/tests/fill.rs` (8): a key present in the environment is substituted; a key **absent** from the +environment leaves that line byte-identical (the line-preservation bugfix — R3); comments and blank +lines preserved; trailing-newline presence preserved; CRLF input round-trips; duplicate keys in the +input; empty input; unresolvable scope → 422 before any file content is touched. + +`api/tests/projects.rs` (8): `GET /projects` lists with nested environments; `POST /projects` +creates; `POST /projects` with an existing name → **409** (R3); `POST /projects` updating by id is +not a duplicate; `DELETE /projects/:id` cascade; `GET /projects/:id/preview-delete` counts match what +`DELETE` then does; `POST /environments` multi-owner grant guard; `DELETE /environments/:id`. + +`project::resolve_environment` (10) and the other in-file suites mirror the same axes at the function +level, without HTTP — cheaper failures and clearer blame when both layers break. + +`project::inject_environment` (6) writes to real paths under `tempfile::tempdir()`; it must assert +that the written file's parent stays inside the tempdir (the fixture #7 will build its traversal +assertions on). + +### 3.6 Making the MCP binary's scope logic testable + +`resolve_environment_id` (crypt-env-mcp.rs L1845) mixes argument parsing with a blocking +`fetch_projects` HTTP call, so it cannot be unit-tested. Split it: + +``` +fn resolve_environment_id(args, token) -> Result // unchanged signature; now = fetch + delegate +fn pick_environment_id(args: &Value, projects: &Value) -> Result // pure, testable +``` + +`append_scope_params` (L798) and `is_safe_env_key` (L816) are already pure — in-file tests only. Same +for `scope::parse_project_config` and `scope::resolve` in the CLI binary; only `fetch_projects` +(scope.rs L118) stays untested, and the `allow_create` gate at L158 is asserted through `resolve` +with both flag values. Bin-target tests run under `cargo test --bins`. + +--- + +## 4. Phasing + +The repo's convention is scoped PRs; this is four of them. Each is independently mergeable and green. + +| Phase | PR title | Contents | Gate added | +|---|---|---|---| +| **0** | *(no PR)* | Install `cargo-llvm-cov`, run the baseline command, paste numbers into the Phase 1 PR body | — | +| **1** | `test(api,db,project): add shared harness and pure-function coverage` | `test_support/`, the `build_router` extraction, `Cargo.toml` dev-dep, 33 cases (`api/tests/units.rs`, `resolve_environment`, `upsert_environment_var`), `.github/workflows/test.yml` running `cargo test --lib --bins` | **CI fails on any test failure** | +| **2** | `test(api): router-level coverage for scope, items and fill` | 30 cases via `oneshot` | — | +| **3** | `test(api,project,vault): projects, environments and item-ownership coverage` | 30 cases | — | +| **4** | `test(share,cli,mcp): collision, scope-resolution and helper coverage; drop stale Postman collection` | 22 cases, the `pick_environment_id` split, collection deletion, `docs/reference.md` REST examples | **CI fails on coverage below the ratcheted floor** | + +Phase 1 is the blocking dependency for issues #7–#13 — their plans should target `crate::test_support` +and can begin as soon as it lands, in parallel with Phases 2–4. + +**CI job (`.github/workflows/test.yml`), new, `on: [push, pull_request]`, `ubuntu-latest`:** +checkout → `dtolnay/rust-toolchain@stable` → `Swatinem/rust-cache` → **apt install of the Tauri Linux +system deps** (`libwebkit2gtk-4.1-dev libgtk-3-dev libayatana-appindicator3-dev librsvg2-dev +libsoup-3.0-dev build-essential`) → `cargo test --lib --bins`. Phase 4 appends +`taiki-e/install-action@cargo-llvm-cov` and the coverage run with `--fail-under-lines` set to the +**achieved** figure minus 2 points, per-file thresholds enforced by a small `jq` step over +`--json --summary-only` output. The floor is a ratchet against regression, never an aspiration — +setting it above what the suite actually achieves makes the gate a nuisance and it gets disabled. + +`release.yml` is not modified; it stays tag-triggered. + +--- + +## 5. Decision on the Postman collection + +**Delete `src-tauri/tests/crypt-env-api.postman_collection.json`. `docs/reference.md` becomes the +single documented contract; the `api/tests/*` suite becomes the executable one.** + +Reasoning. The collection has two jobs — documenting the contract and verifying it — and it is now +failing both. It verifies nothing because no process ever runs it: there is no `newman` step in +`release.yml` and no other CI. It documents wrongly because it asserts a removed `item_count` field +and none of its 15 requests carry the now-mandatory scope parameter, so every one of them 422s. A +contract artifact that nothing executes will always drift; it just did, silently, across a whole +migration. Meanwhile `docs/reference.md` already has a REST API section, and after Phase 2 the router +tests assert the real status codes and payload shapes on every commit. + +**Alternative considered — regenerate and wire `newman` into CI.** Rejected on cost and on security: +it needs a live server, which needs `tls::ensure_tls_config` to mint a self-signed cert plus a client +configured to trust it, plus a vault initialised with a master password held in CI — exactly the +shape CLAUDE.md's "master password only in memory, never persists" rule pushes back on. It would +duplicate coverage the `oneshot` tests already provide, at higher operational cost. + +**Alternative considered — regenerate and leave it unexecuted.** Rejected: identical to the current +state, with the clock reset. It re-rots at the next contract change. + +**The condition for ever bringing it back,** stated so the decision is reversible on purpose rather +than by accident: a Postman collection may return only together with a test that *executes* it — +parse the collection JSON in `api/tests/`, replay each request through the same `oneshot` router, and +assert no request returns 404 or 422. That makes staleness a build failure. Without that test, +re-adding the file recreates R6. + +**Replacement content** (Phase 4, inside the existing `docs/reference.md`, no new file): +`curl` examples for `/unlock`, `/items` (GET + POST), `/items/:id/reveal`, `/fill`, `/projects`, +`/environments/:id/inject`, each showing the mandatory scope parameter in both forms +(`?environment_id=` and `?project=&environment=`), and a one-line note that the server is HTTPS on +`127.0.0.1:47821` with a self-signed certificate. + +--- + +## 6. Trade-offs and risks + +### 6.1 Accepted trade-offs + +| Gaining | Losing | +|---|---| +| Zero widening of the crate's public API for test purposes | Test code lives inside `src/`, which conflicts with the existing `tests/vault_integration.rs` convention. Mitigated by keeping that file untouched — the two conventions coexist with a clear rule: *external tests for public API, in-crate tests for private/`pub(crate)` logic.* | +| One test binary instead of ~8 links of a Tauri-sized crate | `src/api/mod.rs` grows a `mod tests;` declaration; the api test code is ~1000 lines across 5 files under `src/`. `cargo build` is unaffected (`#[cfg(test)]`). | +| `oneshot` tests run in milliseconds, in parallel, with no port binding | The TLS layer, `axum_server::bind_rustls`, and `tls::ensure_tls_config` remain untested. Accepted: that is dependency code, and a manual smoke test against the running app covers it. | +| Static `mcp_token` auth in the fixture — no unlock round-trip | Rate-limiting and session expiry are only covered by the explicit `session_token()` tests, not incidentally by every test. Accepted: incidental coverage of a rate limiter makes suites flaky. | +| Coverage floor set as a ratchet on achieved numbers | It will not force coverage upward on its own. Accepted: aspirational gates get disabled. | +| Deleting the Postman collection | Loss of a click-to-explore artifact for manual API exploration. Mitigated by the `curl` examples; reversible under the §5 condition. | + +### 6.2 Risks and early warning signs + +**Risk: the CI test job fails on `ubuntu-latest` for missing Tauri system deps.** The lib depends on +`tauri = "2"`, which needs `libwebkit2gtk-4.1-dev` on Linux. Locally this is already satisfied +(verified). Warning sign: a `pkg-config` failure in the first CI run. Mitigation: the apt step is in +the Phase 1 job from the start. Fallback if it proves slow or brittle: `cargo test --lib --bins` +under `windows-latest`, matching the project's actual build target, at higher runner cost. + +**Risk: the `build_router` extraction silently changes routing or middleware order.** It moves 36 +`.route()` calls and one `.layer(middleware::from_fn(cors_guard))`. Warning sign: any test in +Phase 2 getting an unexpected 404 or 403. Mitigation: Phase 1 lands the extraction with the +handler-level tests already exercising every route in Phase 2 immediately after; review the diff for +`+`/`-` symmetry on the route list specifically. + +**Risk: fixture coupling.** Six sibling issues will depend on `TestVault`'s seeded shape. Changing +"3 items in `production`" later breaks all of them at once. Mitigation: tests assert against +`v.item_ids` and `v.env_id`, never against literal ids or counts; additions go through +`seed_item`/`seed_project` inside the test that needs them, never by editing `unlocked_vault()`. +Warning sign: a sibling PR that modifies `test_support/mod.rs` — that should trigger a look. + +**Risk: `TempDir` dropped early.** `TestVault` owns the `TempDir`; if a test destructures it away, the +sqlite file vanishes mid-run and failures look like DB corruption. Mitigation: documented on the +struct field; helpers all take `&TestVault`, never move out of it. + +**Risk: a test leaks a secret value into CI logs.** Tests handle real plaintext by design. Mitigation: +fixture values are obvious dummies (`test-master-password-1`, `dummy-secret-*`); no test prints an +item value on the success path, and assertion failure messages compare booleans/ids rather than +dumping decrypted payloads. + +**Assumptions that, if wrong, invalidate this plan:** (a) the Tauri lib compiles and its tests run +under `cargo test` on Linux without a display server — verified locally, must be re-verified in CI at +Phase 1; (b) `cargo-llvm-cov` handles the bundled `libsqlite3-sys` C code and the `staticlib`/`cdylib` +crate types without extra configuration — if it does not, the fallback is `--lib`-only coverage with +the bin targets covered by test count rather than percentage; (c) no sibling issue's fix changes +`resolve_environment`'s signature before Phase 1 merges — if one does, this plan's `resolve_scope` +tests are written against the new signature instead, not the old. + +### 6.3 Rollback + +Every phase is a self-contained PR containing only test code plus two pure refactors. Reverting any +phase restores the previous state with no data-model, schema or API-contract implication. The only +irreversible-feeling step is deleting the Postman collection; it remains recoverable from git history +at `1641f97:src-tauri/tests/crypt-env-api.postman_collection.json`. + +### 6.4 Explicitly out of scope + +Fix behaviour for #7, #8, #9, #10, #12, #13; frontend (TypeScript/Vitest) tests; network doubles for +the relay/share handlers; TUI tests; property-based or fuzz testing; and any performance benchmark. diff --git a/docs/plans/issue-12-environment-name-nocase-uniqueness.md b/docs/plans/issue-12-environment-name-nocase-uniqueness.md new file mode 100644 index 0000000..a19af2d --- /dev/null +++ b/docs/plans/issue-12-environment-name-nocase-uniqueness.md @@ -0,0 +1,303 @@ +# Issue #12 — Case-insensitive uniqueness for `environments.name` + +Status: plan (no code written) +Scope: `src-tauri/src/db/mod.rs`, `src-tauri/src/project/mod.rs`, `src-tauri/src/api/mod.rs`, `src-tauri/Cargo.toml` (dev-deps), new `src-tauri/tests/environment_naming.rs` +Related: #7 (path traversal via environment name), #11 (test coverage / HTTP harness), prior "Bug 2" fix (`idx_projects_name_nocase`) + +--- + +## 1. Objective + +Definition of done — every item below is independently checkable. + +1. **Index exists.** `idx_environments_name_nocase` on `environments(project_id, name COLLATE NOCASE)`, created as an additive, idempotent migration during `VaultDb::init_schema`. Verifiable with `SELECT sql FROM sqlite_master WHERE name='idx_environments_name_nocase'`. +2. **Migration is non-bricking.** Opening a vault that already contains a case-colliding environment pair succeeds. Colliding rows are deterministically renamed (lowest `id` keeps the name; the rest get `-2`, `-3`, … suffixes) *before* the index is created. A machine-readable report of every rename is persisted under the settings key `env_name_dedup_v1`. `init_schema` still returns `Err` if the index cannot be created for any other reason — it is never swallowed with `let _ =`. +3. **Same fix retrofitted to `projects`.** `idx_projects_name_nocase` gets the same pre-check. Today it is a bare statement with no audit, so any install predating that fix that already holds `MyApp` + `myapp` **cannot open its vault at all** (`init_schema` → `Err` → `VaultDb::open` → `Err`). See step 0. +4. **Write path returns 409, not 500.** `POST /environments` with a name that case-insensitively collides with an existing sibling returns `409 CONFLICT` with error code `CONFLICT`. Response body contains no SQL text — no `UNIQUE`, no `sqlite`, no table/column/index names. Success codes are unchanged: `201` when `id == 0`, `200` otherwise. +5. **Ambiguous scope resolution is rejected, not guessed.** `project::resolve_environment` returns an error listing the colliding candidates instead of silently taking the first match. HTTP maps it to `409` with error code `AMBIGUOUS_SCOPE`. This is required, not cosmetic — see §4, Decision D5 (SQLite `NOCASE` folds ASCII only; the Rust resolver folds full Unicode, so the index alone does **not** close the bug for non-ASCII names). +6. **Tests pass.** New file `src-tauri/tests/environment_naming.rs`, cases T1–T8 and T11 in §5. HTTP-level cases (T9, T10) are specified here but implemented under #11's harness. + +--- + +## 2. What is being mitigated + +**Checkable statement of the removed risk:** + +> After this change it is impossible for a single project to hold two environments whose names differ only by ASCII case, and impossible for any name-based scope lookup (`?project=X&environment=Y`, CLI `--env`, MCP resolver) to silently resolve to one of several candidates. Every ambiguous lookup fails loudly and names the candidates. + +Concretely, this closes: + +| Failure | Before | After | +|---|---|---| +| Two `POST /environments` create `production` and `Production` in one project | Both persist | Second → `409 CONFLICT` | +| GUI creates `Production`, CLI auto-create races and creates `production` | Both persist | Loser → `409`, CLI can re-fetch and reuse | +| `--env production` with both rows present | Silently picks `ORDER BY id ASC` first row | Cannot happen (index); if it somehow does (non-ASCII), hard error listing candidates | +| Legacy install already holding a colliding pair | Would brick `VaultDb::open` once the index lands | Deterministic rename + persisted report | + +Severity rationale: this is secret-disclosure-adjacent. The scope contract's entire job is guaranteeing you read from and write to the environment you asked for. A silent wrong-environment resolution means production values injected into a local `.env`, or a local value overwriting a production secret. No log, no error, no signal. + +**Explicitly NOT mitigated by this change** (stated so nobody assumes otherwise): +- Non-ASCII case collisions at the *DB* level. `NOCASE` folds `A–Z` only, so `PRODUCCIÓN` and `producción` remain storable as two rows. They are caught by the app-level pre-check (write path) and the ambiguity rejection (read path) — layers 2 and 3 in §4/D5, not by the index. +- Homoglyph / Unicode-normalization collisions (`prоd` with a Cyrillic `о`). Out of scope; note it in the issue for a follow-up if it matters. + +--- + +## 3. Implementation steps + +Ordered. Each step is independently compilable and testable. + +### Step 0 — Retrofit the duplicate pre-check for `projects` (recommended, decide before starting) + +`src-tauri/src/db/mod.rs` L246 today: + +```rust +"CREATE UNIQUE INDEX IF NOT EXISTS idx_projects_name_nocase ON projects(name COLLATE NOCASE)", +``` + +sits inside the `migrations` array whose loop is `.map_err(|e| format!("migration: {e}"))?`. On an install that already holds `MyApp` + `myapp`, this fails, `init_schema` returns `Err`, `VaultDb::open` returns `Err`, and the vault is unopenable with a raw sqlx string as the only diagnostic. + +**This is in scope** because the plan mirrors that statement; shipping the same latent brick for `environments` and leaving the `projects` one in place would be knowingly duplicating a defect. If the maintainer prefers to split it into its own issue, do that — but do not ship step 3 without step 2, and do not ship step 3 while leaving `projects` unaudited. + +Work: move `idx_projects_name_nocase` out of the `migrations` array (delete L241–246 including the comment, keeping the comment text with the statement at its new site) and re-create it in the imperative block added in step 3, after `dedupe_project_names_nocase`. + +### Step 1 — Detect unique-constraint violations without string matching + +`src-tauri/src/db/mod.rs`, new private helper near `upsert_environment` (L1022): + +- Stop using `.map_err(|e| e.to_string())` for the `INSERT`/`UPDATE` in `upsert_environment`. +- Match on `sqlx::Error::Database(dbe)` and use `dbe.is_unique_violation()` (sqlx 0.8 — already a direct dependency at `Cargo.toml` L34). +- On a unique violation, return the **stable sentinel string**: + `"conflict: an environment with this name already exists in this project"` +- On any other error, keep `e.to_string()` — but see step 5 for the API-side leak fix. + +Rationale for the sentinel living in `db`: `db` owns the schema, therefore owns the constraint's meaning. `project` propagates it unchanged. `api` translates it to a status code. This respects the `db` ↛ `api` decoupling rule in CLAUDE.md while removing the fragile `e.to_lowercase().contains("unique constraint")` pattern used at `api/mod.rs` L1961. + +Apply the same treatment to `upsert_project` and change `handle_save_project` (L1961) to match the sentinel prefix `"conflict:"` instead of sqlx text. + +### Step 2 — Deterministic dedup before the index + +`src-tauri/src/db/mod.rs`, two new private async methods on `VaultDb`: + +``` +async fn dedupe_environment_names_nocase(&self) -> Result, String> +async fn dedupe_project_names_nocase(&self) -> Result, String> +``` + +Algorithm (environments; projects identical minus the `project_id` grouping): + +1. Find collision groups: + `SELECT project_id, LOWER(name) AS k, COUNT(*) c FROM environments GROUP BY project_id, k HAVING c > 1` + (`LOWER()` in SQLite is ASCII-only, which exactly matches `NOCASE` — deliberate; this must find precisely what the index would reject, no more, no less.) +2. For each group, `SELECT id, name FROM environments WHERE project_id=? AND LOWER(name)=? ORDER BY id ASC`. +3. **The first row (lowest `id`) keeps its name.** Every subsequent row `n` is renamed to `-`, incrementing the suffix until the candidate collides with nothing in that project (case-insensitively) — so `prod`, `Prod`, plus a pre-existing `prod-2` yields `prod`, `prod-3`. +4. `UPDATE environments SET name=?, updated=? WHERE id=?`. Only `name` changes. +5. Return the rename records. + +**Why lowest `id` wins:** `db::list_environments` (L951) is `ORDER BY id ASC`, and `project::resolve_environment` takes the first `.find()` match. Lowest `id` is therefore the environment that name-based lookups resolve to *today*. Any other tiebreak (e.g. prefer `is_default`) would silently flip which environment `--env production` points at during an upgrade — the exact class of bug being fixed. + +**Why rename and not merge:** the environment `id` is untouched, so `environment_vars` and `environment_paths` (both FK-on-`environment_id`, `ON DELETE CASCADE`) are structurally unaffected — zero rows move, zero rows are dropped, `item_projects` ownership is untouched. See §4/D3 for why merging was rejected. + +**Blast radius of a rename** (must be in the release note): +- `crypt-env.json` files or `--env` flags naming the *renamed loser* now fail with `project/environment not found` — a loud, correct failure replacing a silent wrong-environment resolution. The user re-points them at the new name or renames the environment back in the GUI. +- `environment_paths` rows are unchanged: a renamed environment still injects to the same configured file paths. If the *path string* embeds the old environment name (e.g. `.env.production`), that string is data and is not rewritten — deliberate, since rewriting user-authored paths is a filesystem side effect a migration must never take. +- The GUI shows the new name on next load; no stale cache to invalidate (TanStack Query refetches on unlock). + +### Step 3 — Wire dedup + index into `init_schema` + +`src-tauri/src/db/mod.rs`, `init_schema` (L95). + +- Add the `environments` index **outside** the declarative `migrations` array — it now has an imperative precondition and no longer belongs in a flat SQL list. +- Insert after the `for stmt in &migrations` loop (L248–250) and before the `backfilled_global_orphans_v1` block (L257): + +``` +1. let project_renames = self.dedupe_project_names_nocase().await?; +2. execute CREATE UNIQUE INDEX IF NOT EXISTS idx_projects_name_nocase + ON projects(name COLLATE NOCASE) -- error mapped with `?` +3. let env_renames = self.dedupe_environment_names_nocase().await?; +4. execute CREATE UNIQUE INDEX IF NOT EXISTS idx_environments_name_nocase + ON environments(project_id, name COLLATE NOCASE) -- error mapped with `?` +5. if !renames.is_empty() { merge into settings['env_name_dedup_v1'] } +``` + +**Placement relative to `PRAGMA foreign_keys=ON`:** the pragma is executed in the first `stmts` array (L98) on the single pooled connection (`max_connections(1)`, L85), so it is already on. Nothing here depends on it — no FK references `environments.name` — but the ordering is stated so a future reader does not have to re-derive it. + +**Ordering relative to the other backfills:** the dedup must run *after* the `INSERT INTO environments … 'default'` backfill (L205–208), because that backfill can itself introduce a `default` row into a project that already has a differently-cased `Default`. Placing the block after the whole `migrations` loop guarantees this. + +**Idempotency:** not gated by a settings flag. With the index in place the collision query returns zero rows, so a re-run is a no-op and costs one grouped scan per open. A flag would only mask a regression. The *report* is gated implicitly — it is only written when a rename actually occurred, and is merged (append) with any existing report rather than overwritten. + +**Fail-safe:** all four statements use `?`. A failed index creation aborts `VaultDb::open` with a message that names the remedy, e.g. +`"migration: could not enforce unique environment names (idx_environments_name_nocase): "`. +It is **never** `let _ = …` — silently skipping the index would leave the vault unprotected with no signal, which is the failure mode the issue is about. + +**Report format** (settings value, JSON): +```json +[{"table":"environments","id":42,"projectId":7,"from":"Production","to":"Production-2","at":"2026-08-03T…Z"}] +``` +Contains names and ids only. It must never contain variable keys or values — environment names are already exposed via `GET /projects`, secret material is not. + +### Step 4 — App-level pre-check in `project::save_environment` + +`src-tauri/src/project/mod.rs`, `save_environment` (L172), before `db.upsert_environment` (L173): + +- List the project's existing environments, compare `input.name.to_lowercase()` against each sibling's, skipping the row whose `id == input.id` (so renaming an environment to a different case of its own name is allowed). +- On collision, return the same sentinel: `"conflict: an environment with this name already exists in this project"`. + +This layer is Unicode-aware (Rust `to_lowercase`) and therefore catches the non-ASCII collisions the index cannot. It is TOCTOU-racy on its own — the DB index is the race backstop for the ASCII case. Neither layer is redundant; each covers the other's gap. + +Also add the same check to `save_project`'s auto-created `"default"` environment (L153) — cheap, since a brand-new project has no siblings, but keeps the choke point single. + +**Sequencing with issue #7:** #7 wants `validate_environment_name` at this exact site. Fixed contract, whichever lands first: + +```rust +validate_environment_name(&input.name)?; // #7 — shape/traversal +ensure_no_case_collision(db, &input)?; // #12 — this plan +let env_id = db.upsert_environment(...) // existing +``` + +Shape validation first (a traversal-unsafe name should be rejected before it is compared against anything). The two edits are adjacent lines in the same function — coordinate the merge order, but there is no logical conflict. + +### Step 5 — HTTP mapping + +`src-tauri/src/api/mod.rs`, `handle_save_environment` (L2022–2063). Replace the catch-all at L2061: + +```rust +Err(e) if e.starts_with("conflict:") => + err_json(StatusCode::CONFLICT, + "an environment with this name already exists in this project", + "CONFLICT").into_response(), +Err(_) => err_json(StatusCode::INTERNAL_SERVER_ERROR, + "internal error", "INTERNAL_ERROR").into_response(), +``` + +Two things happen here. The conflict gets its own 409, and — separately — the fallback stops echoing `&e` into the response body. Today an unexpected sqlx failure returns raw SQL text (table, column, index names) to the client, which violates the CLAUDE.md rule on what may appear in API responses. Log the detail to stderr instead (never the input `name`, and never any var value). + +Mirror the same fallback hardening in `handle_save_project` (L1965). + +### Step 6 — Ambiguity rejection in `resolve_environment` + +`src-tauri/src/project/mod.rs`, `resolve_environment` (L214–237). Replace both `.find()` calls with collect-and-count: + +- Project match: >1 candidate → `Err("ambiguous match for project '

': (id N), (id M). Pass environment_id instead.")` +- Environment match within the resolved project: same shape. + +Message format deliberately mirrors the existing MCP precedent at `src-tauri/src/bin/crypt-env-mcp.rs` :2725–2740 so users see one vocabulary across GUI, HTTP, CLI and MCP. + +API side: map an error starting with `"ambiguous match"` to `409 CONFLICT`, code `AMBIGUOUS_SCOPE`, in every handler that calls `resolve_environment`. 409 rather than 422 because the request is well-formed; it is the vault's state that prevents an unambiguous answer, and the client's fix is to pass `environment_id`. + +Leave `src-tauri/src/bin/crypt-env/commands/scope.rs` and the MCP resolver alone in this change — MCP already handles ambiguity, and CLI's resolver goes through the HTTP API, so it inherits the new 409. Note it and move on. + +### Step 7 — Frontend (optional, low priority) + +`src/` `ProjectManager.tsx` environment editor: client-side case-insensitive check against the already-loaded environment list, to give immediate feedback before `invoke('environment_save')`. Surface the backend sentinel verbatim on failure. + +This is UX only. The client check is **not** enforcement — the DB index and the server pre-check are. Do not let it grow into the only guard. + +Tauri path: `environment_save` (`project/mod.rs` L393) already returns `Result` and will carry the sentinel unchanged, so the GUI gets a distinguishable, stable string without a new command or signature change. + +--- + +## 4. Trade-offs / alternatives considered + +### D1 — Index shape: `(project_id, name COLLATE NOCASE)` + +**Chosen.** `project_id` must stay as the leading column: environments are scoped per project, and `production` in project A and `production` in project B is not merely legal, it is the normal case. A global `UNIQUE(name COLLATE NOCASE)` would break every multi-project vault on first open. + +`project_id` is an INTEGER, so its collation is irrelevant — only `name` carries `COLLATE NOCASE`. + +*Rejected:* changing the column declaration to `name TEXT COLLATE NOCASE` in the `CREATE TABLE`. It reads cleaner, but SQLite does not apply it to existing tables, so it would require a table rebuild (`ALTER TABLE … RENAME` + recreate + copy + FK dance) on every existing install. A separate unique index is additive, `IF NOT EXISTS`-idempotent, and instantly droppable — a far cheaper rollback. + +*Rejected:* a `name_lower` generated/materialized column with a plain unique index. More explicit, and it would let us store a Unicode-folded key — but it changes the table shape, needs a backfill, and adds a column every read path must ignore. Deferred; revisit only if non-ASCII collisions become a real complaint. + +### D2 — Where the case-folding contract lives + +Chosen: SQLite `NOCASE` (ASCII) at the DB, Rust `to_lowercase()` (Unicode) in the app. These two disagree, and the plan treats that disagreement as a documented, tested fact rather than pretending it away — that disagreement is precisely why step 6 exists. + +*Rejected:* switching the Rust resolvers to ASCII-only folding to match SQLite. It would make the two layers agree, but by making resolution *less* strict — `PRODUCCIÓN` and `producción` would then resolve as different environments, which is defensible but changes existing lookup behaviour for anyone relying on it. Loosening a security-relevant comparison to simplify an invariant is the wrong direction. + +*Rejected:* registering a custom Unicode collation with SQLite. Correct in principle, but it makes the index dependent on a runtime-registered function — a vault opened by any other SQLite client (backup tooling, `sqlite3` CLI, a future `crypt-env` build that forgets to register it) would fail to read the index. Not worth it for a desktop vault. + +### D3 — Conflict resolution policy: rename losers + +**Chosen:** lowest `id` keeps the name, others get `-2`, `-3`, … with collision-avoiding suffix search. + +*Rejected — refuse to start.* Honest, but it bricks the vault for a condition the user never caused and cannot fix without a SQL client. The vault is the only place the secrets live. Unacceptable. + +*Rejected — merge the colliding environments.* Superficially the "nicest" outcome, but merging two sets of `environment_vars` means deciding what happens when both define `DB_PASSWORD` with different `item_id`s. Any automatic answer picks one secret over another silently — which is exactly the failure class this issue is about. A migration must never silently choose between two secrets. + +*Rejected — surface a repair command and skip the index until the user runs it.* Keeps the vault open and gives the user full control, but leaves the bug live for an unbounded window, and the "skip" branch is the branch nobody tests. Also needs new UI plus a new CLI subcommand — significantly more surface than a rename. + +The rename's cost is real and stated: a name-based reference to the renamed loser breaks. That break is loud and locally fixable, and it replaces a silent wrong-environment read. Reversible by hand using the persisted `env_name_dedup_v1` report. + +### D4 — Sentinel string vs. a typed error + +**Chosen:** a stable sentinel prefix (`"conflict:"`) on the existing `Result<_, String>`, with the *detection* done properly via `sqlx`'s `is_unique_violation()` rather than substring-matching sqlx's text. + +CLAUDE.md asks for custom error types, and a `VaultError` enum is the right long-term answer. It is not this change: `Result<_, String>` runs through `db`, `project`, every Tauri command (Tauri needs a serializable error) and every HTTP handler. Converting it is a large, mechanical, high-blast-radius refactor that should be its own issue, reviewed on its own merits. Bundling it here would make a security fix hard to review. + +*Rejected:* keeping the existing `e.to_lowercase().contains("unique constraint")` pattern from `handle_save_project` L1961. It couples the API layer to sqlx's error prose, breaks on a sqlx upgrade or a locale change, and requires the raw SQL string to survive all the way to the HTTP layer — which is what causes the SQL leak in the 500 path. + +**Regret check:** if the `VaultError` refactor lands later, every sentinel site is a single `grep "conflict:"` away. The sentinel is a deliberate placeholder, not a permanent design. + +### D5 — Ambiguity rejection in `resolve_environment`: required, not belt-and-braces + +The initial framing was "defence in depth for installs that predate the index". That framing is wrong, and the correct reason is stronger: + +SQLite's `NOCASE` folds ASCII `A–Z` only. `project::resolve_environment` uses Rust `to_lowercase()`, which folds full Unicode. Therefore `PRODUCCIÓN` and `producción` **satisfy the new index** (SQLite sees two distinct names) while the resolver sees two matches and silently takes the first. The index alone does not close the reported bug for non-ASCII names. + +Steps 4 and 6 are what actually close it for those names. Step 6 also covers the residual TOCTOU race in step 4's pre-check. + +*Rejected:* relying on the index alone. Cheaper, and correct for the ASCII case that motivated the issue — but it would ship a fix that is advertised as complete and is not. + +### D6 — `resolve_environment` ambiguity → 409, not 300/422 + +409 `CONFLICT` + code `AMBIGUOUS_SCOPE`. The request is syntactically valid, so not 422; the server refuses because vault state makes the answer non-unique, and the client's remedy (`environment_id`) is deterministic. 300 Multiple Choices is technically closest but is effectively unused in practice and would surprise every existing client. + +### D7 — Test seeding requires `sqlx` in dev-dependencies + +Test T3 must seed a DB that *already* contains a colliding pair — impossible through the public API once the index exists. Integration tests link the lib crate but not its dependencies, so they cannot `use sqlx` today. + +**Chosen:** add `sqlx = { version = "0.8", features = ["sqlite", "runtime-tokio"] }` to `[dev-dependencies]` in `src-tauri/Cargo.toml` (L97–99). Zero extra compilation — the crate is already built as a normal dependency (L34). + +*Rejected:* a `pub` test-only hook on `VaultDb` (e.g. `insert_environment_unchecked`). It puts a constraint-bypassing method on the production API surface for test convenience — precisely the hidden complexity CLAUDE.md rejects, and a future caller will eventually use it. + +*Rejected:* shipping a fixture `.db` file in the repo. Opaque binary, invisible to review, and rots the moment the schema changes. + +--- + +## 5. Tests + +New file: `src-tauri/tests/environment_naming.rs`. Same pattern as the existing `vault_integration.rs` — `#[tokio::test]` + `tempfile::tempdir()` + `VaultDb::open`. No new harness. + +**Coordination with #11:** #11 owns the HTTP-level test harness (spawning the axum app, tokens). Cases T9 and T10 below are specified but belong to that harness — do not build a second one here. Cases T1–T8 and T11 need only a `VaultDb`, so they are self-contained and can land first. + +| # | Case | Assertion | +|---|---|---| +| T1 | Fresh DB, `upsert_environment(0, p, "production")` then `"Production"` | Second returns `Err` starting with `"conflict:"` | +| T2 | `"production"` in project A and project B | Both succeed — proves `project_id` is in the index | +| T3 | Seed DB via raw sqlx: drop the index, insert `production` (id 1) + `Production` (id 2), close, reopen with `VaultDb::open` | Opens successfully; id 1 still `production`, id 2 now `production-2` | +| T4 | T3's DB seeded with vars + paths on both rows | `environment_vars` / `environment_paths` counts per `environment_id` unchanged; `item_projects` unchanged | +| T5 | Open T3's DB a second time | No further renames; `env_name_dedup_v1` report has exactly the T3 entries, not duplicated | +| T6 | Seed `prod`(1), `Prod`(2), `PROD`(3) | → `prod`, `prod-2`, `prod-3` | +| T7 | Seed `prod`(1), `Prod`(2), plus an existing `prod-2`(3) | Loser becomes `prod-3`, `prod-2` untouched | +| T8 | Seed `producción` + `PRODUCCIÓN` (both survive the index — ASCII-only `NOCASE`) | `project::resolve_environment(db, None, Some(p), Some("PRODUCCIÓN"))` returns `Err` containing `"ambiguous match"` and both ids | +| T9 | *(#11 harness)* `POST /environments` with a colliding name | `409`, code `CONFLICT`; body contains none of `UNIQUE`, `sqlite`, `environments`, `idx_` | +| T10 | *(#11 harness)* `POST /environments` with a non-ASCII case collision | `409` from the step-4 app pre-check, same body constraints | +| T11 | `save_project` auto-creates `default`; then save an environment named `Default` | `Err` with the `"conflict:"` sentinel | + +Also extend the projects side minimally: one test that a DB seeded with `MyApp` + `myapp` now **opens** (today it would fail `init_schema`) and that `myapp` was renamed to `myapp-2`. + +--- + +## 6. Rollback + +- **Index:** `DROP INDEX idx_environments_name_nocase;` — instant, no data touched. Reverting the binary alone is not enough: the index persists in the file, so a downgrade must drop it or the old binary will start hitting constraint errors on write. State this in the release note. +- **Renames:** not automatically reversible. Mitigated by the persisted `env_name_dedup_v1` report, which contains every `(id, from, to)` needed to reverse them by hand or via a future repair command. This is the one irreversible part of the change and the reason the rename policy is deterministic and documented. +- **Sentinel / 409 mapping / ambiguity rejection:** pure code, revert with the commit. + +--- + +## 7. Open questions for the maintainer + +1. **Step 0 in or out?** Retrofitting the `projects` pre-check fixes a real vault-bricking path but widens the diff beyond issue #12's title. Recommendation: in, same PR, called out in the commit message. +2. **Suffix format.** `production-2` assumed. If environment names are ever used verbatim as filename fragments, confirm `-` is safe there — this overlaps with #7's `validate_environment_name`, and the generated suffix must satisfy whatever that validator ends up allowing. +3. **Surfacing the rename to the user.** The report lands in `settings`. Minimum viable surface is a line in `crypt-env doctor` (`src-tauri/src/bin/crypt-env/commands/doctor.rs` already reports on `crypt-env.json` and is the natural home). A GUI toast on first unlock after the migration is nicer but is a product decision, not an architectural one. diff --git a/docs/plans/issue-13-global-items-scoped-visibility.md b/docs/plans/issue-13-global-items-scoped-visibility.md new file mode 100644 index 0000000..80842a5 --- /dev/null +++ b/docs/plans/issue-13-global-items-scoped-visibility.md @@ -0,0 +1,422 @@ +# Issue #13 — Global items invisible through scoped list/search surfaces + +Status: plan only (no code written). +Branch target: a dedicated `fix/global-items-scoped-visibility` branch off `main`. +Related: issue #11 (test-coverage plan — this plan depends on it for the HTTP harness, see §5). + +--- + +## 0. Correction to the issue text + +The issue claims `is_global` "isn't even in scope at the point of filtering (already +stripped by `decrypt_all_items`'s tuple shape)". **That is not accurate against current +code.** + +`src-tauri/src/api/mod.rs:145-166` (`decrypt_all_items`) maps raw rows +`(id, _, data, _, is_global)` through `crate::vault::decrypt_item(&key, id, &data, is_global)`, +and `VaultItem.is_global: Option` (`src-tauri/src/vault/mod.rs:55-56`) is therefore +populated on every item reaching the filter site at `src-tauri/src/api/mod.rs:480`. + +Consequence for scoping this work: the Rust change is small (a filter predicate plus a +query-param type). The bulk of the effort is **contract definition, response shape, +downstream surfaces (CLI/MCP), documentation, and tests** — not the filter itself. + +--- + +## 1. Objective (definition of done) + +### 1.1 Contract to be implemented + +Introduce an explicit, documented tri-state query parameter on the two **discovery** +endpoints, and leave every **materialization** endpoint untouched. + +| Surface | Kind | Change | +|---|---|---| +| `GET /items` | discovery | accepts `include_global=true\|false\|only`, **default `true`** | +| `GET /commands` | discovery | accepts `include_global=true\|false\|only`, **default `true`** | +| `POST /fill` | materialization | **unchanged** — resolves strictly through `environment_vars` | +| `POST /environments/:id/inject` | materialization | **unchanged** | +| `POST /environments/:id/example` | materialization | **unchanged** | +| `POST /share/listen` | action | **unchanged** — sender may still only share items linked into the resolved environment | +| `POST /items`, `/share/connect`, `/share/import`, `/relay/receive` | write/scoped | **unchanged** | + +Semantics: + +- `include_global=true` (default): returned set = (items linked in the resolved + environment) ∪ (all items with `is_global = 1`), deduplicated by item id. +- `include_global=false`: returned set = items linked in the resolved environment only — + byte-for-byte the current behaviour, i.e. "what `/fill` and `/inject` will materialize". +- `include_global=only`: returned set = all `is_global = 1` items, ignoring linkage — the + REST/CLI/MCP equivalent of the GUI's `GlobalSecrets.tsx` screen, but still requiring a + valid scope so error handling stays uniform. +- Any other value → `422 VALIDATION_ERROR` with field `include_global` + (reusing `err_validation`, `src-tauri/src/api/mod.rs:211`). +- Scope resolution is unchanged: `environment_id`, or `project`+`environment`, still + required; still 422 when unresolvable. + +### 1.2 Response shape + +Every item returned by `GET /items` and `GET /commands` carries two discriminators: + +- `isGlobal: bool` — already serialized today via `VaultItem` (`vault/mod.rs:55`). +- `linked: bool` — **new, API-response-only**: `true` iff the item id appears in the + resolved environment's `environment_vars`. + +`linked` MUST NOT be added to `VaultItem`. `VaultItem` is the struct that gets serialized +into the AES-GCM ciphertext (`vault::encrypt_item`), so a view-only field on it risks being +persisted into the encrypted blob by any round-trip write path. Instead add an API-layer +wrapper in `src-tauri/src/api/mod.rs`: + +```rust +#[derive(Serialize)] +struct ScopedItem { + #[serde(flatten)] + item: VaultItem, + linked: bool, +} +``` + +Redaction is unchanged: `redact_item` (`api/mod.rs:169-174`) still strips +`value`/`password`/`content` before the item is wrapped. **No plaintext secret is added to +any response by this change** — the union widens *metadata* visibility only. + +### 1.3 Downstream surfaces + +- CLI `crypt-env search` (`src-tauri/src/bin/crypt-env/commands/search.rs`): new + `--scope-globals ` flag (default `with`), plus a `SCOPE` column + rendering `linked` / `global` / `global+linked`. +- CLI `crypt-env list` (`.../commands/list.rs`, consumes `/commands`): same flag, same column. +- CLI `crypt-env cmd` / `crypt-env exec` (`.../commands/cmd.rs`, `.../commands/exec.rs`): + on name collision between a linked command and a global command, **prefer the linked one** + and print a one-line stderr warning naming the shadowed global id. +- MCP `crypt_env_list_items` and `crypt_env_search_items` + (`src-tauri/src/bin/crypt-env-mcp.rs:205`, `:229`): add `include_global` to both + `inputSchema`s with the enum and a description that states the linked/global distinction, + and forward it in the URL builder next to `append_scope_params` (`:798`). +- GUI: **no change**. `src/components/GlobalSecrets.tsx:31` filters `allItems` client-side + off the unscoped Tauri command and continues to work. This keeps the diff off the + frontend entirely and respects the `invoke()`-only rule. + +### 1.4 Verifiable acceptance criteria + +Done when all of the following hold: + +1. `GET /items?project=X&environment=Y` returns a global item that is linked into **no** + environment, with `isGlobal: true, linked: false`. +2. The same request with `&include_global=false` returns exactly the pre-change set. +3. `&include_global=only` returns exactly the set the GUI's Global Secrets screen shows. +4. `&include_global=bogus` returns 422 with `include_global` in the message. +5. An item that is both global and linked appears **once**, with `linked: true`. +6. `POST /fill`, `/environments/:id/inject`, `/environments/:id/example` produce + byte-identical output before and after the change for the same environment. +7. `POST /share/listen` still 422s when asked to share an unlinked global item. +8. **9 automated tests pass** (7 unit + 2 integration; enumerated in §5), plus the + 4-step manual repro from the issue no longer reproduces. +9. `docs/reference.md` states the contract, and the stale Notes paragraph at + `docs/reference.md:280` ("Global items … are invisible to …") is replaced. + +--- + +## 2. What is being mitigated + +**Concrete bug.** `environment_item_ids` (`src-tauri/src/api/mod.rs:438-440`) builds the +allowed-id set purely from `env.vars`, and `handle_list_items` (`:467`, `:480`) filters on +it with no `is_global` branch. A global item that is not linked into the queried environment +is therefore absent from the response — **indistinguishable, from the caller's side, from an +item that does not exist**. + +Checkable statement of the defect: + +> With a vault containing exactly one item, created global and linked to no environment, +> `GET /items?project=X&environment=Y` returns `[]` for every existing project/environment, +> while the GUI Global Secrets screen lists it. + +**Risks this reduces:** + +1. **Contract divergence between GUI and headless surfaces.** The documented mental model in + `CLAUDE.md` and `docs/reference.md` is "create a value once, mark it global, reference it + from any environment without re-entering it". Today only the GUI honours that; REST, CLI + and MCP do not. `docs/reference.md:280` already admits this in a Notes paragraph, which + makes the divergence a *known, undocumented-in-the-contract-table* behaviour — the worst + of both. +2. **Duplicate-secret proliferation.** An agent or script that cannot see a global secret has + exactly one recovery path: create a second copy of the same credential inside the project + scope. That multiplies the number of places a rotation must reach, which is a security + regression, not just an ergonomics one. +3. **Unrecoverable dead end for MCP agents.** There is **no** unscoped or global-only route + today — `/globals` does not exist and `/items` has no `global` filter (route table, + `api/mod.rs:3055-3090`). An LLM agent has no tool call that can answer "does a reusable + global secret for this service already exist?". + +**Explicitly *not* mitigated by this change** (stated so nobody mistakes the scope): +this is a visibility/discovery fix. It does not make globals participate in `/fill` or +`/inject`. Linking a global into an environment remains a deliberate act. + +--- + +## 3. Implementation steps + +Ordered. Each step is independently compilable; steps 1–3 are the behavioural core, 4–7 are +surface propagation, 8–9 are docs and tests. + +### Step 1 — Query-param type and parser (`src-tauri/src/api/mod.rs`) + +- Add near `EnvScopeQuery` (`:227`): + ```rust + #[derive(Clone, Copy, PartialEq)] + enum IncludeGlobal { With, Without, Only } + ``` + with `fn parse(raw: Option<&str>) -> Result` + mapping `None|"true"|"with" → With`, `"false"|"without" → Without`, `"only" → Only`, + and anything else → `err_validation("include_global", "must be one of: true, false, only")`. + No `unwrap()`; `Result` per CLAUDE.md. +- Add `include_global: Option` to `ItemsQuery` (`:425-434`) and to the + `EnvScopeQuery` extractor used by `handle_list_commands` — or, cleaner, add it to + `EnvScopeQuery` alone and have `ItemsQuery` keep its own copy, matching the existing + duplication style rather than refactoring the extractor in a bug-fix PR. + +### Step 2 — Replace the scope predicate (`src-tauri/src/api/mod.rs`) + +- Replace the helper at `:438-440` with a pair: + ```rust + fn environment_item_ids(env: &project::Environment) -> HashSet // keep, unchanged + fn scope_items(items: Vec, linked: &HashSet, mode: IncludeGlobal) -> Vec + ``` + `scope_items` is a **pure function** — no `ApiState`, no lock, no crypto — so it is unit + testable without a vault or an HTTP server. It performs the union/dedup and stamps + `linked`. +- `handle_list_items` (`:442-481`): replace `.filter(|item| allowed_ids.contains(&item.id))` + with `scope_items(items, &allowed_ids, mode)`, then apply the existing type/category/search + filters over `ScopedItem` (matching on `s.item.*`). Order matters: union first, filters + second, so `search` also searches globals. + +### Step 3 — Apply to `/commands` (`src-tauri/src/api/mod.rs:964-1022`) + +- `handle_list_commands` uses the same helper at `:989`; route it through `scope_items` with + the same parsed mode. The placeholder-extraction logic is untouched. + +### Step 4 — Confirm the untouched sites stay untouched + +Audit, do not edit, the remaining `resolve_scope` callers so the split between discovery and +materialization is deliberate and reviewable: + +| Line | Handler | Uses `environment_item_ids`? | Action | +|---|---|---|---| +| `:587` | `handle_create_item` | no | unchanged | +| `:978` | `handle_list_commands` | yes (`:989`) | **changed** (step 3) | +| `:1329` | `handle_fill` | no — matches `env.vars.key` directly | unchanged | +| `:1527` | `handle_share_listen` | yes (`:1538`) | **unchanged on purpose** — see §4.3 | +| `:1617` | `handle_share_connect` | no | unchanged | +| `:1877` | `handle_share_import` | no | unchanged | +| `:2448` | `handle_relay_receive` | no | unchanged | + +Also note but do **not** change `handle_create_item`'s hardcoded +`body.item.is_global = Some(false);` (`:647`). It is documented behaviour +(`docs/reference.md:14`) and changing item-creation semantics does not belong in a +visibility fix; it is called out in §4.5 as follow-up. + +### Step 5 — CLI (`src-tauri/src/bin/crypt-env/`) + +- `client.rs:54-64` — add `#[serde(default, rename = "isGlobal")] pub is_global: bool` and + `#[serde(default)] pub linked: bool` to `ItemSummary`; add the same two fields to + `CommandDetail` (`:66+`). +- `commands/search.rs` — add the `--scope-globals` flag, append `include_global=` to the URL + built at `:22-26`, add the `SCOPE` column to the `println!` table. +- `commands/list.rs` — same flag, appended to the `/commands` URL at `:29`, extra table column. +- `commands/cmd.rs` (`:42`, `:75`, `:124`) and `commands/exec.rs` (`:31`) — these resolve a + command **by name**; add linked-wins tie-breaking plus the stderr shadow warning. They do + not get the new flag (they are execution paths, not listings) and should send + `include_global=true` implicitly so a global command is runnable from any project. + +### Step 6 — MCP (`src-tauri/src/bin/crypt-env-mcp.rs`) + +- Add `include_global` (`"type": "string"`, `"enum": ["true","false","only"]`) to the + `inputSchema` of `crypt_env_list_items` (`:205`) and `crypt_env_search_items` (`:229`). + Descriptions must state: *"true (default) also lists reusable global secrets not yet linked + into this environment — these appear with `linked: false` and will NOT be written by + generate/inject/fill until linked."* +- Forward the value in the two URL builders that call `append_scope_params` (`:798`) for + those tools. Do **not** touch `crypt_env_generate_env` / `crypt_env_inject_env` / + `crypt_env_fill_env`. + +### Step 7 — Tauri commands + +No change. The GUI reaches items through the unscoped `vault_get_items` path, and +`GlobalSecrets.tsx:31` filters on `isGlobal` client-side. Registering a new `module_action` +command is unnecessary and would duplicate contract surface. + +### Step 8 — Documentation (`docs/reference.md`) + +1. Rewrite the `GET /items` row (`:14`) and the `GET /commands` row to describe + `include_global`, its default, and the `linked` field. +2. Add a short **"Global items and scope"** subsection under the REST table stating the + contract in one paragraph: *discovery surfaces union globals; materialization surfaces + never do; `linked` is the discriminator.* +3. Replace the stale Notes paragraph at `:280` with a description of the new behaviour and + the remaining gap (globals still require an explicit link before `/fill`/`inject` uses them). +4. Update the MCP tool table rows for `crypt_env_list_items` / `crypt_env_search_items`. +5. Update the CLI table rows for `list` / `search` with the new flag. + +### Step 9 — Tests (see §5). + +--- + +## 4. Trade-offs and alternatives considered + +### 4.1 DECISION — tri-state `include_global`, defaulting to `true`, on discovery endpoints only + +Union globals into `GET /items` and `GET /commands` by default, mark every returned item with +`isGlobal` + `linked`, and leave `/fill`, `/inject`, `/example` and `/share/listen` strictly +linkage-based. This is the smallest change that makes the documented mental model true for +headless callers, while keeping the "what will actually be written" question answerable via +`include_global=false`. + +### 4.2 ALTERNATIVE A — unconditional union, no parameter (the issue's option (a)) + +Strengths: one-line diff, zero new surface, nothing to document beyond a sentence, no risk of +callers passing a wrong enum value. + +Rejected because it **destroys an answer that currently exists**. Today `GET /items?project=X&environment=Y` +is the only way to ask "what will `/fill` and `/inject` write for this environment". An +unconditional union removes that with no replacement, so any consumer doing a pre-inject +diff, a CI drift check, or a "which keys are missing" report silently starts reporting the +whole global set as present. `include_global=false` preserves it at the cost of one enum. + +### 4.3 ALTERNATIVE B — document the current behaviour as intended, add a `/globals` route (the issue's option (b)) + +Strengths: intellectually the cleanest split — "global means *available to link*, not +*implicitly present*" is a defensible model, and a dedicated route keeps each endpoint's +answer unambiguous. It is also the least likely to surprise an existing script. + +Rejected on two grounds. First, it contradicts the redesign premise already written into +`CLAUDE.md` and `docs/reference.md` ("reference it from any environment without re-entering +it") — adopting it means editing the product's stated model to match an implementation +accident. Second, it costs *more* surface, not less: a new route, a new MCP tool, a new CLI +subcommand, all of which an LLM agent must be taught to call at the right moment. Agents +reliably call the obvious tool and stop; a discovery gap that requires knowing about a second +tool is a discovery gap. + +Partially adopted anyway: `include_global=only` gives the same capability as a `/globals` +route without a new route, MCP tool, or auth surface. + +### 4.4 ALTERNATIVE C — union into every scoped surface including `/fill` and `/share/listen` + +Strengths: maximal consistency; one rule, no discovery/materialization distinction to explain. + +Rejected as a security regression. `/fill` writing every global secret into a project's +`.env` because it happens to match a template key turns "reusable across projects" into +"leaked into every project's on-disk env file". `/share/listen` is worse: it would let a +sender transmit globals that were never associated with the project they claimed to be +sharing from — a one-call exfiltration widening, exactly the class of hazard already noted +for `crypt_env_inject_environment` in `docs/reference.md`. Materialization must stay an +explicit, per-link decision. + +### 4.5 Sub-decisions + +**Default `true` vs default `false`.** Default `false` is strictly backward compatible and +therefore tempting. It is rejected because it fixes nothing for existing callers: every MCP +agent and CLI script keeps seeing an empty result for globals until someone rewrites it to +pass a parameter it has no reason to know exists. The bug is precisely "callers cannot +discover that globals exist" — a fix gated behind an opt-in the caller cannot discover is +not a fix. The cost is a genuine behaviour change to `GET /items`, mitigated by `linked`, +by the unchanged materialization endpoints, and by the escape hatch. + +**`linked` on a wrapper vs on `VaultItem`.** Adding `linked` to `VaultItem` is fewer lines +but `VaultItem` is the plaintext struct that gets encrypted; a per-request view flag on it +can be persisted into ciphertext by any read-modify-write path, permanently baking a +scope-relative boolean into an item's encrypted payload. The `ScopedItem` wrapper with +`#[serde(flatten)]` keeps `db`/`vault` unaware of API concerns, honouring the module +decoupling rule. + +**Applying it to `/commands` too.** Considered leaving `/commands` alone (smaller blast +radius). Rejected: `crypt-env list`, `crypt-env cmd` and `crypt-env exec` all read +`/commands`, so a global command would remain invisible to the CLI while global secrets +became visible — a new inconsistency in place of the old one. The name-collision tie-break +(linked wins, warn on shadow) is the cost of that consistency and is cheap. + +**Not fixing `handle_create_item`'s `is_global = Some(false)` here.** It is a separate +contract question ("can the REST API create a global item?") with its own ownership and +cascade implications. Bundling it would make this PR's diff span creation semantics as well +as read semantics. Follow-up issue. + +### 4.6 Reversibility + +High. The change is one enum, one pure function, one wrapper struct, plus additive fields on +CLI/MCP schemas. Reverting is a single `git revert` of the API commit; CLI and MCP additions +are backward-compatible (an unknown flag simply stops being sent) and can be left in place or +reverted independently. No database migration, no schema change, no persisted state — nothing +to roll forward or clean up. + +--- + +## 5. Test plan + +**Dependency on issue #11.** There is no HTTP-level test harness today: +`src-tauri/tests/vault_integration.rs` (106 lines) exercises `VaultDb` directly with +`tempfile::tempdir()` + `#[tokio::test]`, and never boots the axum router or unlocks a vault. +Building one (unlock, token, router, client) is issue #11's job. **This plan must not +duplicate that harness design.** It instead extracts the logic into a pure function so the +behaviour is fully covered without HTTP, and leaves exactly one end-to-end assertion to be +added once #11 lands. + +### 5.1 Unit tests — 7, in `src-tauri/src/api/mod.rs` under `#[cfg(test)] mod scope_tests` + +Fixtures are plain `VaultItem` values and a `project::Environment` with synthetic `vars`; no +database, no key, no async. + +1. `global_unlinked_item_visible_by_default` — env links item A (non-global); vault also has + item B (`is_global = true`, unlinked). Default mode returns both; B has `linked: false`. +2. `linked_item_reports_linked_true` — A comes back with `linked: true`, `isGlobal: false`. +3. `global_and_linked_item_appears_once` — item C is global **and** linked; returned exactly + once with `linked: true` (dedup guard). +4. `include_global_false_matches_legacy_scope` — `Without` returns exactly the linked set, + asserting parity with the pre-change filter. +5. `include_global_only_returns_globals_regardless_of_link` — `Only` returns B and C, not A. +6. `search_and_type_filters_apply_to_unioned_globals` — `search=` narrows within the union + (guards the union-before-filter ordering from step 2). +7. `invalid_include_global_value_is_rejected` — `IncludeGlobal::parse(Some("bogus"))` is + `Err`, and `parse(None)` is `Ok(With)`. + +### 5.2 Integration tests — 2, appended to `src-tauri/tests/vault_integration.rs` + +Same `tempdir()` + `#[tokio::test]` style as the existing 7 tests. + +8. `test_db_list_items_preserves_is_global_flag` — `upsert_item(..., true)` and + `upsert_item(..., false)`; assert tuple index 4 round-trips per row + (`db/mod.rs:299-320`). Guards the data path the API filter now depends on. +9. `test_db_set_item_global_roundtrip` — `set_item_global(id, true)` (`db/mod.rs:374`) then + `list_items()` reflects it; flip back to `false` and re-assert. + +### 5.3 Deferred to issue #11's harness — 1 + +`http_list_items_includes_unlinked_global` — full `GET /items?project=…&environment=…` +against a booted router with an unlocked temp vault, asserting the JSON contains +`"isGlobal":true,"linked":false`. Track as a checklist item on #11 rather than a blocker here. + +### 5.4 Manual verification (the issue's repro, inverted) + +1. Create a global item, link it to nothing. +2. `curl 'https://127.0.0.1:47821/items?project=X&environment=Y'` → item present, + `linked:false`. +3. Same with `&include_global=false` → absent. +4. `crypt-env search ` → present, `SCOPE = global`. +5. `POST /fill` with a template referencing that key → key **not** filled, reported as a + warning (unchanged behaviour — proves the materialization split holds). + +--- + +## 6. Risks and monitoring + +| Risk | Early warning | Mitigation | +|---|---|---| +| A consumer treated `GET /items` as the materialization set and silently changes behaviour | Drift/diff scripts stop reporting missing keys | `linked` flag + `include_global=false` + explicit doc row; call it out in the PR description and release notes | +| Item-name collisions between a global and a linked item confuse name-based lookups (`crypt_env_generate_env`, `crypt-env cmd/exec`) | Wrong value injected, or "ambiguous name" reports | Linked-wins tie-break plus stderr warning (step 5); the existing name-vs-key mismatch note in `docs/reference.md` already flags this family of bugs | +| Larger `/items` responses in vaults with many globals | Noticeably slower CLI/MCP listings | `decrypt_all_items` already decrypts the whole vault on every call — the union adds no decryption work, only response size. Revisit only if measured | +| Perceived "secret leakage across projects" | User confusion in review | No plaintext is added: `redact_item` still strips `value`/`password`/`content`, and `GET /items/:id` is already unscoped — scope is documented as a display filter, not an access boundary | +| Scope creep into item-creation semantics | PR diff touching `handle_create_item` | Explicitly out of scope (§4.5); open a follow-up issue | + +**Assumption that would invalidate this plan:** that `is_global` is intended as +"available to link", not "implicitly present". If the project owner confirms that reading, +Alternative B (§4.3) becomes the correct choice and this plan should be replaced — the +implementation cost is comparable, but the documentation and MCP-tool surface differ +substantially. **This is the one question worth confirming before writing code.** diff --git a/docs/plans/issue-3-wsl-bridge-env-paths.md b/docs/plans/issue-3-wsl-bridge-env-paths.md new file mode 100644 index 0000000..6b8ebd6 --- /dev/null +++ b/docs/plans/issue-3-wsl-bridge-env-paths.md @@ -0,0 +1,256 @@ +# Issue #3 — WSL bridge: reach project `.env` files across the Windows/WSL boundary (Phase 1) + +Status: plan (no code written) +Scope: new `src-tauri/src/wsl/`, `src-tauri/src/lib.rs`, `src-tauri/src/project/mod.rs` (one command signature), `src/components/ProjectManager.tsx` +Related: **#7** (path traversal via environment name), **#8** (silent truncation of `output_path`) — both touch the same write path; see §5, which is the highest-value part of this document. Also #11 (test conventions). +Label: enhancement. **Not a bug fix, not a security fix.** + +> **Stale name correction.** The issue names the picker `workspace_pick_env_path`. It was renamed during the projects/environments migration and is now **`project_pick_env_path`**, at `src-tauri/src/project/mod.rs:416`, registered in `lib.rs:213`. Every reference below uses the current name. + +--- + +## 1. Objective + +Definition of done — Phase 1 only. Each item is independently checkable. Phase 2 is explicitly **not** committed (§6). + +1. **`wsl_list_distros()` exists and is honest about absence.** New Tauri command in a new `src-tauri/src/wsl/` module, registered in `lib.rs`. Returns `Ok(Vec)` of installed distro names. Returns `Ok(vec![])` — **never `Err`** — when `wsl.exe` is not on `PATH` (`io::ErrorKind::NotFound`), when WSL is installed with zero distros, and on every non-Windows build target. Returns `Err` only for a timeout or a genuinely unparseable response. +2. **The parser is pure and covered.** `wsl::parse_distro_list(&[u8]) -> Vec` takes raw child stdout bytes and returns names. Covered by **10 unit tests** (T1–T10, §7.1) in an in-file `#[cfg(test)] mod tests`, over fixtures that include a real captured UTF-16LE-with-BOM sample, a BOM-less UTF-16LE sample, UTF-8 (the `WSL_UTF8=1` path), UTF-8-with-BOM, empty output, CRLF/trailing-blank-line noise, and a name containing a space. +3. **The command cannot hang the UI.** `wsl_list_distros` completes or errors within a hard 10 s budget, implemented with `tokio::process::Command` + `kill_on_drop(true)` inside `tokio::time::timeout`. It never acquires the `SharedState` mutex, so it cannot block unlock, save, or inject. +4. **A "Browse WSL" affordance exists in the environment path picker.** In the PATHS block of the environment editor (`src/components/ProjectManager.tsx:1138-1177`, next to the existing browse button at L1163): a control that lists distros, and on selection opens the existing native dialog **pre-seeded** at `\\wsl.localhost\\home\\` (or `\\wsl.localhost\\home\` when the user directory cannot be determined unambiguously — see §3.4). The control does not render on non-Windows platforms, and does not render when the distro list is empty. +5. **Seeding is verified, not assumed.** The seed directory is probed for existence before the dialog is opened. If the probe fails (stopped distro, unsupported UNC form, WSL not running), the user sees an actionable message and falls back to the manual path input that already exists at `ProjectManager.tsx:1155` — the dialog is not opened at a wrong location. +6. **Manual verification passed on real hardware.** All of M1–M9 (§7.2) checked off, including the issue's own gate: `std::fs::write` (used by `project::inject_environment`, `src-tauri/src/project/mod.rs:357`) actually writes correctly to a real `\\wsl.localhost\...` path, and re-injection preserves unrelated keys already in that file. +7. **The UNC requirements in §5 have been communicated to #7 and #8** and appear in their plans. Phase 1 is **not closeable** while `inject_environment` still does `std::fs::read_to_string(path).unwrap_or_default()` at `src-tauri/src/project/mod.rs:328` — see §3.6, this is a data-loss gate, not a nicety. +8. **Nothing else changed.** No new Cargo dependency. No change to the API bind (`127.0.0.1:47821`). No Linux/macOS compilation target added. No DB schema change, no migration, no persisted state. `cargo check` clean on Linux (the maintainer's `cargo check` host) and on Windows. + +--- + +## 2. What is being mitigated + +**Checkable statement of the removed friction:** + +> After this change, a Windows user whose project lives inside a WSL distro can attach that project's `.env` to an environment by clicking a distro name and picking the file in the normal file dialog — without knowing the UNC syntax, without knowing their distro's registration name, and without silently attaching a path that does not currently resolve. + +This is an **enhancement**, and the baseline it improves on is not "impossible" — it is "undiscoverable". Today the manual path input at `ProjectManager.tsx:1155` already accepts any string, and Windows already resolves `\\wsl.localhost\Ubuntu\home\me\app\.env`. A user who knows that can type it and inject works. So the honest accounting is: + +| | Before | After | +|---|---|---| +| Attach a WSL `.env` at all | Possible, if you know the UNC form and your distro's exact registered name | Two clicks, no knowledge required | +| Discover that it is possible | Nothing in the UI suggests it | A visible "WSL" control appears exactly on machines that have WSL | +| Typo in distro name / distro not started | Path is stored, inject fails later (or worse, §3.6) | Probe fails at pick time, before the path is stored | +| No WSL installed / not on Windows | — | Affordance absent; no dead UI, no error | + +**Who this actually helps.** Windows developers who run WSL2. That is a narrow slice of any general user base, but this project is Windows-first by construction — NSIS is the primary bundle target, biometric unlock is Windows Hello (`src-tauri/src/biometric/mod.rs:20`), and the documented dev loop is PowerShell. Among Windows developers who keep `.env` files in a secrets manager, keeping the actual source tree in WSL is the common case, not the exotic one. The maintainer is the archetypal user: per `CLAUDE.local.md`, this repository itself lives at `/home/maosuarez/Programas/crypt-env` inside WSL2 Ubuntu and is built from Windows against `\\wsl.localhost\Ubuntu\...`. That matters operationally, because item 6 of §1 is a **manual** gate that cannot be automated in CI — and there is real hardware to run it on. + +**Cost proportionality.** One new module of roughly 150 lines including tests, one command, one optional argument on an existing command, one UI control, zero new dependencies, zero persisted state, and a rollback that is three file reverts (§8). For a feature that removes a daily papercut for the person maintaining it, this is proportionate. If it were expensive, the correct answer would be a README paragraph (§4, alternative A) — and that alternative is not a strawman. + +**Explicitly NOT mitigated:** +- Running the CLI, MCP server, or API client from inside WSL. That is Phase 2, uncommitted (§6). +- Any secret ever crossing the boundary by a route other than a file the user explicitly chose. +- The stopped-distro silent-overwrite failure, which is **#8's** helper to fix (§3.6, §5). + +--- + +## 3. Implementation steps + +Ordered. Steps 1–3 are Rust-only and independently testable; step 4 is the UI; steps 5–6 are gates. + +### 3.1 Step 1 — New module `src-tauri/src/wsl/mod.rs` + +Declared in `lib.rs` alongside the existing `mod` list (`api`, `biometric`, `cli`, `crypto`, `db`, `mcp`, `project`, `share`, `tls`, `vault`). A dedicated module — not a helper inside `project` — for two reasons: the command name `wsl_list_distros` mandated by the issue only satisfies the `module_action` convention if `wsl` is a module, and the module must stay decoupled (it knows nothing about `db`, `vault`, `api`, or `SharedState`; `project` does not call into it; the frontend composes the two). + +Public surface — three functions, one of them a command: + +- `pub fn parse_distro_list(bytes: &[u8]) -> Vec` — pure, no I/O, no `unwrap`. Decode → split → trim → drop empties. Decoding order: + 1. UTF-8 BOM (`EF BB BF`) → strip, decode UTF-8 lossy. + 2. UTF-16LE BOM (`FF FE`) → strip, decode via `String::from_utf16_lossy` over `chunks_exact(2).map(u16::from_le_bytes)`. + 3. No BOM, but length is even **and** every second byte in the first 32 bytes is `0x00` → treat as BOM-less UTF-16LE, same decode. (This case is real: some `wsl.exe` builds omit the BOM on `--quiet`.) + 4. Otherwise → UTF-8 lossy. + Then `lines()`, `trim()` (this also disposes of `\r` from CRLF and of any stray NUL), drop empty lines. No filtering of names (see §4, decision D6). +- `pub fn unc_root(distro: &str) -> String` — pure. Returns `\\wsl.localhost\\`. A sibling `pub fn unc_root_legacy(distro: &str) -> String` returns `\\wsl$\\`. Both testable on Linux. +- `#[tauri::command] pub async fn wsl_list_distros() -> Result, String>` — see step 2. +- `#[tauri::command] pub async fn wsl_distro_home(distro: String) -> Result` — see step 3. + +In-file `#[cfg(test)] mod tests` per the convention #11 establishes. These tests need **no** database and therefore do **not** consume #11's `test_support` harness — they are pure-function tests, exactly the category #11 puts in-file. Nothing is invented here; nothing is duplicated from there. + +### 3.2 Step 2 — `wsl_list_distros()` + +Command body, Windows arm: + +- Spawn `wsl.exe --list --quiet` with `tokio::process::Command`, `.env("WSL_UTF8", "1")`, `.kill_on_drop(true)`, stdout+stderr piped, and — because this is a GUI process — `CREATE_NO_WINDOW` (`0x0800_0000`) via `std::os::windows::process::CommandExt::creation_flags`, so no console flashes on screen. +- Wrap the `output()` future in `tokio::time::timeout(Duration::from_secs(10), …)`. +- Map results: spawn error with `ErrorKind::NotFound` → `Ok(vec![])`. Any other spawn error → `Err` with the `io::Error` message (no path or secret content — there is none here). Non-zero exit status → `Ok(vec![])` (WSL present but reporting no installation; `wsl --list` on a WSL-less-but-stubbed system exits non-zero with a "no installed distributions" message that is localized and therefore not worth parsing). Timeout → `Err("WSL did not respond within 10s")`. +- Success → `parse_distro_list(&output.stdout)`. + +Non-Windows arm: `#[cfg(not(target_os = "windows"))] { Ok(Vec::new()) }`. The command is registered unconditionally in `lib.rs` on all targets — only its body is `cfg`-gated. Rationale in §4, decision D5. + +No `unwrap()`, no `expect()`. No `SharedState`, no vault key, no logging of anything beyond distro names (which are not secrets, but are also not logged — nothing in this module logs). + +### 3.3 Step 3 — `wsl_distro_home(distro)` + +Returns the directory the dialog should be seeded at, or an `Err` the UI can show verbatim. + +1. Reject a `distro` argument containing `\`, `/`, `..`, or a NUL. It is going straight into a path; it comes from a list we produced, but the command is invokable with anything. +2. Build `\\wsl.localhost\\home\`. Probe with `std::path::Path::new(&p).is_dir()`, itself wrapped in the same 10 s `spawn_blocking` + `timeout` shape (a UNC probe against a cold distro blocks; see §4, decision D4). +3. If that fails, retry once with the legacy `\\wsl$\\home\` form. If that also fails → `Err("cannot reach \\\\wsl.localhost\\ — is the distro running?")`. +4. On success, `read_dir` the `home` directory. If it yields **exactly one** entry and that entry is a directory, return that child path (`…\home\\`). Otherwise return `…\home\`. No guessing from the Windows username — see §4, decision D3. +5. Never propagate the raw `io::Error` for the `read_dir` step; a failure there is not fatal, it just means we return `…\home\`. + +**Documented side effect:** touching `\\wsl.localhost\\…` starts the distro if it is stopped. Clicking "Browse WSL" can therefore boot a WSL VM, taking several seconds and consuming memory. This is inherent to the UNC bridge, not to our implementation, and it is why the whole path is behind an explicit user click rather than being probed eagerly when the environment editor opens. It must be stated in the button's tooltip. + +### 3.4 Step 4 — Seed the existing dialog + +Change `project_pick_env_path` (`src-tauri/src/project/mod.rs:416`) from `()` to: + +``` +pub async fn project_pick_env_path(start_dir: Option) -> Result, String> +``` + +and, when `start_dir` is `Some`, call `.set_directory(dir)` on the `rfd::FileDialog` builder inside the existing `spawn_blocking`. Everything else about the command is unchanged. The existing frontend call site (`ProjectManager.tsx:732`, `invoke('project_pick_env_path')` with no arguments) keeps working: Tauri deserializes a missing argument into `None`. + +This is the **only** one of the five `rfd::FileDialog` call sites that gains WSL awareness. The other four — `project_export` (`project/mod.rs:496`), `project_import` (`project/mod.rs:513`), and the two in `src-tauri/src/vault/share_commands.rs:166,188` — are left alone. Rationale in §4, decision D7. + +Frontend, `src/components/ProjectManager.tsx`: + +- Add `const [isWindows] = useState(() => platform() === 'windows')` using `@tauri-apps/plugin-os`, mirroring the existing pattern in `src/components/WindowChrome.tsx:3,15`. Nothing new is added to Cargo or package.json. +- On mount of the environment editor, when `isWindows`, `invoke('wsl_list_distros')` once and hold the result in local component state. Not in the Zustand store (`src/store/projectStore.ts`) — it is ephemeral machine state, not vault state, and nothing else needs it. Not a TanStack Query either; a single fire-and-forget on an editor that is already mounted per-session is enough, and adding a query key for it is ceremony. +- Render, in the PATHS row at `ProjectManager.tsx:1154-1176` beside the existing browse button (L1163), a "WSL" button that is present only when `isWindows && distros.length > 0`. With one distro it acts directly; with several it opens a small inline list (the existing panel/select styling in this file, Tailwind classes only, no new component library). +- Click handler: set a busy state on the button → `await invoke('wsl_distro_home', { distro })` → on success `await invoke('project_pick_env_path', { startDir: home })` and reuse the exact existing result handling from `handlePickEnvPath` (`ProjectManager.tsx:730-744`, including the `folderNameFromPath` project-name inference) → on failure `showToast(String(e), 'error')` and leave focus in the manual input. Best implemented by extracting the shared tail of `handlePickEnvPath` into a small local helper rather than copying it. +- While busy: the button shows a spinner/disabled state. This is the whole "what the UI shows meanwhile" answer — the list fetch is silent and speculative, and the only user-visible wait is behind an explicit click. + +### 3.5 Step 5 — Register and check + +`src-tauri/src/lib.rs`: add `mod wsl;`, add `wsl::{wsl_distro_home, wsl_list_distros}` to the `use` block (near the `project::{…}` import at L32), add both names to the `invoke_handler!` list (near L213, next to `project_pick_env_path`). Then `cargo check` on Linux and `cargo check` / `pnpm tauri dev` on Windows per `CLAUDE.local.md`. + +### 3.6 Step 6 — The gate that is not ours to write + +`inject_environment` reads each target with `std::fs::read_to_string(path).unwrap_or_default()` (`src-tauri/src/project/mod.rs:328`) and then writes the merged result with `std::fs::write` (L357). It does not call `create_dir_all`. `unwrap_or_default()` treats *any* read failure as "the file is empty". + +For local paths that is merely sloppy. For a `\\wsl.localhost\...` path it is a data-loss path, and it is the **most likely real-world failure of this feature**: the distro is stopped, the read fails, the existing `.env` — which may hold dozens of keys this environment does not manage — is treated as empty, and the write replaces it with only this environment's keys. Silently. Success is reported. + +**#8 owns the write helper.** This plan does not write it. What this plan does: + +- States the requirement: the read must distinguish `ErrorKind::NotFound` (legitimate: create a new file) from every other error kind (`PermissionDenied`, `NotConnected`, the various Windows network errors a dead 9p mount produces) — which must abort that path with an error, not proceed. +- Declares the sequencing gate: **Phase 1 is not closeable while L328 stands as written.** If #8 lands first, this is free. If #8 slips, this plan lands the minimal three-line `match e.kind()` guard at L328 itself and #8 subsumes it later. Shipping "Browse WSL" on top of a silent-overwrite path is not acceptable, and merging the feature while pretending the gate is someone else's problem is exactly how it would happen. + +--- + +## 4. Trade-offs and alternatives considered + +**A. Do nothing; document the UNC path in the README.** *This is the strongest competitor and deserves a straight answer.* Cost: zero. Users can already type `\\wsl.localhost\Ubuntu\home\me\app\.env` into the existing input at `ProjectManager.tsx:1155`, and it already works end-to-end. Rejected because the delta is discoverability and pre-flight validation, not capability: nothing in the UI hints the boundary can be crossed, users must know their distro's *registered* name (which is often not what they call it), and a typo or a stopped distro currently surfaces as a confusing later failure — or, per §3.6, as silent data loss. The feature is convenience, and it should be sold as convenience. If the implementation cost were meaningfully higher than §2's estimate, A would win. + +**B. Ship the Phase 2 CLI-inside-WSL instead, and skip the path picker.** Strength: it is the architecturally correct answer — a client on the Linux side talking to the vault beats poking Linux files through a network share, and it is what Docker Desktop actually does. Rejected for now: three unanswered blocking questions (§6), at least one of which (TLS trust distribution) has no design at all today, and one of which (`127.0.0.1` reachability) may be unanswerable without loosening the API bind, which `CLAUDE.md` marks critical. Deferring is not a judgement that B is worse — it is that B's scope is unknown and Phase 1's is not. + +**C. `--list --verbose` (as the issue literally specifies) vs `--list --quiet`.** *Deviating from the issue here.* `--list --verbose` output is UTF-16LE, column-aligned, carries a `*` default marker, and is **localized** — both the header row and the `Running`/`Stopped` state strings are translated on non-English Windows, so any parser keying on those strings is broken for a large fraction of users, and a column-offset parser breaks on the translated header widths. `--list --quiet` emits one bare name per line: no header, no marker, no localized text, no columns. The only things lost are the running-state and the default-distro flag. Neither is needed: the picker only needs names, and *selecting* a distro starts it anyway (§3.3), so displaying state would be decoration that goes stale the moment it is read. Reading state to gray out stopped distros would be actively wrong — they are perfectly selectable. Decision: `--list --quiet`, no state. This is scope reduction relative to the issue and should be called out at review. + +Also considered: `--list --running` (only started distros — hides exactly the distro the user wants to start) and parsing the registry under `HKCU\Software\Microsoft\Windows\CurrentVersion\Lxss` (no subprocess, no encoding problem, instant — but it is an undocumented implementation detail of WSL that Microsoft can change without notice, and it would need a Windows-registry crate). Both rejected. + +**D1. `WSL_UTF8=1` + defensive decode, vs decode-only.** Setting `WSL_UTF8=1` in the child environment makes recent `wsl.exe` builds (WSL 0.64+, broadly Win11 and updated Win10) emit UTF-8, which sidesteps the whole problem. It is ignored — harmlessly — by older builds. So we set it *and* keep the full BOM/heuristic decoder (§3.1), because "recent enough" cannot be assumed and a wrong guess produces a distro list full of NUL bytes. Belt and braces, and the belt costs one `.env()` call. + +**D2. Hand-rolled UTF-16LE decode vs `encoding_rs`.** Hand-rolled, ~10 lines over `chunks_exact(2)` + `String::from_utf16_lossy`. `encoding_rs` is an excellent, widely-deployed, pure-Rust crate with no Windows-specific risk — this is not a warning about it, it is a scope argument: we decode exactly two known encodings from one known producer, and `CLAUDE.md`'s rule is to justify dependencies, which for this one comes out negative. **Nothing is added to `Cargo.toml` by this plan.** (`CLAUDE.md`'s Windows-dependency warning rule is therefore not triggered; if review disagrees and wants `encoding_rs`, note that it is `no_std`-capable, has no build script beyond a trivial one, and compiles cleanly on `x86_64-pc-windows-msvc` — the warning would be "none known".) + +**D3. `\\wsl.localhost\` vs `\\wsl$\`.** Emit `\\wsl.localhost\` as primary. It is the current form, preferred on Win10 21H2+ and Win11, and is what `CLAUDE.local.md` documents for this very repository. `\\wsl$\` is the legacy form; it still resolves on modern builds as an alias, and it is the *only* form that resolves on pre-21H2 builds where `\\wsl.localhost\` does not exist. Rather than detect the Windows build number — brittle, and the mapping is not clean — §3.3 probes `\\wsl.localhost\` and falls back to `\\wsl$\` once. Two filesystem probes on a click is cheap; version sniffing that silently picks wrong is not. Stored paths therefore normally carry the `.localhost` form, and on old builds carry `\\wsl$\`; both are opaque strings to everything downstream. + +**D4. Guessing `home\\` from the Windows username, vs enumerating.** The Windows username and the WSL username are unrelated and frequently differ. Guessing yields a nonexistent directory, `rfd`'s `set_directory` then silently no-ops, and the dialog opens somewhere arbitrary — the worst outcome, because it looks like the feature is broken rather than unavailable. Enumerating `\\wsl.localhost\\home\` and descending only when there is exactly one child is reliable and self-limiting (multi-user distros stop at `home\`, which is still a useful seed). Its cost is the distro-start side effect, stated in §3.3 and surfaced in the tooltip. Rejected third option: parse `/etc/passwd` through the share — more I/O, more parsing, same side effect, no better answer. + +**D5. Async/timeout strategy.** `tokio::process::Command` + `kill_on_drop(true)` inside `tokio::time::timeout`, not `spawn_blocking` + `std::process::Command`. `spawn_blocking` is the idiom already used in this file for `rfd` (`project/mod.rs:417,495,512`) and would have been the consistent choice — but a `timeout` around a `JoinHandle` does not cancel the blocked thread: it returns while the thread stays parked on a hung `wsl.exe`, leaking a blocking-pool slot for as long as the child lives. `tokio::process` cancels for real and kills the child on drop. `tokio` is already `features = ["full"]` in `src-tauri/Cargo.toml:46`, so `process` is available with no manifest change. The one place `spawn_blocking` *is* still correct is the `is_dir()` UNC probe in §3.3 — there is no async filesystem primitive for it, and the leak window there is bounded by the SMB client's own timeout rather than by a process we control. Accepted, and stated rather than hidden. + +**D6. `std::process::Command` (via `tokio::process`) vs adding `tauri-plugin-shell`.** `CLAUDE.md`'s first-session dependency list mentions `tauri-plugin-shell`, but it is **not** in the actual manifest — `src-tauri/Cargo.toml:23-27` lists `opener`, `global-shortcut`, `clipboard-manager`, `os`, `updater` only. Keep it that way. The shell plugin's purpose is to let the *webview* spawn processes; adding it to run one command from Rust would grant a broad new capability to the frontend of a secrets manager in exchange for nothing, since Rust can spawn processes natively. This is a security argument, not a preference: fewer webview capabilities is strictly better here, and the plugin would also need capability entries that someone later widens. Rejected. + +**D7. Filtering `docker-desktop` / `docker-desktop-data` out of the list.** Tempting — they are WSL distros nobody wants to browse, and `docker-desktop-data` in particular has nothing useful in `/home`. Rejected: it hardcodes one vendor's naming into our parser, it is a guess about intent, and it breaks the moment Docker renames anything. Show what WSL reports. Users recognise their own distros. + +**D8. Non-Windows behaviour: `Ok(vec![])` vs an `Err("unsupported")` vs `cfg`-ing the command out of `invoke_handler`.** `tauri.conf.json:37` targets `["nsis", "dmg", "deb"]`, so this ships on macOS and Linux. `cfg`-ing the registration list is rejected — conditional `invoke_handler!` entries are easy to get subtly wrong and produce a runtime "command not found" that only appears on one platform. `Err` is rejected because it forces the frontend to distinguish "no WSL" from "broken", and the frontend's response to both is identical: hide the button. `Ok(vec![])` collapses non-Windows, WSL-not-installed, and WSL-with-zero-distros into one signal — *there is nothing to browse* — which is exactly the frontend's decision variable. The `platform() === 'windows'` check on the frontend is then a pure optimisation (skip a pointless IPC round trip), not a correctness requirement. + +**D9. Extending `project_pick_env_path` with `start_dir` vs a new `wsl_pick_env_path(distro)`.** A dedicated WSL picker would duplicate the dialog configuration (title, filters) and drag `rfd` concerns into the `wsl` module, coupling it to something it has no business knowing. Worse, it would collapse two distinguishable outcomes — "could not reach the distro" and "user cancelled the dialog" — into one `Option`, and the UI wants to say different things about them. Splitting into `wsl_distro_home` (reachability) + `project_pick_env_path(start_dir)` (selection) keeps one dialog code path and gives the frontend two distinct failures. Cost: two IPC calls per click instead of one. Irrelevant at human latency. + +**D10. Does `rfd`'s `set_directory` accept a UNC path?** `rfd` 0.14's Windows backend drives the native `IFileDialog`, and `set_directory` resolves the path via `SHCreateItemFromParsingName` before `SetFolder`/`SetDefaultFolder`. That API does accept UNC paths, and `rfd`'s own documentation notes the directory must exist. **This is a claim to verify (M3), not to assert** — the failure mode if it is wrong is quiet: `set_directory` no-ops and the dialog opens at the shell default, so the button appears to do nothing. Verification is the §3.3 existence probe plus M3 on real hardware. Fallback if M3 fails: keep `wsl_list_distros` and `wsl_distro_home`, drop the dialog seeding, and have the WSL button instead **insert the resolved UNC prefix into the manual path input** at `ProjectManager.tsx:1155` for the user to complete. That fallback delivers most of §2's value, needs no new Rust, and is a UI-only change — so the feature does not die on this uncertainty. + +--- + +## 5. Cross-cutting: what #7's and #8's helpers must do about UNC paths + +**This is the most important section of this plan.** #7 adds a containment check over `output_dir`-derived paths; #8 adds a no-clobber/marker/backup helper. Both sit directly on `inject_environment`'s write path (`src-tauri/src/project/mod.rs:268-364`), which is precisely the path a WSL `.env` travels. A naive implementation of either will break this feature — or, in the reverse direction, this feature will look like a way around a security control. Requirements, stated so neither happens: + +1. **Explicit `environment.paths` entries are not `output_dir`-derived and must not be forced under any base directory.** #7's containment applies to the `output_dir` join at `project/mod.rs:286-289`, where an attacker-influenced environment *name* is concatenated into a filename. It must **not** be applied to `env.paths` (L280) or to an explicit `output_path` (L281-284): those are absolute paths the user picked in a native dialog, and a WSL `.env` is by definition outside every local base directory. Forcing containment there does not add security — the input is already trusted-by-selection — and it kills this feature outright. +2. **`Path::components()` on a UNC path starts with `Component::Prefix`, not `Component::RootDir`.** `\\wsl.localhost\Ubuntu\home\me\.env` yields `Prefix(PrefixComponent { kind: Prefix::UNC("wsl.localhost", "Ubuntu") })`, then `RootDir`, then normal components. A traversal check that scans for `ParentDir`/`CurDir` components is fine. One that assumes the first component is `Prefix::Disk(_)`, or that a path is "absolute" only if it starts with a drive letter, is not. +3. **`std::fs::canonicalize` on Windows returns verbatim paths, and rewrites the UNC prefix.** `\\wsl.localhost\Ubuntu\x` canonicalizes to `\\?\UNC\wsl.localhost\Ubuntu\x` — the leading `\\` is *replaced*, not extended. Therefore a check of the form `canonicalize(child).starts_with(base)` where `base` was not itself canonicalized **always fails** for UNC inputs. Canonicalize both sides or neither. +4. **`Prefix::UNC` and `Prefix::VerbatimUNC` are unequal values for the same location.** Never compare `Prefix` variants for equality, and never compare a canonicalized path against a non-canonicalized one component-wise. +5. **`canonicalize` requires the target to exist, and a UNC target to a stopped distro does not.** A helper that canonicalizes to decide whether a file already exists must treat a canonicalize failure as **unknown**, never as "does not exist, safe to create". This is #8's core case and it is the same root cause as §3.6. +6. **Verbatim (`\\?\`) paths disable OS-level path normalization — `..` is *not* resolved.** Any helper that canonicalizes a base and then joins untrusted components onto the result loses the protection it thought it had. Reject `..` lexically **before** joining; never rely on canonicalization to strip it. +7. **Case sensitivity is split across the boundary.** The UNC host and share components (`wsl.localhost`, the distro name) compare case-insensitively, like all Windows path prefixes. Everything after them lands on a Linux filesystem through 9p and is **case-sensitive** — `\\wsl.localhost\Ubuntu\home\Me` and `…\home\me` are different directories. A comparison helper that lowercases the whole path to compare is wrong on the tail; one that compares the whole path case-sensitively is wrong on the prefix. If comparison is unavoidable, split at the prefix. +8. **The read side needs the same care as the write side.** #8's helper must cover `read_to_string` at L328, not only `write` at L357 — see §3.6. Distinguishing `ErrorKind::NotFound` from every other error kind is the single required behaviour. +9. **`create_dir_all` is still not called anywhere on this path, and this plan does not add it.** Worth noting for #8: auto-creating parent directories across a 9p mount has different semantics (ownership, permissions) than locally, so if #8 adds it, that is a decision to make explicitly rather than inherit. + +--- + +## 6. Phase 2 — deferred spike, not designed here + +The issue is explicit that a CLI running inside WSL and talking back to the Windows vault is **not committed scope**. This plan does not design it. It records the three blocking questions, and how each gets answered — as a timeboxed investigation producing written answers and no committed code. + +**Q1 — Does building `crypt-env` for Linux drag in Tauri/GTK?** Almost certainly yes: `[[bin]]` targets share package-level dependencies, so the `crypt-env` and `crypt-env-mcp` binaries in `src-tauri` link the same dependency graph as the library, `tauri` included. *How to answer:* in a clean container, `cargo build --bin crypt-env --target x86_64-unknown-linux-gnu` and observe whether `tauri`, `glib-sys`, `webkit2gtk-sys` compile. If they do, Phase 2 requires splitting the CLI into its own crate in a workspace — a substantial refactor that must be scoped separately, not smuggled in. + +**Q2 — Is `127.0.0.1:47821` reachable from WSL2?** A configuration question, not a code question. Under default NAT networking, WSL2 is a separate network namespace: its `127.0.0.1` is its own loopback, not the Windows host's, so the answer is **no** — reaching Windows requires the host IP from `/etc/resolv.conf` (or `$(hostname).local`), and the server does not listen there. Under `networkingMode=mirrored` (`.wslconfig`, Win11 22H2+), host loopback is shared and the answer is **yes**. *How to answer:* `curl -k https://127.0.0.1:47821/health` from inside WSL under both modes, on real hardware. **Constraint that is not negotiable:** the bind stays `127.0.0.1` (`CLAUDE.md`, security-critical). Any Phase 2 design that begins with "just bind `0.0.0.0`" is rejected before it is written. + +**Q3 — How does a WSL client come to trust the self-signed TLS certificate?** No mechanism exists today (`src-tauri/src/tls/`). *How to answer:* produce a written proposal covering where the cert is exported from, how it reaches the Linux trust store or a client-side pin, and what happens on regeneration. **Any proposal that copies a private key into the WSL filesystem is dead on arrival** — the WSL side is a different trust domain with different filesystem permissions. + +**Gate:** Phase 2 is scheduled as committed work only once Q1, Q2, and Q3 have written answers. Until then it is not designed, not estimated, and not promised. + +--- + +## 7. Verification + +### 7.1 Automated — `parse_distro_list` and `unc_root`, in-file `#[cfg(test)] mod tests` + +All pure; all run on Linux in CI; none need a DB, a vault, or #11's harness. A helper `fn utf16le(s: &str, bom: bool) -> Vec` builds synthetic fixtures inline (raw UTF-16 byte literals in source are unreadable), and **one** real capture from the maintainer's machine is committed as `src-tauri/tests/fixtures/wsl-list-quiet.bin` as ground truth for T9. + +| # | Input | Expected | +|---|---|---| +| T1 | UTF-16LE **with** BOM, `"Ubuntu\r\ndocker-desktop\r\n"` | `["Ubuntu", "docker-desktop"]` | +| T2 | UTF-16LE **without** BOM, same content | same | +| T3 | UTF-8, no BOM (the `WSL_UTF8=1` path) | same | +| T4 | UTF-8 **with** BOM (`EF BB BF`) | same | +| T5 | Empty byte slice | `[]` | +| T6 | Only CRLFs and spaces | `[]` | +| T7 | Trailing blank lines, mixed `\n` / `\r\n` | names only, no empties | +| T8 | A name containing a space (`"openSUSE Leap 15.5"`) | preserved verbatim, not split | +| T9 | The committed real `wsl-list-quiet.bin` capture | the exact distro list on that machine | +| T10 | `unc_root("Ubuntu")` / `unc_root_legacy("Ubuntu")` | `\\wsl.localhost\Ubuntu\` / `\\wsl$\Ubuntu\` | + +Not unit-testable, by construction: everything that touches a real UNC path or spawns `wsl.exe`. CI has no WSL. That is why §7.2 is a gate and not a suggestion. + +### 7.2 Manual — the real gate (maintainer's Windows + WSL2 Ubuntu, per `CLAUDE.local.md`) + +| # | Check | Pass condition | +|---|---|---| +| M1 | `wsl_list_distros` on the dev machine | Returns the real distro list; no console window flashes | +| M2 | Same build on a Windows machine/VM without WSL | `Ok([])`, WSL button absent, no error toast | +| M3 | Click WSL → pick distro | Native dialog opens **at** `\\wsl.localhost\Ubuntu\home\maosuarez\` — confirms D10 | +| M4 | Pick a `.env` there | Stored path is the UNC string; it appears in the PATHS list | +| M5 | **The issue's own gate:** inject to that path | `std::fs::write` succeeds; `cat` from inside WSL shows correct content and LF-only line endings (no CRLF conversion over 9p) | +| M6 | Pre-seed the target file with unrelated keys, re-inject | Unrelated keys survive — exercises the L328 read over UNC | +| M7 | `wsl --terminate Ubuntu`, then inject to a stored UNC path | Before §3.6's fix: reproduces the silent overwrite (**expected failure, documents the bug**). After: errors without writing | +| M8 | Force the timeout (e.g. cold-boot WSL then click immediately) | UI stays responsive, button shows busy, error after ≤10 s | +| M9 | Inspect the written file from inside WSL | Ownership/permissions usable by the Linux user; the file is not root-owned or unreadable | +| M10 | macOS or Linux build (`dmg`/`deb` targets) | No WSL button; no console errors from the absent-command path | + +M7 is deliberately listed as a failure to *observe* before it is a check to *pass* — it is the evidence that §3.6's gate is real and not theoretical. + +--- + +## 8. Rollback + +Fully reversible, no data implications: + +- Delete `src-tauri/src/wsl/`; remove `mod wsl;`, the `use`, and the two `invoke_handler` entries from `lib.rs`. +- Revert `project_pick_env_path`'s signature to `()` and drop the `set_directory` call. +- Remove the WSL button and its handler from `ProjectManager.tsx`. + +No schema change, no migration, no settings key, no persisted state — nothing to undo in the vault. UNC paths already stored in `environments.paths` are plain strings and **remain valid and functional after a rollback**; the user simply loses the convenient way to enter new ones. The §3.6 read-error guard, if this plan ends up landing it, is *not* part of the rollback — it is a correctness fix that stands on its own. + +--- + +## 9. Documentation + +Per `CLAUDE.md`, no new `.md` files beyond this plan. On merge: + +- `docs/reference.md` — add `wsl_list_distros` and `wsl_distro_home` to the Tauri command list, and note `project_pick_env_path`'s new optional `startDir` argument. +- A short "WSL paths" note in the user-facing docs stating the `\\wsl.localhost\` form, that browsing starts a stopped distro, and that a stopped distro must be running for inject to work. diff --git a/docs/plans/issue-4-share-whole-project-via-relay.md b/docs/plans/issue-4-share-whole-project-via-relay.md new file mode 100644 index 0000000..7d8456d --- /dev/null +++ b/docs/plans/issue-4-share-whole-project-via-relay.md @@ -0,0 +1,348 @@ +# Issue #4 — Share a whole project (all environments) via relay + +**Label:** enhancement (not a bug) +**Branch base:** `main` (the projects/environments migration is merged — `projects`, `environments`, `environment_vars`, `item_projects` all exist) +**Related:** #11 (test harness, in flight — this plan *consumes* it), #12 (`idx_projects_name_nocase` semantics, in flight — this plan *depends on* its collision error mapping) + +--- + +## 0. Read this first — the value gate + +The issue asks to build a feature. Before the implementation steps, here is the honest case for and against, because a cheaper action closes the issue's stated complaint. + +### 0.1 What the investigation actually found + +The dead-protocol claim in the issue is **true but understated**. `handle_workspace_relay_send` / `handle_workspace_relay_receive` in `/home/maosuarez/Programas/crypt-env/src-tauri/src/api/mod.rs` do not merely lack a UI — they read and write the **legacy `workspaces` / `workspace_vars` tables**, which post-migration no product surface writes to and no product surface reads from: + +- `handle_workspace_relay_send` (L2662, L2676) calls `db.list_workspaces()` / `db.get_workspace_vars(id)`. Those tables are now **frozen backfill sources only** — `db/mod.rs` L209-212 drains them once into each project's default environment. Sending from them ships a snapshot of pre-migration state, not the user's current project. +- `handle_workspace_relay_receive` (L3004, L3026) calls `db.upsert_workspace()` / `db.set_workspace_vars()`. It writes rows into `workspaces` that **nothing can display**: the GUI, the CLI (`crypt-env project`) and the MCP project tools all read `projects`/`environments`. The one-time backfill is gated by a settings flag and will never re-run. The received secrets *are* created as real `items` rows, but they land with **zero owners** and are not linked into any environment, so they surface only in Global Secrets, unlabelled. + +`/home/maosuarez/Programas/crypt-env/docs/reference.md` L45-46 already documents both endpoints as "Legacy… items imported this way are NOT linked into any project/environment and are invisible to the scoped endpoints above". So this is not undiscovered — it is **known-broken code with a live MCP surface** (`crypt_env_share_workspace_send` / `_receive`, `crypt-env-mcp.rs` L2077 / L2112) that an LLM agent can call today and get a silent data black hole. + +That is a stronger reason to act than "unused code exists". + +### 0.2 What already covers the user workflow + +Project-level sharing is **not** absent today. Two surfaces already exist: + +| Surface | Carries structure | Carries values | Where | +|---|---|---|---| +| `project_export` / `project_import` | yes (env names, `isDefault`, var **keys**) | **no** — `literal: None` is hard-coded, deliberately | `project/mod.rs` L458-525, wired in `ProjectManager.tsx` L676/L685 | +| `crypt-env relay receive --project X --env local` | no | yes | `commands/relay.rs` L23-36 | + +So the concrete gap is: **one operation that carries structure *and* values for N environments at once.** Today the same outcome takes: export template → send template file → receiver imports → sender selects the items in ShareModal → relay-send → receiver relay-receives into each environment, once per environment. + +That is a real ergonomic win for the "onboard a teammate onto a project" workflow, but it is an ergonomic win, not a new capability. Do not oversell it. + +### 0.3 Recommendation + +**Do both, in this order, as separable PRs:** + +1. **Phase 0 — delete the workspace relay surface. Unconditional, ships on its own.** This alone closes the issue's stated complaint ("finished protocol with no product surface") and removes a broken, agent-reachable code path. If the feature below is never built, the repo is still strictly better. ~1 day. +2. **Phase 1-3 — build the project relay.** Recommended, with a **reduced surface**: protocol + Tauri + GUI + CLI. **Drop the MCP tool** (see D9). ~3-4 days. + +**Why not delete-only:** the receive side is where the value concentrates. A teammate reconstructing a runnable three-environment project in one step is materially better than seven manual steps, and the safety default the issue proposes (non-default environments unchecked) makes the values-carrying version *safer* than the current "select the items by hand in ShareModal" flow, which has no environment awareness at all. + +**Why not build-only:** deleting the legacy handlers is a prerequisite, not an afterthought. Leaving `/workspaces/*/relay/*` alive next to `/projects/*/relay/*` guarantees someone (or some agent) picks the wrong one. + +**If the maintainer disagrees on value:** ship Phase 0, close #4 with the deletion, and open a new issue for the feature. That is a legitimate outcome of this plan and it is cheap. + +--- + +## 1. Objective + +Definition of done. Every item below is independently checkable. + +### 1.1 Deleted + +- `WorkspaceBundle`, `WorkspaceBundleVar`, `WorkspaceBundle::KIND`, `encrypt_workspace`, `decrypt_workspace` are gone from `src-tauri/src/share/relay.rs`. +- Routes `POST /workspaces/:id/relay/send` and `POST /workspaces/relay/receive` are gone from the router in `api/mod.rs`; handlers `handle_workspace_relay_send`, `handle_workspace_relay_receive` and the response structs `WorkspaceRelaySendResponse` / `WorkspaceRelayReceiveResponse` are deleted. +- MCP tools `crypt_env_share_workspace_send` / `crypt_env_share_workspace_receive` and their handlers `tool_share_workspace_send` / `tool_share_workspace_receive` are gone from `crypt-env-mcp.rs`, and removed from the `tools/list` manifest. +- `grep -rn "WorkspaceBundle\|workspace_relay\|share_workspace" src-tauri/src src/` returns **zero hits**. +- The `workspaces` / `workspace_vars` / `workspace_paths` tables and their `db` accessors are **kept** (they are still the migration backfill source — see D10). + +### 1.2 Protocol + +- `ProjectBundle` in `share/relay.rs` with `kind: "project"`, `version: 1`, and `environments: Vec`; items hoisted to the bundle root, referenced by name. +- `encrypt_project(&ProjectBundle, &[u8;32]) -> Result` and `decrypt_project(&str, &[u8;32]) -> Result`, the latter rejecting a wrong `kind` **and** an unknown `version` before returning. +- No `literal` field anywhere in the new format (D3). No `paths` field anywhere in the new format (D6). + +### 1.3 Delivered surfaces + +| Surface | Name | File | +|---|---|---| +| HTTP | `POST /projects/:id/relay/send` | `api/mod.rs` | +| HTTP | `POST /projects/relay/receive` | `api/mod.rs` | +| Tauri | `project_relay_send(project_id, environment_ids) -> RelayShareResult` | `project/relay_commands.rs` (new), registered in `lib.rs` | +| Tauri | `project_relay_receive(code, passphrase, project_name_override) -> ProjectReceiveResult` | same | +| CLI | `crypt-env project share --id N --envs a,b` | `bin/crypt-env/commands/project.rs` | +| CLI | `crypt-env project receive --code X --passphrase Y [--as NAME]` | same | +| GUI | "SHARE PROJECT" button on the project detail view, opening an environment checklist + key manifest + confirm, then the existing code/passphrase display | `ProjectManager.tsx` + new `ProjectShareModal.tsx` | +| GUI | "RECEIVE PROJECT" on the projects list view | same | + +MCP: **no new tool** (D9). + +### 1.4 Tests (all in `src-tauri`, no network) + +Named cases, consuming the #11 harness (`test_support::{unlocked_vault, seed_project, seed_item, link_var, read_item}`): + +**Pure, in `share/relay.rs` `#[cfg(test)] mod tests`:** +1. `project_bundle_roundtrip_preserves_structure` — encrypt → decrypt returns identical env names, `is_default` flags, var keys and item names. +2. `decrypt_project_rejects_items_payload` — a payload produced by `encrypt_items` fails with `ShareError::Protocol`, not a panic and not a successful parse. +3. `decrypt_project_rejects_unknown_version` — a bundle with `version: 99` is rejected with a message naming the supported version. +4. `decrypt_project_rejects_wrong_passphrase` — a different `derive_relay_key` input yields `ShareError::Crypto`/decrypt failure. + +**Integration, in `src-tauri/tests/project_relay.rs` (`#[tokio::test]` + `tempfile`, transport stubbed — bundle handed directly to the receive-side function):** + +5. **`share_three_env_project_dedups_reused_items`** — the round-trip case named in the objective. Seed a project with 3 environments (`local` default, `staging`, `production`) and 5 distinct items, 2 of which are linked into all 3 environments. Build the bundle for all 3 environments; assert `bundle.items.len() == 5` (not 11), and `bundle.environments.iter().map(|e| e.vars.len()).sum() == 11`. Receive into a fresh vault; assert the receiving vault contains **exactly 5 new `items` rows**, **11 `environment_vars` rows**, **3 `environments` rows**, **5 `item_projects` rows all pointing at the new project**, and that the two shared items appear as **one row each** (query `items` by decrypted name, assert count 1) — i.e. **0 duplicated ciphertext rows**. +6. `receive_refuses_case_insensitive_project_name_collision` — receiving `MyApp` into a vault that has `myapp` returns the typed conflict error, and the vault is **unchanged**: no new `items`, `environments`, `environment_vars` or `item_projects` rows (transaction rollback, D5/D7). +7. `receive_with_name_override_succeeds_after_collision` — same bundle, `project_name_override = "MyApp-received"`, completes and produces the row counts from case 5. +8. `send_excludes_unselected_environments` — selecting only the default environment from the 3-env project yields a bundle with 1 environment and only the items reachable from it; an item used *only* by `production` is absent from `bundle.items`. +9. `send_skips_dangling_item_reference` — an `environment_vars` row whose `item_id` no longer resolves is skipped without failing the send (mirrors the existing L2707 behaviour), and the resulting bundle's var count is one lower. +10. `received_items_are_project_owned_not_global` — every received item has `is_global = false` and exactly one `item_projects` row. +11. `bundle_never_contains_paths` — serialize a bundle built from a project whose environments have `paths` set; assert the JSON string contains none of those path substrings (D6 is enforced by the type, this test guards regressions). + +**Test gate:** `cargo test` green; case 5 and case 6 are the two that must not be weakened during review. + +### 1.5 Docs + +`docs/reference.md` rows L45-46 replaced with the two `/projects/...` routes; MCP table rows L251-252 and the L282 backward-compatibility paragraph deleted; `CHANGELOG.md` entry under Unreleased noting the **breaking** removal of the workspace relay endpoints and MCP tools. + +--- + +## 2. What is being mitigated + +This is an enhancement. Stated as two checkable claims, without inflation. + +**(a) Dead-and-broken protocol code with zero product surface is resolved — either shipped or deleted.** + +| Check | Today | After | +|---|---|---| +| `grep -c "WorkspaceBundle" src-tauri/src` | 12 | 0 | +| Endpoints backed by frozen legacy tables | 2 | 0 | +| MCP tools that write rows no surface can read | 2 | 0 | +| Protocol structs with no consumer | `WorkspaceBundle`, `WorkspaceBundleVar` | none | + +The specific defect removed: an agent calling `crypt_env_share_workspace_receive` today decrypts real secrets, writes them into the vault as **ownerless items**, and rebuilds the project into a table the application no longer reads. The user sees loose secrets in Global Secrets and no project. Nobody has reported it because the tool is undiscoverable — that is the *reason* it is worth removing rather than an argument that it is harmless. + +**(b) The user workflow unlocked.** + +> "Give my teammate everything they need to run this project" becomes one send + one receive, instead of: export template file → transfer it → import → open ShareModal → hand-select the items belonging to each environment → relay-send → relay-receive with `--env`, repeated per environment. + +Honest bound on the value: the *capability* mostly exists (see §0.2). What is new is (i) doing it in one step, (ii) preserving multi-environment structure and values together, and (iii) the sender seeing an explicit, per-environment list of exactly which keys are about to leave the machine — which the current item-picker flow does not provide, because it has no idea which environment an item belongs to. Item (iii) is the strongest security argument for building the feature rather than deleting only. + +**Not claimed:** this does not make sharing more secure at the transport layer (same relay, same AES-256-GCM, same Argon2id, same 24h TTL, same burn-after-read), does not add access control, and does not reduce the trust placed in the Supabase relay operator. + +--- + +## 3. Decisions (with trade-offs) + +### D1 — `ProjectBundle` shape: nested environments, root-level deduped items + +``` +ProjectBundle { + kind: "project", // discriminator, checked on decrypt + version: 1, // numeric, checked on decrypt + name, description?, template, + environments: [ EnvironmentBundle { name, is_default, vars: [ { key, item_name } ] } ], + items: [ PlainItem ] // deduped by name, referenced from vars by item_name +} +``` + +Rejected: (i) items nested inside each environment — simplest to build, but duplicates the ciphertext-bearing payload once per environment and makes "same item in 3 envs" indistinguishable from "3 items with the same name" on receive, which is exactly the bug the issue asks to avoid; (ii) reusing `ExportedProject` from `project/mod.rs` by adding an optional values field — attractive (one format for file export and relay) but it would make it possible to *accidentally* write values into a `.cryptenv-proj` file on disk, and that file format's whole point is that it never carries values. Keeping the two formats separate is the safer default. Cost accepted: two similar structs to maintain. + +**Reference key is `item_name`, not an id.** Ids are meaningless across vaults. Cost: two items with the same name in the sender's vault collapse into one in the bundle. That is already the behaviour of the existing workspace bundle (`bundled.entry(item_name)` at L2711) and of `import_plain_items_into_vault`. Accepted, but the send-side preview (D4) must show the deduped list so the sender sees the collapse. + +### D2 — `version` field, checked + +`decrypt_project` rejects `version != 1` with `ShareError::Protocol("this package was created by a newer version of CryptEnv (format v{n}); update to receive it")`. Without this, the next format change repeats today's problem: a second discriminator swap. Cost: one field, one branch. Do this now — it is free before the first release and impossible after. + +### D3 — `literal` does **not** survive into the new format + +`environment_vars.item_id` is mandatory post-migration and `vault::migrate_literal_vars_to_items` (gated by `settings['migrated_literals_v1']`) converted legacy literals into real items. Carrying `literal` forward would re-introduce a plaintext-value-in-a-var path that the data model deliberately removed, and would give the receive side two code paths where one suffices. + +**Legacy rows that still hold a literal:** they exist only if the migration failed or was skipped. The send side reads through `project::list_projects` → `EnvironmentVar { item_id: i64 }`, which already cannot represent a literal — such rows are invisible to it. Explicit behaviour: **they are silently absent from the bundle**, exactly as they are already absent from every other project-scoped read path. No new handling, no new error. Documented here so nobody "fixes" it later by re-adding the field. + +### D4 — Sender sees the keys, never the values, and confirms + +**This is the highest-value safety affordance in the feature and it is not in the issue's checklist.** Before upload the GUI shows, per selected environment, the list of `KEY → item name` pairs that will leave the machine, with the count, and a confirm button. No values, ever — the manifest is built from data the frontend already holds via `project_list`, so **no new backend command and no new decryption is required**. The CLI prints the same manifest and requires `--yes` or an interactive `y/N` confirmation. + +Rejected: a `project_relay_preview` Tauri command. Rejected because it adds a command and a decryption pass for information the frontend already has. + +**Environment selection default:** all non-default environments **unchecked**. Extended reasoning beyond the issue's: the failure mode is asymmetric and unrecoverable. Under-sharing costs one extra round trip; over-sharing puts production credentials in a third-party relay row and in a teammate's vault, and the only remediation is rotating every leaked secret. Additionally, any environment whose name matches `prod|production|live|release` (case-insensitive) renders with a distinct warning treatment and requires the confirm step to name it explicitly. Cost: a heuristic on names, which is inexact — accepted, because it only *adds* friction, never removes it. + +### D5 — Receive creates a **new** project; a name collision is a hard error + +Rejected: (i) **merge by name** — silently mutating an existing project's environments and repointing vars is destructive, invisible, and would let a sender overwrite a receiver's production values by choosing a matching project name. Rejected on security grounds. (ii) **auto-rename to `MyApp (2)`** — non-destructive, but produces confusing duplicates and hides the collision from the user. + +Behaviour: the receive path checks for a case-insensitive project-name match **before writing anything**. On collision it returns a typed error carrying the colliding name, and the caller may retry with `project_name_override` (Tauri) / `--as NAME` (CLI). The GUI catches the error and shows a rename field pre-filled with `"{name}-received"`. + +**Interaction with #12:** `idx_projects_name_nocase` already exists on `projects`. Relying on the index alone would surface a raw SQLite `UNIQUE constraint failed` string through the error path — which #12's plan explicitly forbids (no SQL text in responses) and which SQLite's ASCII-only `NOCASE` would miss for non-ASCII names anyway. So the check is an **application-level Unicode-aware pre-check** in the receive function, with the index as the backstop. HTTP maps it to `409 CONFLICT`, error code `CONFLICT`, consistent with #12 step 5. **Sequencing note:** if #12 lands first, reuse its `is_unique_violation` helper rather than duplicating it. + +### D6 — Environment `paths` are stripped, not shipped + +`paths` are absolute filesystem paths from the sender's machine: `C:\Users\maosuarez\dev\myapp\.env.production`. They are meaningless on the receiver's machine (inject would write to a path that does not exist, or worse, one that does), and they leak the sender's OS, username and directory layout to both the receiver and — as ciphertext the relay operator can size-analyse, though not read — the relay. + +Decision: **the field does not exist in `EnvironmentBundle`.** Not "included but ignored", not "offered as a review step" — absent from the type, so it cannot be leaked by a future code path. This matches the existing precedent twice over: `project_export` drops paths deliberately (L431-433) and `handle_workspace_relay_receive` already notes "no paths — receiver sets their own .env targets" (L3002). + +Cost: the receiver must set paths per environment before the first inject. Mitigation: the GUI's post-receive toast links straight into the environment editor. Accepted — this is a one-time step and the alternative is a leak. + +Rejected: "offer paths as a review step so the receiver can adapt them." Rejected because it makes the leak the default and the redaction opt-out. + +### D7 — Item ownership and the transaction boundary + +On receive, for a bundle with `I` unique items and `V` total var links across `E` environments: + +- `I` rows in `items` — one per unique `item_name`, encrypted with the **receiver's** vault key. `is_global = false`. +- 1 row in `projects`. +- `E` rows in `environments`. Exactly one has `is_default = true` — if the bundle's selected set contains no default (because the sender deselected it), the **first** environment in the bundle is promoted, so the receiver never ends up with a project that has no default environment. +- `I` rows in `item_projects` — every received item is owned by the new project, **not global**. Rationale: a received item's provenance is one project; promoting it to global would make it appear in Global Secrets and be reusable across the receiver's unrelated projects, which is a scope decision only the receiver should make (they can toggle it afterwards with `vault_set_item_global`). +- `V` rows in `environment_vars` — an item linked into 3 environments produces 3 rows pointing at **one** `items` row. This is the dedup assertion in test case 5. +- 0 rows in `project_categories` — categories are the receiver's taxonomy; the bundle does not carry them. + +**Transaction boundary: the entire receive is one SQLite transaction.** Today's code is not transactional — `handle_workspace_relay_receive` upserts items one at a time and a failure midway leaves orphans. With a project bundle the orphan blast radius is much larger, and the burn-after-read delete has already fired by then, so the payload is **unrecoverable**. All-or-nothing is not optional here. + +**Module decoupling (CLAUDE.md: `db` must not know about `api`; `vault` orchestrates).** The `db` layer must not decrypt or encrypt. Therefore: + +1. `vault`/`project` layer encrypts each `PlainItem` with the vault key, producing opaque ciphertext strings. +2. It hands `db` a single plain-data struct (project row + environment rows + `(item_name → ciphertext, item_type, created)` + var links by name) via one new function, e.g. `db::insert_received_project(...) -> Result`, which opens one transaction, does the name pre-check, inserts everything, and commits. +3. `db` sees ciphertext strings and never a key. `api` and the Tauri command both call the `project`-layer orchestrator, never `db` directly. + +**Do not reuse `share::import_plain_items_into_vault` for the item writes.** It is the right helper for the *item* relay path and it owns the collision/skip behaviour there (the code path where #11 found a prior bug), but its contract is "import loose items, optionally linking into one existing environment" — it takes `link: Option<(i64, i64)>`, a single pair. A project bundle needs N environments, a new project, and one transaction spanning all of it. Forcing it through that signature would mean N calls, N implicit transactions, and no rollback. **Reuse its per-item encrypt-and-upsert body by extracting it into a shared private helper**; do not reuse the outer function. Note this explicitly in the PR so a reviewer does not read it as reinvention. + +### D8 — Payload size: cap it client-side before the relay rejects it + +A whole-project bundle is much larger than a few items. Rough sizing: a `PlainItem` serialises to ~150-600 bytes; AES-256-GCM adds nonce+tag (~28 bytes) and base64 inflates by 4/3. A 200-item project lands around 100 KB plaintext → ~140 KB of base64 in one `text` column and one PostgREST request body. That is comfortably fine. A 5000-item project is ~3.5 MB, which is where Supabase's gateway request-size limit becomes a real risk. + +**The exact relay limit is not verifiable from this repo** — there is no `relay_packages` schema in `docs/`, and the setup SQL is user-provided. Therefore do not guess it: **cap on the client**. Refuse to send when the pre-encryption bundle JSON exceeds **1 MiB**, with an actionable error naming the size and suggesting fewer environments. A deterministic local error beats an opaque `relay upload failed (413)` from a third party. Revisit the constant if a real limit is ever documented. + +### D9 — No MCP tool for project relay + +The issue does not ask for one; the current MCP workspace tools are being deleted. Adding a project equivalent would let an LLM agent push an entire project's decrypted secrets to a third-party relay from a single tool call, with the confirmation affordance from D4 unavailable (an agent cannot meaningfully "confirm" on the user's behalf). CLAUDE.md's MCP rule — *the MCP server does not return secret values* — is about the return direction, but the spirit is that MCP is the lowest-trust surface. Sending is worse than returning. + +Decision: **MCP loses two tools and gains none.** If it is wanted later, it is a separate issue with its own consent design. + +### D10 — Keep the legacy `workspaces` tables, delete only the relay code + +The tables are still the source for the one-time backfill in `db/mod.rs` L209-212, which runs for any user upgrading from a pre-migration install. Dropping them would break that upgrade path. Deleting the *relay handlers* does not touch them. The `db` accessors `list_workspaces` / `get_workspace_vars` become unused by production code after Phase 0 — they remain referenced by the existing migration test at `db/mod.rs` L1325, so they stay, and no `#[allow(dead_code)]` is needed. Verify this with `cargo check` after Phase 0; if a warning does appear, keep the function and annotate rather than delete. + +### D11 — Breaking REST change: clean break, no aliases + +`POST /workspaces/:id/relay/send` and `POST /workspaces/relay/receive` are **deleted**, not aliased. The issue's "never reached users" claim was verified: `docs/reference.md` L45-46 documents them as legacy and explicitly warns their imports are invisible to the scoped endpoints; `CHANGELOG.md` L34 documents the MCP tools as "unchanged (out of scope, workspace-table-backed)". They are documented, so the change is breaking and belongs in the CHANGELOG — but they are documented *as broken*, and the only shipped consumer is the MCP server in the same repo, updated in the same PR. Aliasing would preserve a data black hole for the sake of a contract nobody can be depending on correctly. + +### D12 — Relay code entropy: adequate, but three notes (all out of scope) + +The `XXXX-XXXX` code is a **lookup handle, not the confidentiality boundary**. Confidentiality rests on `generate_passphrase` (12 chars over a 62-symbol alphabet ≈ 71 bits) run through Argon2id (m=32 MiB, t=2, p=2). Guessing a code yields ciphertext only, and an offline attack on 71 bits behind that Argon2id cost is not practical. **A whole-project payload does not change this analysis** — the same key protects it. + +Three observations, recorded so they are not rediscovered, and **explicitly out of scope for this issue**: + +1. **Modulo bias.** Both `generate_share_code` (relay.rs L121) and `generate_passphrase` (`share/crypto.rs` L95) do `ALPHA[(byte as usize) % ALPHA.len()]` over a `u8`. `256 % 36 = 4` and `256 % 62 = 8`, so the first few symbols are marginally over-represented. The entropy loss is a fraction of a bit — cosmetic, not exploitable, but it is a crypto-hygiene defect worth a one-line fix (`rand::seq::IndexedRandom` / rejection sampling) in a separate PR. +2. **Code enumeration.** 36^8 ≈ 2.8 × 10^12 handles, but `relay_download` is an unauthenticated-ish PostgREST `GET` with the anon key. Nothing in this repo rate-limits it. An attacker who enumerates codes harvests ciphertexts for offline passphrase attack. Higher-value payloads make harvesting more attractive even though it does not become more feasible. **Mitigation belongs in the relay's RLS policy / rate limit, not in this codebase** — but the relay setup SQL documentation should say so. +3. **Burn-after-read is best-effort.** `relay_download` filters `retrieved=eq.false` but nothing ever sets `retrieved = true`; the burn is the subsequent `relay_delete`, whose result is discarded (`let _ = ...` at api/mod.rs L2933 and `share_commands.rs`). If the delete fails, the payload stays downloadable until TTL. Also `relay_download` does not filter on `expires_at`, so expiry depends entirely on a server-side purge job. Larger payloads raise the cost of this failure. Record it; do not fix it here. + +--- + +## 4. Implementation steps + +Ordered. Each phase is a reviewable PR. + +### Phase 0 — Delete the workspace relay surface *(PR 1, independent, ships first)* + +`fix(api,mcp,share): remove the workspace relay protocol and its endpoints` + +1. `src-tauri/src/share/relay.rs` — delete L57-110: `WorkspaceBundleVar`, `WorkspaceBundle`, `impl WorkspaceBundle`, `encrypt_workspace`, `decrypt_workspace`, and the section comment. Keep everything else (`derive_relay_key`, `encrypt_items`, `decrypt_payload`, `generate_share_code`, the three transport fns, the ISO-8601 helpers). +2. `src-tauri/src/api/mod.rs` — delete `handle_workspace_relay_send`, `handle_workspace_relay_receive`, `WorkspaceRelaySendResponse`, `WorkspaceRelayReceiveResponse`, and router lines L3089-3090. Drop the now-unused `DbWorkspaceVar` import if `cargo check` flags it. +3. `src-tauri/src/bin/crypt-env-mcp.rs` — delete `tool_share_workspace_send` (L~2040-2100) and `tool_share_workspace_receive` (L~2101-2130), their dispatch arms, and their entries in the `tools/list` manifest. +4. `docs/reference.md` — delete rows L45-46, rows L251-252, and the L282 paragraph. +5. `CHANGELOG.md` — Unreleased → Removed: the two endpoints and the two MCP tools, marked **breaking**, with the reason (backed by frozen legacy tables; imports were invisible to every project-scoped surface). +6. `cargo check && cargo clippy` — confirm no orphaned imports and that `list_workspaces` / `get_workspace_vars` are still reachable from the migration test (D10). + +**Gate:** `grep -rn "WorkspaceBundle\|workspace_relay\|share_workspace" src-tauri/src src/ docs/` → zero hits. + +### Phase 1 — Protocol + core orchestration + tests *(PR 2)* + +`feat(share,project,db): project relay bundle format and receive orchestration` + +7. `src-tauri/src/share/relay.rs` — add the D1/D2 section: `ProjectBundleVar { key, item_name }`, `EnvironmentBundle { name, is_default, vars }`, `ProjectBundle { kind, version, name, description?, template, environments, items }`, `ProjectBundle::{KIND, VERSION}`, `encrypt_project`, `decrypt_project` (kind check **then** version check, mirroring the existing L104-108 guard, with a message that points at the items receive flow). +8. `src-tauri/src/db/mod.rs` — add `insert_received_project(...)` per D7: one `BEGIN`/`COMMIT`, Unicode-aware project-name pre-check first, then project → environments → items (ciphertext in, no key) → `item_projects` → `environment_vars`. Returns inserted ids and counts. **No crypto, no knowledge of `api` or `vault`.** +9. `src-tauri/src/project/mod.rs` (or a new `src-tauri/src/project/relay.rs` if `mod.rs` is getting long) — two orchestrators shared by the HTTP handler and the Tauri command: + - `build_project_bundle(db, vault_key, project_id, environment_ids) -> Result` — reads the project via `list_projects`, filters to the selected environments, decrypts each referenced item once, dedups by name (`HashMap` entry-API, same shape as api/mod.rs L2711), skips dangling `item_id`s, enforces the D8 size cap. + - `receive_project_bundle(db, vault_key, bundle, name_override) -> Result` — default-environment promotion, encrypt each item with the receiver's key, single call into `insert_received_project`. +10. `src-tauri/src/share/mod.rs` — extract the per-item encrypt-and-upsert body out of `import_plain_items_into_vault` (L656+) into a private helper both paths share (D7). Do not change `import_plain_items_into_vault`'s public behaviour — #11's tests cover it. +11. Tests: `share/relay.rs` `mod tests` (cases 1-4) + `src-tauri/tests/project_relay.rs` (cases 5-11), on the #11 harness. + +**Gate:** `cargo test` green, cases 5 and 6 passing. + +### Phase 2 — HTTP + Tauri + GUI *(PR 3)* + +`feat(api,project,ui): share a whole project via the encrypted relay` + +12. `src-tauri/src/api/mod.rs` — `handle_project_relay_send` / `handle_project_relay_receive`, following the existing relay handlers' structure (settings lookup → `spawn_blocking` for the blocking reqwest calls → typed error json). Routes `POST /projects/:id/relay/send`, `POST /projects/relay/receive`. Collision → `409 CONFLICT`. Oversize → `413` or `422` with the D8 message. **No secret values in any response** — send returns `{code, passphrase, project, environment_count, item_count}`; receive returns `{project, environments: [names], item_count}`. +13. `src-tauri/src/project/relay_commands.rs` (new) — `project_relay_send`, `project_relay_receive`, modelled on `vault/share_commands.rs` L212/L283 including the `guard.touch()` calls and the `DEFAULT_RELAY_URL`/`DEFAULT_RELAY_ANON_KEY` fallbacks. **Note the existing inconsistency:** the Tauri commands fall back to bundled defaults while the HTTP handlers hard-fail with `NOT_CONFIGURED`. Match the Tauri side for the new Tauri commands and the HTTP side for the new handlers — do not "fix" the divergence here; that is its own issue. +14. `src-tauri/src/lib.rs` — register both in `invoke_handler` next to the existing project commands (~L211-213) and add the `use` entries (~L31-32). +15. `src/components/ProjectShareModal.tsx` (new) — environment checklist (non-default unchecked, prod-named highlighted), the D4 key manifest built from `useProjectStore` data, confirm step, then the code/passphrase display **extracted from `ShareModal.tsx`**. Receive side: code + passphrase inputs, and a rename field revealed on `CONFLICT`. `invoke()` only; Tailwind only. +16. `src/components/ShareModal.tsx` — extract the code/passphrase display and the relay security note (L980-1060 region) into a shared component so both modals use one implementation. Behaviour unchanged. +17. `src/components/ProjectManager.tsx` — "SHARE PROJECT" on the project detail view (next to the existing export button, L676), "RECEIVE PROJECT" on the projects list (next to import, L685). Refresh `projectStore` + vault items after a successful receive. + +**Gate:** manual round trip between two vaults on Windows per `CLAUDE.local.md`; confirm the receiver's items are project-owned and absent from Global Secrets. + +### Phase 3 — CLI + docs *(PR 4)* + +`feat(cli,docs): crypt-env project share / receive` + +18. `src-tauri/src/bin/crypt-env/commands/project.rs` — add `Share { id/name, envs: Option, yes: bool }` and `Receive { code, passphrase, as_name: Option }` to `ProjectCmd` (L16) and the `run` match. Follow `commands/relay.rs` for the `authenticated_post` calls and reuse its code/passphrase box output verbatim (L78-86). `--envs` omitted means **default environment only**, matching D4's GUI default; `--envs all` is the explicit opt-in. Print the D4 manifest and require `y/N` unless `--yes`. +19. `docs/reference.md` — new REST rows for the two `/projects/...` routes with request/response examples; CLI section updated. +20. `CHANGELOG.md` — Added entry for the feature. + +**Gate:** `crypt-env project share` → `crypt-env project receive` round trip against a second vault. + +--- + +## 5. Trade-offs and alternatives considered + +### 5.1 The big one: build vs. delete-only + +| Option | Cost | What you get | Verdict | +|---|---|---|---| +| **Delete only (Phase 0)** | ~1 day | Issue's stated complaint closed; broken agent-reachable path removed; ~250 lines gone | **Ships regardless.** Legitimate stopping point. | +| **Build it (Phases 0-3, no MCP)** | ~4-5 days, 6 files of new backend + 3 of frontend | One-step project onboarding, per-environment key manifest, safe defaults | **Recommended.** | +| Build it including MCP | +1 day | An agent can exfiltrate a whole project in one call | **Rejected (D9).** | +| Do nothing | 0 | Broken endpoints stay live | Rejected. | + +The honest risk with "build it": this is a **six-surface feature for a workflow that already has a two-step workaround**. If the user base is one person (it currently is), the payback is thin. That is why Phase 0 is separable and why MCP is dropped — the plan is structured so that stopping after any phase leaves the repo coherent. + +### 5.2 Format alternatives + +- **Extend `ExportedProject` with optional values** — one format instead of two. Rejected (D1): risks writing values into a `.cryptenv-proj` file, whose entire contract is that it never carries them. +- **Keep `kind: "workspace"` and just add fields** — no new discriminator, smaller diff. Rejected: it is precisely the format-confusion the issue asks to avoid, and the old receiver would parse the new payload's shared fields and silently drop the environments. +- **Ship items nested per environment** — simplest builder. Rejected (D1): duplicated ciphertext, dedup information destroyed. + +### 5.3 Receive-semantics alternatives + +- **Merge into the existing project by name** — the most "convenient" option, and the most dangerous: a sender chooses the project name, so a sender chooses which of the receiver's projects to mutate. Rejected on security grounds (D5). +- **Auto-rename on collision** — non-destructive but confusing; hides the collision. Rejected in favour of an explicit error plus a caller-supplied override. +- **Receive items as global** — would make them immediately reusable. Rejected (D7): scope is the receiver's decision, and global items surface in Global Secrets where their provenance is invisible. + +### 5.4 Transport alternatives + +- **Chunk large bundles across multiple relay rows** — removes the size ceiling. Rejected: multi-row burn-after-read, partial-download and partial-burn semantics are a meaningful protocol expansion for a limit that a 1 MiB cap makes unreachable in practice (D8). +- **Compress before encrypting** — would raise the effective ceiling several-fold and is cheap. Rejected *for now*: compression before encryption leaks plaintext-size structure via ciphertext length, and the cap makes it unnecessary. Revisit only if real users hit the cap. + +### 5.5 What breaks first, and what would make this decision wrong + +- **Breaks first at scale:** the D8 size cap, on a project with thousands of variables. Warning sign: users reporting the cap error. Fix: compression (5.4) or chunking. +- **Breaks first in correctness:** name-based item matching (D1). Two distinct items sharing a name collapse into one. Warning sign: a receiver reporting a missing variable that "was in the send". Mitigation: the D4 manifest shows the deduped list, so the sender can see the collapse before uploading. +- **Would invalidate this plan:** if the `workspaces` tables turn out to still be written by some path this investigation missed, D10 and Phase 0 both need revisiting — `grep -rn "upsert_workspace\|set_workspace_vars"` was run and returned only `api/mod.rs`, but re-run it at implementation time. +- **Would invalidate D9:** an explicit product decision that MCP agents are trusted to initiate outbound secret transfer. That is a product decision, not an architectural one. +- **Reversibility:** Phase 0 is a pure deletion, recoverable from git. Phases 1-3 are additive — the new endpoints, commands and UI can be removed without touching the data model, since receiving only ever *creates* normal `projects`/`environments`/`items` rows that the rest of the app already owns. There is **no schema migration in this plan**, which is what makes it cheap to unwind. + +--- + +## 6. Sequencing dependencies + +| Depends on | Why | If it lands later | +|---|---|---| +| #11 (test harness) | Cases 5-11 use `test_support::{unlocked_vault, seed_project, seed_item, link_var}` | Phase 1 blocks on it, or writes a throwaway local fixture and refactors onto the harness afterwards — prefer blocking | +| #12 (`nocase` uniqueness) | D5's 409 mapping and the `is_unique_violation` helper | Implement the application-level pre-check regardless; adopt #12's helper as the backstop when it lands | + +Phase 0 depends on nothing and should not wait for either. diff --git a/docs/plans/issue-5-linux-window-controls.md b/docs/plans/issue-5-linux-window-controls.md new file mode 100644 index 0000000..943ea90 --- /dev/null +++ b/docs/plans/issue-5-linux-window-controls.md @@ -0,0 +1,253 @@ +# Issue #5 — Linux: window has no close/minimize buttons + +Status: plan only. No code written yet. +Scope owner: frontend chrome (`src/components/WindowChrome.tsx`) + Linux verification path. +Related: issue #6 (oversized cursor on Linux) — same platform, same verification session, **not fixed here**. + +--- + +## 1. Objective + +**Definition of done (measurable):** + +1. On a Linux build produced from this branch (`.deb` installed, or `pnpm tauri build` output run directly), the window can be **minimized** and **closed** from controls rendered inside the app's titlebar. Both are reachable by mouse and by keyboard (`Tab` focus + `Enter`/`Space`). +2. The window remains **draggable** by the titlebar spacer (`data-tauri-drag-region`) on Linux — explicitly checked, because this is the one behaviour that `decorations: false` can silently break under Wayland. +3. Verified on **two** desktop configurations: + - GNOME on Wayland (Ubuntu 24.04 default session), and + - a non-GNOME / X11 session (KDE Plasma X11 or Xfce). +4. **No regression on Windows**, verified by the maintainer's normal loop (`pnpm tauri dev` from PowerShell against the WSL path, per `CLAUDE.local.md`): minimize works, close works, LOCK button still present and correctly hidden on the lock screen, wordmark still centred, drag still works. +5. **No change of behaviour on macOS**: macOS renders exactly what it renders today (no controls). This is a deliberate no-op — see §4.3. The macOS `dmg` job in `.github/workflows/release.yml` still builds green. +6. `pnpm build` (tsc + vite) and `cargo check` pass; no new dependency added to `package.json` or `src-tauri/Cargo.toml`. + +**Explicitly NOT in the definition of done:** maximize/restore. See §4.4 — the issue's "Expected" section names it, and this plan defers it on purpose rather than silently dropping it. + +--- + +## 2. What is being mitigated + +Checkable statement of the current defect: + +- `src-tauri/tauri.conf.json` sets `app.windows[0].decorations: false`. The OS therefore draws **no** titlebar, on any platform. +- `src/components/WindowChrome.tsx` renders the replacement minimize/close buttons behind `platform() === 'windows'` (line 46, `{isWindows && (...)}`). +- Consequence: on Linux there is **no way to close or minimize the window from within the application at all**. The user's only exits are the window manager's own affordances (`Alt+F4`, `Super+H`, right-click on a taskbar entry, `pkill`) — none of which are discoverable, and several of which are absent on minimal WMs. +- `bundle.targets` is `["nsis", "dmg", "deb"]` and `.github/workflows/release.yml` has a working `build-linux` job on `ubuntu-latest` that uploads a `.deb`. Linux is a **shipped target**, not hypothetical: one of three shipped bundles ships a window the user cannot close. +- The same gate also excludes macOS (`dmg`, also shipped). That is a real hole, tracked separately — see §4.3. + +Verification that the defect is real: run the Linux build, confirm the titlebar shows only the `CRYPTENV` wordmark and (post-unlock) the `LOCK` button, with empty space where the Windows build has two buttons. + +--- + +## 3. Decision + +**Option (A): extend the existing custom chrome to Linux. Keep `decorations: false`. Right-aligned, Windows-order controls. No desktop-preference probing.** + +Reasoning: + +- `CLAUDE.md` states the window is decorationless with a custom titlebar. Option (A) is the only option that keeps that invariant; option (B) contradicts a documented project decision for one platform. +- It is the smallest correct diff: one gate condition, in one file, with buttons that already exist and already work. +- It keeps a single visual identity for an industrial/utilitarian app whose whole chrome is deliberately non-native. +- The cost of (A) is convention divergence (GNOME users expect close at the far right — which we already satisfy — and no minimize button at all in default GNOME, which we are *adding*, not removing). This is an acceptable, reversible cosmetic divergence. It is not a functional defect. + +**Rejected: reading the desktop's button layout.** GNOME's `org.gnome.desktop.wm.preferences button-layout` is the only authoritative source for button order/side, it is not readable from the webview, and reading it would require a new Rust command (`window_button_layout` or similar), a gsettings/dconf dependency at runtime, a fallback path for every non-GNOME desktop that does not publish an equivalent key, and per-desktop layout code in `WindowChrome.tsx`. That is a large amount of machinery to move two buttons a few pixels in an application that already refuses to look native anywhere. **Over-engineering — do not build it.** If a user complains about order after shipping, revisit with real feedback. + +--- + +## 4. Implementation steps + +Ordered. Each step is independently checkable. + +### 4.1 — Confirm permissions (no change expected) + +File: `/home/maosuarez/Programas/crypt-env/src-tauri/capabilities/default.json` + +Already granted: + +``` +core:window:allow-minimize +core:window:allow-close +core:window:allow-start-dragging +core:window:allow-is-maximized +``` + +Capabilities in this project are **not** platform-scoped (no `"platforms"` key), so the Windows-working permissions apply verbatim on Linux. **No permission additions are required for this plan.** + +Only if maximize were adopted (it is not — §4.4) would `core:window:allow-maximize`, `core:window:allow-unmaximize` and/or `core:window:allow-toggle-maximize` need to be added here. Note `core:window:allow-is-maximized` is already present but is a getter only and grants nothing on its own. + +### 4.2 — Rewrite the platform gate in `WindowChrome.tsx` + +File: `/home/maosuarez/Programas/crypt-env/src/components/WindowChrome.tsx` + +Replace the boolean `isWindows` state + `useEffect` with a **platform-derived config object**, computed once. Rationale for an object rather than a second boolean: the question this component actually asks is "what chrome does this platform want?", and adding platforms as booleans (`isWindows`, `isLinux`, `isMac`) produces exactly the tangle that caused this bug — a check that silently means "not-Windows = nothing". An object makes the per-platform answer explicit and makes a future macOS entry (§4.3) a data change, not a logic change. + +Shape (illustrative, adjust naming to taste): + +```ts +type Chrome = { showControls: boolean }; + +function chromeFor(os: string): Chrome { + switch (os) { + case 'windows': + case 'linux': + return { showControls: true }; + default: // macos and anything else: unchanged behaviour + return { showControls: false }; + } +} +``` + +Consume it with a **lazy `useState` initializer**, not a `useEffect`: + +```ts +const [chrome] = useState(() => chromeFor(platform())); +``` + +Notes: +- `platform()` from `@tauri-apps/plugin-os` is **synchronous** in Tauri v2, so the current `useEffect` + `setState` is unnecessary indirection and causes a one-frame render with no controls. Removing it is in scope because we are editing those exact lines; do not extend the cleanup beyond this component. +- Prefer the lazy initializer over a module-scope `const` so the call happens at first render inside the Tauri webview rather than at import time. (`getCurrentWindow()` is already at module scope, so module scope would also work — the initializer is simply the safer of two acceptable choices.) +- Change the render gate from `{isWindows && (...)}` to `{chrome.showControls && (...)}`. **Nothing inside the button block changes**: same markup, same order (minimize then close), same right alignment, same Tailwind classes, same inline SVGs. + +### 4.3 — Do NOT include macOS in this change + +The `platform() === 'windows'` gate leaves macOS with no controls either, and `dmg` is a shipped target. This plan **deliberately does not fix macOS**, for two reasons: + +1. `CLAUDE.md`: "Keep scope strictly to what is requested." Issue #5 is titled and scoped to Linux. +2. macOS is not a free ride-along. Its convention is traffic lights on the **left**, and the idiomatic Tauri approach there is not a hand-drawn right-aligned control cluster but `titleBarStyle: "Overlay"` with `hiddenTitle`, which keeps native traffic lights while allowing custom content — a different mechanism, a config change, and a layout change (left padding for the lights, wordmark re-centring). That is its own decision with its own trade-offs and its own verification hardware. + +**Action:** open a sibling issue ("macOS: window has no close/minimize buttons") referencing this plan, so the hole is tracked rather than forgotten. Do not implement it in this PR. + +### 4.4 — Maximize / restore: explicitly deferred, not dropped + +The issue's Expected section says "close/minimize (and maximize)". Position: + +- `tauri.conf.json` sets `"resizable": false`. With that, maximize is a no-op or a WM-refused request on most platforms — a button that visibly does nothing is worse than no button. +- Making it meaningful requires `"resizable": true`, which turns a fixed 560×700 industrial layout into a responsive one. Every screen (`ProjectManager`, `GlobalSecrets`, item forms, footer nav) would need to be re-checked at large sizes. That is a UI-layout project, not a window-controls fix. +- Maximize is also absent on Windows today, so adding it on Linux would create the platform inconsistency the issue is complaining about, in the opposite direction. + +**Decision: maximize is out of scope for issue #5.** Record this as a comment on the issue when the PR opens ("minimize + close shipped; maximize deferred, requires `resizable: true` and a responsive-layout pass — filed as #NN"), so the requirement is visibly deferred rather than quietly unmet. If maximize is later adopted, it needs: `resizable: true`, `core:window:allow-toggle-maximize` in `capabilities/default.json`, a third button, and a maximized/restored icon state. + +### 4.5 — Accessibility + +Keep the existing pattern exactly: each button is a real ` + + + + + ); +} + // ─── VarRow (project detail: real vault item, type-aware) ───────────────────── function varSummary(item: VaultItem, reveal: boolean): string { @@ -422,7 +475,7 @@ function EnvironmentCard({ }: { env: Environment; onOpen: () => void; - onInject: (id: number) => Promise<{ paths: string[]; written: string[] }>; + onInject: (id: number) => Promise; }) { const [injectState, setInjectState] = useState<'idle' | 'ok' | 'err'>('idle'); @@ -438,7 +491,12 @@ function EnvironmentCard({ await onInject(env.id); setInjectState('ok'); setTimeout(() => setInjectState('idle'), 2000); - } catch { + } catch (err) { + // A cancelled confirm-overwrite dialog isn't a failure — just reset. + if (String(err) === 'Error: cancelled') { + setInjectState('idle'); + return; + } setInjectState('err'); setTimeout(() => setInjectState('idle'), 2000); } @@ -510,9 +568,49 @@ export function ProjectManager() { const cats = useVaultStore((s) => s.cats); const getItemOwners = useVaultStore((s) => s.getItemOwners); - const { projects, loading, load, saveProject, removeProject, saveEnvironment, removeEnvironment, inject } = + const { projects, loading, load, saveProject, removeProject, saveEnvironment, removeEnvironment, inject, previewInject } = useProjectStore(); + // Pending confirm-then-inject flow (see `runInject` below): `injectConfirm` + // holds the modal's display data, while the resolve/reject pair for the + // promise `runInject` returned to its caller lives in a ref so it survives + // re-renders without becoming React state itself. + const [injectConfirm, setInjectConfirm] = useState<{ id: number; foreign: string[] } | null>(null); + const pendingInjectRef = useRef<{ resolve: (r: InjectResult) => void; reject: (e: unknown) => void } | null>(null); + + // Single entry point for both the project-list quick-inject button and the + // environment editor's INJECT button: previews first, and only prompts for + // confirmation when the preview reports a path crypt-env doesn't manage. + const runInject = async (id: number): Promise => { + const preview = await previewInject(id); + if (preview.foreign.length === 0) return inject(id, false); + return new Promise((resolve, reject) => { + pendingInjectRef.current = { resolve, reject }; + setInjectConfirm({ id, foreign: preview.foreign }); + }); + }; + + const confirmPendingInject = async () => { + if (!injectConfirm) return; + const { id } = injectConfirm; + const pending = pendingInjectRef.current; + pendingInjectRef.current = null; + setInjectConfirm(null); + try { + const result = await inject(id, true); + pending?.resolve(result); + } catch (e) { + pending?.reject(e); + } + }; + + const cancelPendingInject = () => { + const pending = pendingInjectRef.current; + pendingInjectRef.current = null; + setInjectConfirm(null); + pending?.reject(new Error('cancelled')); + }; + const [mode, setMode] = useState('projects'); const [selectedProject, setSelectedProject] = useState(null); const [selectedEnv, setSelectedEnv] = useState(null); @@ -544,6 +642,24 @@ export function ProjectManager() { const [saving, setSaving] = useState(false); const [injecting, setInjecting] = useState(false); + // WSL bridge (issue #3) — ephemeral machine state, not vault state, so a + // plain local state is enough (no Zustand store, no TanStack Query). + const [isWindows, setIsWindows] = useState(false); + const [wslDistros, setWslDistros] = useState([]); + const [wslBusy, setWslBusy] = useState(false); + const [wslPickerOpen, setWslPickerOpen] = useState(false); + + useEffect(() => { + setIsWindows(platform() === 'windows'); + }, []); + + useEffect(() => { + if (!isWindows) return; + invoke('wsl_list_distros') + .then(setWslDistros) + .catch(() => setWslDistros([])); + }, [isWindows]); + useEffect(() => { load(); }, []); // Keep the selected project/environment in sync once the store reloads @@ -727,22 +843,41 @@ export function ProjectManager() { setEnvPaths((prev) => prev.filter((_, i) => i !== idx)); }; + // Shared tail of "a path was picked" handling, reused by both the plain + // browse button and the WSL-seeded one (plan §3.4). + const applyPickedEnvPath = (picked: string | null) => { + if (!picked) return; + const trimmed = picked.trim(); + if (!envPaths.includes(trimmed)) setEnvPaths((prev) => [...prev, trimmed]); + if (!projName && isCreatingProj) { + const folder = folderNameFromPath(trimmed); + if (folder) setProjName(folder); + } + }; + const handlePickEnvPath = async () => { try { const picked = await invoke('project_pick_env_path'); - if (picked) { - const trimmed = picked.trim(); - if (!envPaths.includes(trimmed)) setEnvPaths((prev) => [...prev, trimmed]); - if (!projName && isCreatingProj) { - const folder = folderNameFromPath(trimmed); - if (folder) setProjName(folder); - } - } + applyPickedEnvPath(picked); } catch (e) { showToast(String(e), 'error'); } }; + const handleBrowseWsl = async (distro: string) => { + setWslPickerOpen(false); + setWslBusy(true); + try { + const home = await invoke('wsl_distro_home', { distro }); + const picked = await invoke('project_pick_env_path', { startDir: home }); + applyPickedEnvPath(picked); + } catch (e) { + showToast(String(e), 'error'); + } finally { + setWslBusy(false); + } + }; + const handleUnlinkVar = (idx: number) => { setEnvVars((prev) => prev.filter((_, i) => i !== idx)); }; @@ -809,11 +944,15 @@ export function ProjectManager() { if (envPaths.length === 0) { showToast('Add at least one path before injecting', 'error'); return; } setInjecting(true); try { - const result = await inject(selectedEnv.id); + const result = await runInject(selectedEnv.id); const pathLabel = result.paths.length === 1 ? result.paths[0] : `${result.paths.length} paths`; - showToast(`Injected ${result.written.length} variable${result.written.length !== 1 ? 's' : ''} → ${pathLabel}`); + let msg = `Injected ${result.written.length} variable${result.written.length !== 1 ? 's' : ''} → ${pathLabel}`; + if (result.unmanagedPaths.length > 0) { + msg += ` (warning: ${result.unmanagedPaths.length} unmanaged path${result.unmanagedPaths.length !== 1 ? 's' : ''} written, backup kept)`; + } + showToast(msg); } catch (e) { - showToast(String(e), 'error'); + if (String(e) !== 'Error: cancelled') showToast(String(e), 'error'); } finally { setInjecting(false); } @@ -1029,7 +1168,7 @@ export function ProjectManager() { )} {selectedProject.environments.map((env) => ( - openEnvironment(env)} onInject={inject} /> + openEnvironment(env)} onInject={runInject} /> ))} @@ -1166,6 +1305,39 @@ export function ProjectManager() { > + {isWindows && wslDistros.length > 0 && ( +

+ + {wslPickerOpen && wslDistros.length > 1 && ( +
+ {wslDistros.map((distro) => ( + + ))} +
+ )} +
+ )}