From 0c1c5393b6c476bd8bf910f5ebc3133ab550b912 Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 13:07:58 +0400 Subject: [PATCH 01/17] docs: spec per-workspace slash command discovery Project skills never reach the composer popup: command discovery asks the agent for a session in $HOME, so the agent never sees /.claude/skills. Measured against claude-agent-acp 0.66.0: 110 commands from a project cwd vs 76 from $HOME, the 34 missing entries being that project's skills. Specs the fix: commands become identified by (harness, cwd), an engine-side cache fed by both a cwd-aware probe and the live AvailableCommands event that sessions.rs currently drops, and a ListCommands RPC that carries a workspace. --- docs/slash-commands.md | 238 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 238 insertions(+) create mode 100644 docs/slash-commands.md diff --git a/docs/slash-commands.md b/docs/slash-commands.md new file mode 100644 index 000000000..2e3f2a358 --- /dev/null +++ b/docs/slash-commands.md @@ -0,0 +1,238 @@ +# Slash commands: per-workspace discovery + +Status: PLANNED · 2026-08-16 investigation (project skills missing from the composer popup). + +## Why + +Project skills never appear in the slash-command popup. A repo with skills in +`/.claude/skills` shows only the built-in and user-level commands. + +The cause is one line. Command discovery asks the agent for a session in the wrong +directory (`crates/harness/src/acp/mod.rs:782-784`): + +```rust +let cwd = std::env::var("HOME").unwrap_or_else(|_| "/".into()); +let session = client + .request("session/new", json!({ "cwd": cwd, "mcpServers": [] })) +``` + +ACP agents build the command list from the session `cwd`. Project skills live under the +project. With `cwd = $HOME` the agent never sees them. + +### Measured + +The `claude-agent-acp` 0.66.0 adapter was driven twice by hand. Only the `session/new` +cwd changed: + +| session cwd | commands | +|---|---| +| `~/Documents/AppDev/read-aloong` | 110 | +| `$HOME` | 76 | + +The 34 missing entries are exactly that project's installed skills: `ask-matt`, `tdd`, +`wayfinder`, `wizard`, `triage`, `prototype`, `research`, `grill-with-docs`, and the rest. + +The skills were installed as symlinks into a content-addressed store. The agent followed +every symlink once the cwd was right. Symlinked skill directories are not a factor. + +### The real shape of the bug + +ACP advertises commands **per session**, and a session is defined by its `cwd`. Zeron +models commands **per harness**, with no workspace anywhere in the path. Three layers +carry that mismatch, and fixing one alone changes nothing: + +1. **Discovery cwd.** `discover_commands` spawns with `spawn_agent(None, ...)` and sends + `cwd = $HOME` (`acp/mod.rs:767-784`). +2. **RPC shape.** `ListCommands` borrows `ListModelsParams`, which carries only `harness` + (`engine/src/rpc.rs:85-87`, `1023-1036`). There is no field for a workspace. +3. **Two caches keyed by harness only.** The `OnceCell` in the harness (`acp/mod.rs:548`) + and the composer's `slash_cache` (`ui/src/composer.rs:3312`, `4018`). With the cwd + threaded through but the keys unchanged, the first project's list would serve every + project. + +A fourth fact shapes the design. A live session already produces the correct list. +`acp/mod.rs:2218-2222` emits `AgentEvent::AvailableCommands` from a session started in the +real cwd. `doc/src/parts.rs:339-343` drops it, and no engine code reads it. The comment +there claims the event "feeds the engine's per-harness command cache". No such cache +exists. The comment is stale. + +## Design + +The identity of a command list becomes `(harness, cwd)`. + +The `cwd` is a path on the **host device** that owns the agent, so a space on another +device uses that device's path. `~` travels unexpanded and expands on the host, matching +how run cwd already works (`composer.rs:4536-4539`). + +### Topology + +``` + probe (cold, TTL-bounded) +composer popup ── ListCommands{harness, cwd} ── engine CommandCache ── Harness::commands(cwd) + ^ ^ + └── stale-while-revalidate render └── AgentEvent::AvailableCommands + (live, from a running session) +``` + +Two sources feed one cache. The probe serves cold projects and chats that never started. +The live event corrects any chat that is running. + +### The harness probe + +`Harness::commands` stops caching and becomes a plain probe: + +```rust +async fn commands(&self, cwd: Option<&str>) -> Result, HarnessError> +``` + +- The `OnceCell` at `acp/mod.rs:548` is deleted. +- `discover_commands` passes the cwd to both `spawn_agent` and `session/new`. +- `cwd: None` means `$HOME`. That is today's behavior, kept for callers with no workspace. +- The trait default still returns an empty list for non-ACP harnesses. + +`discover_models` keeps its own `OnceCell` and its `$HOME` cwd. Models are not treated as +workspace-scoped in this spec. See Non-goals. + +### The engine cache + +New file: `crates/engine/src/commands.rs`. + +The cache lives in the engine, not the harness, because the two sources arrive in two +different places. The probe result returns inside the harness. The live event arrives in +the engine run loop at `sessions.rs:1572-1596`, where `run_cwd` is already in scope. One +cache in the engine is fed by both, and the harness stays a thin protocol client. + +| Policy | Value | +|---|---| +| Key | `(HarnessId, String)`, path normalized | +| Fresh TTL | 10 minutes | +| Negative TTL | 30 seconds | +| Bound | LRU, 16 entries | +| Concurrency | single-flight per key; waiters subscribe to the in-flight probe | +| Live write | `AvailableCommands` overwrites `(harness, run_cwd)` and resets its TTL | + +Key normalization reuses `expand_home` (`sessions.rs:1032-1042`) and trims trailing +separators. The engine runs on the host device, so it expands `~` with the right home. +This matters for the live write: `run_cwd` is `request.cwd`, which can still hold `~` +(`sessions.rs:1072`). Both writers must produce the same key, or a running chat would fill +one entry while the popup reads another. + +The live write keys by the **request** cwd, not by the cwd on the `SessionStarted` event. +The rule next door is the opposite: `sessions.rs:1577` scopes a stored session id by the +event's own cwd, because that is where the harness really created the session. The command +cache needs the other one. The popup looks up by `chat.cwd`, which is the request cwd, so +keying by the event would fill an entry that nothing ever reads. + +Entry states are explicit, which makes single-flight testable: + +- `Fresh { commands, at }` +- `Failed { error, at }` +- `InFlight { subscribers }` + +A read that finds `InFlight` waits on it. A read that finds a stale entry treats it as a +miss and probes. The engine never answers with a stale list, because it has no way to push +a correction afterwards. Freshness on the wire keeps the UI's own stale-while-revalidate +render honest. + +### The RPC + +`ListCommands` gets its own params struct instead of borrowing `ListModelsParams`: + +```rust +struct ListCommandsParams { + harness: HarnessId, + cwd: Option, + target_device_id: Option, +} +``` + +`cwd` is optional. An engine on an older device ignores the unknown field and answers with +its `$HOME` list, so version skew degrades to today's behavior instead of failing. + +### The composer + +`slash_cache` is keyed by `(HarnessId, String)`. The cwd resolves when the popup opens: + +- selected chat: `chat.cwd` +- new chat: the space path, or `~` when project-less +- the checkout plan is ignored + +A worktree is a checkout of the same repo, so the space path is the right answer for a +`NewWorktree` plan that has no directory yet, and close enough for `ReuseWorktree`. A +worktree that lacks untracked skills self-corrects through the live event once the session +runs. + +Rendering is **stale while revalidate**. A cached entry renders at once with no spinner, +and a background `ListCommands` refreshes it. Changing the harness or the project picker +changes the key, so the list follows the project. + +The composer cache has no TTL of its own. Every popup open sends one `ListCommands`, and +the engine decides whether that costs a probe. One expiry policy, in one place. + +The stale comment at `parts.rs:339-343` is corrected, because the event now does feed a +cache. + +## Error handling + +| Case | Behavior | +|---|---| +| Probe fails (adapter missing, timeout, auth) | Existing `slash_error_message` path. Cache `Failed` for 30 seconds. | +| `session/new` rejects the cwd (deleted worktree, bad path) | Retry once with `$HOME`, then cache and show that list. The user keeps built-in commands. | +| Remote device runs an older engine | It ignores `cwd` and returns the `$HOME` list. No error. | +| No harness resolved yet | Unchanged. Empty popup, no fetch. | +| First open, nothing cached | Unchanged loading state. | + +## Probe cost + +A probe is not free. Measured on `claude-agent-acp` 0.66.0: + +- it starts a full `claude` process, +- it runs the project's SessionStart hooks, +- it leaves a bare session directory under `~/.claude/projects//`. + +The current `$HOME` probes have left 666 bare directories and 7.9 MB in +`~/.claude/projects/-Users-/`. After this change that litter moves into real project +directories. It stays invisible to `/resume`, because there is no transcript file, but it +is untidy. + +Four mitigations are in the design: the 10 minute TTL, single-flight, negative caching, and +probing only on popup open. A running chat never probes at all, because its own event feeds +the cache. + +The clean fix belongs upstream: a capabilities handshake that lists commands without +`session/new`. Record it as an adapter ask. Do not block this work on it. + +## Testing + +**Unit, `engine/src/commands.rs`** +- key normalization: `~`, trailing separator, two spellings of one path +- a stale entry is treated as a miss, not served +- TTL expiry and negative TTL +- LRU bound at 16 entries +- single-flight: two concurrent reads of one cold key produce one probe + +**Harness, extending `crates/harness/tests/acp.rs:580`** +- the mock agent asserts `session/new` receives the requested cwd +- a rejected cwd triggers exactly one `$HOME` retry + +**Engine** +- an `AvailableCommands` event during a run writes `(harness, run_cwd)` +- a later `ListCommands` for that cwd returns it with no probe + +**UI** +- cwd resolution for the three cases: chat row, space row, project-less +- switching the project picker changes the cache key + +**Manual E2E** +- open a project with installed project skills, type `/`, confirm they appear +- open a project without them, confirm they do not +- the two-cwd probe above is the reproduction, and it gives the exact expected diff + +## Non-goals + +- **Models.** They keep the `$HOME` probe. `ListCommandsParams` and the cache key are + shaped so models can join later without another interface change. +- **File watchers on `.claude`.** The TTL plus the live event covers the real workflow. +- **Cleaning the 666 stale directories.** Separate chore. +- **Changing how an agent resolves skills.** Symlinked skills work correctly once the cwd + is right. From 75759d97de161cddd0345dfa13a556d5928b5bb3 Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 13:20:49 +0400 Subject: [PATCH 02/17] docs: correct the slash-command spec after review Review against the code found the live-event leg does not exist as described. The AvailableCommands emission at acp/mod.rs:2218 reads the initialize response, which claude-agent-acp leaves empty; the real cwd-scoped list arrives as an available_commands_update notification that request_draining discards. The spec now specs the harness capture that makes the live leg real. Also corrected: run_cwd is already expanded at sessions.rs:298 so only the RPC side needs it; the probe must not set the child process directory (a missing dir would misreport as NotInstalled); the composer cache key needs the target device; ListCommandsParams needs camelCase and no targetDeviceId; chat.cwd is optional; InFlight/LRU/live-write races and lock discipline are now defined; the ui crate has no gpui tests, so cwd resolution is factored into a pure function instead. --- docs/slash-commands.md | 137 ++++++++++++++++++++++++++++++++--------- 1 file changed, 108 insertions(+), 29 deletions(-) diff --git a/docs/slash-commands.md b/docs/slash-commands.md index 2e3f2a358..3c682d9e9 100644 --- a/docs/slash-commands.md +++ b/docs/slash-commands.md @@ -50,11 +50,28 @@ carry that mismatch, and fixing one alone changes nothing: threaded through but the keys unchanged, the first project's list would serve every project. -A fourth fact shapes the design. A live session already produces the correct list. -`acp/mod.rs:2218-2222` emits `AgentEvent::AvailableCommands` from a session started in the -real cwd. `doc/src/parts.rs:339-343` drops it, and no engine code reads it. The comment -there claims the event "feeds the engine's per-harness command cache". No such cache -exists. The comment is stale. +A fourth fact shapes the design, and it is worse than it first looks. A live session runs in +the real cwd, so it holds the correct list. Zeron never sees it. Two separate reasons: + +1. **The existing emission never fires for Claude.** `acp/mod.rs:2218-2222` emits + `AgentEvent::AvailableCommands`, but only from `init_commands`, which + `scan_available_commands` reads out of the **initialize** response + (`acp/mod.rs:2004`). Initialize runs before `session/new`, so that list is not + cwd-scoped, and for `claude-agent-acp` it is empty. Measured: driving the adapter by + hand, initialize and the `session/new` response both carry no commands. All 110 arrive + in one `available_commands_update` notification after `session/new`. +2. **That notification is dropped.** Every run sends `session/new` through + `request_draining` (`acp/mod.rs:2036`, and `2010`/`2018` for the resume paths). That + helper answers server requests and discards notifications (`Some(_) => {}`, + `acp/mod.rs:1895`); its post-response flush handles only `Incoming::Request` + (`acp/mod.rs:1907-1911`). The update lands inside exactly that window. + +Only a mid-session update, sent after the handshake, reaches the main loop and +`normalize.rs:365-368`. + +Then `doc/src/parts.rs:339-343` drops whatever does get through, and no engine code reads +it. The comment there claims the event "feeds the engine's per-harness command cache". No +such cache exists. The comment is stale. ## Design @@ -77,6 +94,22 @@ composer popup ── ListCommands{harness, cwd} ── engine CommandCache ─ Two sources feed one cache. The probe serves cold projects and chats that never started. The live event corrects any chat that is running. +### Making the live event real + +The live leg does not work today, for the two reasons in Why. It needs one contained change +in the harness, in the run path: + +- `request_draining` gains an out-parameter for `available_commands_update`. It keeps + discarding every other notification, which is the behavior its doc comment describes and + the reason it exists (a replayed `session/load` must not re-enter the doc). +- After the handshake, the run emits `AgentEvent::AvailableCommands` from the captured + update when there is one, and from `init_commands` otherwise. The gate at + `acp/mod.rs:2218` stops being "initialize said something" and becomes "we have a list". + +This is about fifteen lines. It is not free, as first assumed, but it is the only way the +running-session correction exists at all, and it also fixes a silent hole: an agent that +advertises its commands only after `session/new` is invisible to Zeron today. + ### The harness probe `Harness::commands` stops caching and becomes a plain probe: @@ -86,9 +119,19 @@ async fn commands(&self, cwd: Option<&str>) -> Result, Harness ``` - The `OnceCell` at `acp/mod.rs:548` is deleted. -- `discover_commands` passes the cwd to both `spawn_agent` and `session/new`. +- `discover_commands` passes the cwd to `session/new` only. It does **not** set the child + process directory. `spawn_agent` calls `current_dir` (`acp/mod.rs:734-736`), and a + missing directory then fails the spawn with `ErrorKind::NotFound`, which maps to + `HarnessError::NotInstalled` (`acp/mod.rs:741-744`). A deleted worktree would report + "adapter not installed" and never reach the retry below. The adapter resolves skills from + the session cwd, so the child's own directory buys nothing. +- With a `cwd` supplied, the probe always opens a session. Today it skips `session/new` + whenever initialize advertised commands (`acp/mod.rs:780-781`). That shortcut would make + the new cwd dead for any agent that answers initialize, and would fill every cwd key with + one identical list. - `cwd: None` means `$HOME`. That is today's behavior, kept for callers with no workspace. -- The trait default still returns an empty list for non-ACP harnesses. +- The trait default still returns an empty list for non-ACP harnesses. `MockHarness` and + `AcpHarness` are the only implementors, so the signature change is contained. `discover_models` keeps its own `OnceCell` and its `$HOME` cwd. Models are not treated as workspace-scoped in this spec. See Non-goals. @@ -111,17 +154,18 @@ cache in the engine is fed by both, and the harness stays a thin protocol client | Concurrency | single-flight per key; waiters subscribe to the in-flight probe | | Live write | `AvailableCommands` overwrites `(harness, run_cwd)` and resets its TTL | -Key normalization reuses `expand_home` (`sessions.rs:1032-1042`) and trims trailing -separators. The engine runs on the host device, so it expands `~` with the right home. -This matters for the live write: `run_cwd` is `request.cwd`, which can still hold `~` -(`sessions.rs:1072`). Both writers must produce the same key, or a running chat would fill -one entry while the popup reads another. +Key normalization reuses `expand_home` and trims trailing separators. `expand_home` is +private to `sessions.rs:1032-1042` today, so it moves or becomes `pub(crate)`. + +Only the RPC side needs the expansion. The run path already expands at +`sessions.rs:298` ("expand it here, on the host, where the run spawns") before `drive_run` +captures `run_cwd` (`sessions.rs:1072`), so the live write always carries an absolute path. +The popup can still send `~` for a project-less chat. Both writers must land on one key. -The live write keys by the **request** cwd, not by the cwd on the `SessionStarted` event. -The rule next door is the opposite: `sessions.rs:1577` scopes a stored session id by the -event's own cwd, because that is where the harness really created the session. The command -cache needs the other one. The popup looks up by `chat.cwd`, which is the request cwd, so -keying by the event would fill an entry that nothing ever reads. +The live write keys by `run_cwd`. For the ACP harness this is not a real fork in the road: +`SessionStarted` carries `request.cwd` verbatim (`acp/mod.rs:2208`), so the event's cwd and +the request's cwd are the same value. The contrasting rule at `sessions.rs:1577`, which +scopes a stored session id by the event's own cwd, does not apply here. Entry states are explicit, which makes single-flight testable: @@ -129,31 +173,55 @@ Entry states are explicit, which makes single-flight testable: - `Failed { error, at }` - `InFlight { subscribers }` -A read that finds `InFlight` waits on it. A read that finds a stale entry treats it as a -miss and probes. The engine never answers with a stale list, because it has no way to push -a correction afterwards. Freshness on the wire keeps the UI's own stale-while-revalidate -render honest. +Read rules: + +- `InFlight` waits on the in-flight probe. +- A stale entry counts as a miss and probes. The engine never answers with a stale list, + because it has no way to push a correction afterwards. Freshness on the wire keeps the + UI's own stale-while-revalidate render honest. + +Write and eviction rules, because these are the cases the unit tests exist to pin: + +- A live write onto `InFlight` resolves the waiters with the live list and marks the entry + `Fresh`. The live list came from a real session in that cwd, so it is at least as good as + the probe's. +- A probe result that lands on an entry already made `Fresh` by a later live write is + discarded. Newest write wins, compared by timestamp, never by arrival order. +- LRU eviction skips `InFlight` entries. Evicting one would orphan its waiters. +- The cache lock is never held across an await. A read takes the lock, decides, and drops + it before probing or waiting. ### The RPC `ListCommands` gets its own params struct instead of borrowing `ListModelsParams`: ```rust +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] struct ListCommandsParams { harness: HarnessId, cwd: Option, - target_device_id: Option, } ``` +The struct carries no `targetDeviceId`. Forwarding reads that field from the raw params +before any parse (`rpc.rs:986-992`), and `LIST_COMMANDS` is already forwardable +(`rpc.rs:766-772`), so the new `cwd` rides to the host device with no routing work. + `cwd` is optional. An engine on an older device ignores the unknown field and answers with its `$HOME` list, so version skew degrades to today's behavior instead of failing. ### The composer -`slash_cache` is keyed by `(HarnessId, String)`. The cwd resolves when the popup opens: +`slash_cache` is keyed by `(HarnessId, Option, String)`. The device belongs in the +key because the popup already targets a device (`composer.rs:4032-4044`), and two devices +share the same path string for every project-less chat. Without it, one device's list +renders for a chat hosted on another. + +The cwd resolves when the popup opens: -- selected chat: `chat.cwd` +- selected chat: `chat.cwd`, or `~` when it is `None` (the field is optional, + `composer.rs:4414-4418`) - new chat: the space path, or `~` when project-less - the checkout plan is ignored @@ -208,20 +276,31 @@ The clean fix belongs upstream: a capabilities handshake that lists commands wit - key normalization: `~`, trailing separator, two spellings of one path - a stale entry is treated as a miss, not served - TTL expiry and negative TTL -- LRU bound at 16 entries +- LRU bound at 16 entries, and eviction skipping `InFlight` - single-flight: two concurrent reads of one cold key produce one probe +- a live write onto `InFlight` resolves the waiters, and the late probe result is discarded -**Harness, extending `crates/harness/tests/acp.rs:580`** -- the mock agent asserts `session/new` receives the requested cwd +**Harness, in `crates/harness/tests/acp.rs`** +- the `fake-acp.sh` fixture asserts `session/new` receives the requested cwd - a rejected cwd triggers exactly one `$HOME` retry +- a fixture that sends `available_commands_update` immediately after the `session/new` + response produces one `AgentEvent::AvailableCommands` in the run's event stream. This is + the regression test for the dropped notification, and it fails against today's code. +- the existing `commands_discovery_scans_the_initialize_response` (line 580) asserts that a + second call is served from cache. Deleting the `OnceCell` invalidates that assertion, so + the test loses its caching half. The caching contract moves to the engine unit tests. **Engine** - an `AvailableCommands` event during a run writes `(harness, run_cwd)` - a later `ListCommands` for that cwd returns it with no probe **UI** -- cwd resolution for the three cases: chat row, space row, project-less -- switching the project picker changes the cache key +The ui crate has no `TestAppContext` or `gpui::test` coverage today, so a test that drives +the popup is not writable against the current infrastructure. Rather than add a gpui test +harness for this change, cwd resolution is factored into a pure function that takes the +selected chat row, the selected space row, and the device id, and returns the cache key. +The tests cover that function, next to the existing pure-function tests at +`composer.rs:5648`. Anything beyond it is covered by the manual E2E below. **Manual E2E** - open a project with installed project skills, type `/`, confirm they appear From 8b7fdf735af291bb21eababe405712205187281f Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 13:49:52 +0400 Subject: [PATCH 03/17] test: keep /usr/bin on the shell-env fallback fixture's PATH MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit falls_back_when_interactive_attempt_hangs failed on every macOS run. The fixture set PATH to /zeron-test/fallback/bin:/bin, then the probe script ran env — which lives only at /usr/bin/env on macOS, with no /bin/env. The shell found no env, printed the two markers with nothing between them, and the fallback parsed as a failure. Linux has /bin/env, so CI never saw it. The sibling snapshots_path_from_fake_shell already keeps /usr/bin for the same reason. --- crates/harness/src/shell_env.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/harness/src/shell_env.rs b/crates/harness/src/shell_env.rs index 28af6a008..fed9cf28a 100644 --- a/crates/harness/src/shell_env.rs +++ b/crates/harness/src/shell_env.rs @@ -323,7 +323,12 @@ exit 1 let shell = fake_shell( dir.path(), &format!( - "#!/bin/sh\ncase \" $* \" in *\" -i \"*) sleep 60;; esac\nPATH=\"/zeron-test/fallback/bin:/bin\"; export PATH\n{RUN_PAYLOAD}" + // `/usr/bin` must stay on the fixture's PATH: the probe + // script runs `env`, which lives only at /usr/bin/env on + // macOS. Without it the shell finds no `env`, prints the + // markers with nothing between them, and the fallback looks + // like a failure that only reproduces off Linux. + "#!/bin/sh\ncase \" $* \" in *\" -i \"*) sleep 60;; esac\nPATH=\"/zeron-test/fallback/bin:/usr/bin:/bin\"; export PATH\n{RUN_PAYLOAD}" ), ); let start = Instant::now(); From b2ea5c31484893bf35723831aa26d00e725288ee Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 13:59:50 +0400 Subject: [PATCH 04/17] harness: capture available_commands_update during the session handshake The setup window discarded every notification, including the one that carries the agent's cwd-scoped command list. Agents that advertise commands only after session/new (claude-agent-acp) were invisible. --- crates/harness/src/acp/mod.rs | 75 ++++++++++++++++++++--- crates/harness/tests/acp.rs | 38 ++++++++++++ crates/harness/tests/fixtures/fake-acp.sh | 6 ++ 3 files changed, 109 insertions(+), 10 deletions(-) diff --git a/crates/harness/src/acp/mod.rs b/crates/harness/src/acp/mod.rs index 5b6c664e0..36126d06b 100644 --- a/crates/harness/src/acp/mod.rs +++ b/crates/harness/src/acp/mod.rs @@ -1877,12 +1877,14 @@ fn cursor_answer_outcome(asked: &[CursorQuestion], answers: &[UserInputAnswer]) /// Await a setup request while draining incoming messages, so a `session/load` /// whose replay outruns the incoming channel's capacity can't deadlock the /// reader. Replayed `session/update`s are dropped (the doc already holds the -/// history); server requests are answered. +/// history) except `available_commands_update`, which is captured (see +/// [`capture_available_commands`]); server requests are answered. async fn request_draining( client: &RpcClient, incoming: &mut mpsc::Receiver, method: &'static str, params: Value, + captured: &mut Option>, ) -> Result { let mut fut = prompt_like_request(client.clone(), method, params); let res = loop { @@ -1892,6 +1894,9 @@ async fn request_draining( Some(Incoming::Request { id, method, params }) => { handle_server_request(client, id, &method, ¶ms); } + Some(Incoming::Notification { method, params }) => { + capture_available_commands(&method, ¶ms, captured); + } Some(_) => {} None => { return Err(HarnessError::Protocol(format!( @@ -1905,13 +1910,39 @@ async fn request_draining( // replay updates the reader forwarded BEFORE the response line may still // sit in the buffer — flush them now or they'd leak into the live turn. while let Ok(inc) = incoming.try_recv() { - if let Incoming::Request { id, method, params } = inc { - handle_server_request(client, id, &method, ¶ms); + match inc { + Incoming::Request { id, method, params } => { + handle_server_request(client, id, &method, ¶ms); + } + Incoming::Notification { method, params } => { + capture_available_commands(&method, ¶ms, captured); + } + _ => {} } } res } +/// The one notification the setup window must not drop: agents that advertise +/// their commands only after `session/new` (claude-agent-acp) send it here, and +/// the list is cwd-scoped, which is the whole point of per-workspace discovery. +/// Every other replayed update stays dropped — the doc already holds that history. +fn capture_available_commands(method: &str, params: &Value, out: &mut Option>) { + if method != "session/update" { + return; + } + let Some(update) = params.get("update") else { + return; + }; + if update.get("sessionUpdate").and_then(Value::as_str) != Some("available_commands_update") { + return; + } + let commands = parse_commands(update.get("availableCommands")); + if !commands.is_empty() { + *out = Some(commands); + } +} + fn prompt_like_request( client: RpcClient, method: &'static str, @@ -2002,12 +2033,24 @@ async fn run_session(session: Session) { .await?; let steer_ext = steering_supported(&init); let init_commands = scan_available_commands(&init); + // The session's own advertisement is cwd-scoped, unlike the + // initialize list; captured here so claude-agent-acp's post-session/new + // `available_commands_update` (never seen at initialize) is not lost. + let mut session_commands: Option> = None; let session_params = json!({ "cwd": request.cwd, "mcpServers": [] }); let (session_id, session_response) = if let Some(resume) = &request.resume { let mut load = session_params.clone(); load["sessionId"] = Value::String(resume.clone()); - match request_draining(&client, &mut incoming, "session/load", load).await { + match request_draining( + &client, + &mut incoming, + "session/load", + load, + &mut session_commands, + ) + .await + { Ok(resp) => (resume.clone(), resp), // A missing/foreign session falls back to a fresh one. Err(e) => { @@ -2020,6 +2063,7 @@ async fn run_session(session: Session) { &mut incoming, "session/new", session_params.clone(), + &mut session_commands, ) .await?; ( @@ -2032,8 +2076,14 @@ async fn run_session(session: Session) { } } } else { - let new = - request_draining(&client, &mut incoming, "session/new", session_params).await?; + let new = request_draining( + &client, + &mut incoming, + "session/new", + session_params, + &mut session_commands, + ) + .await?; ( new.get("sessionId") .and_then(Value::as_str) @@ -2079,6 +2129,7 @@ async fn run_session(session: Session) { &mut incoming, "session/set_config_option", Value::Object(params), + &mut session_commands, ) .await { @@ -2117,6 +2168,7 @@ async fn run_session(session: Session) { &mut incoming, "session/set_config_option", Value::Object(params), + &mut session_commands, ) .await { @@ -2126,13 +2178,14 @@ async fn run_session(session: Session) { ); } } - Ok::<(String, bool, Vec), HarnessError>(( + Ok::<(String, bool, Vec, Option>), HarnessError>(( session_id, steer_ext, init_commands, + session_commands, )) }; - let (session_id, steer_ext, init_commands) = tokio::select! { + let (session_id, steer_ext, init_commands, session_commands) = tokio::select! { res = tokio::time::timeout(handshake_timeout, setup) => { let res = res.unwrap_or_else(|_| { // A hung handshake (agent waiting on a login it can never @@ -2215,11 +2268,13 @@ async fn run_session(session: Session) { shutdown_child(&mut child, kill_grace).await; return; } - if !init_commands.is_empty() + // The session's own list wins: it is cwd-scoped, the initialize list is not. + let advertised = session_commands.unwrap_or(init_commands); + if !advertised.is_empty() && !send( &event_tx, AgentEvent::AvailableCommands { - commands: init_commands, + commands: advertised, }, ) .await diff --git a/crates/harness/tests/acp.rs b/crates/harness/tests/acp.rs index f7b7a155a..f876882aa 100644 --- a/crates/harness/tests/acp.rs +++ b/crates/harness/tests/acp.rs @@ -589,6 +589,44 @@ async fn commands_discovery_scans_the_initialize_response() { assert_eq!(again, commands); } +#[tokio::test] +async fn available_commands_update_in_the_handshake_reaches_the_run_stream() { + let harness = harness(); + // An inert scenario: `scenario:happy` also advertises `deep-research` + // mid-turn, which would always postdate (and shadow) the handshake + // capture, defeating the last-write-wins assertion below. + let mut req = request("scenario:resumed"); + req.cwd = "/tmp/live-commands".into(); + // spawn_agent sets this cwd as the child process's real working + // directory (not just a session/new field), so the marker path must + // exist on disk for the spawn to succeed. + std::fs::create_dir_all(&req.cwd).expect("create marker cwd"); + let (controls, _steer, _cancel) = controls(); + let stream = harness.run(req, controls).await.expect("run starts"); + let events: Vec = stream.filter_map(|e| async { e.ok() }).collect().await; + // Assert on the LAST such event, not on the flattened set. This fixture + // also advertises `compact`/`goal` at initialize, and whether the update is + // caught by the handshake capture or by the main loop depends on a read + // race, so the stream may legitimately carry two events. Last write wins + // in production too: `note_live` overwrites the cache entry. + let advertised = events + .iter() + .filter_map(|e| match e { + AgentEvent::AvailableCommands { commands } => Some(commands.clone()), + _ => None, + }) + .next_back() + .unwrap_or_default(); + assert_eq!( + advertised + .iter() + .map(|c| c.name.as_str()) + .collect::>(), + vec!["live"], + "{events:?}" + ); +} + #[tokio::test] async fn missing_binary_surfaces_not_installed_with_install_hint() { let harness = AcpHarness::grok().with_executable("/nonexistent/definitely-not-grok"); diff --git a/crates/harness/tests/fixtures/fake-acp.sh b/crates/harness/tests/fixtures/fake-acp.sh index 788708b9f..d0612e280 100755 --- a/crates/harness/tests/fixtures/fake-acp.sh +++ b/crates/harness/tests/fixtures/fake-acp.sh @@ -53,6 +53,12 @@ elif has "$line" '"method":"session/new"'; then # feeds discovery first; the first-class `models` state (SessionModelState) # is the legacy fallback — codex-acp enumerates model × effort there. emit "{\"id\":$(rid "$line"),\"result\":{\"sessionId\":\"s-1\",\"models\":{\"availableModels\":[{\"modelId\":\"grok-4-fast\",\"name\":\"Grok 4 Fast\",\"description\":\"Fast tier\"},{\"modelId\":\"grok-4.5\",\"name\":\"Grok 4.5\"}],\"currentModelId\":\"grok-4.5\"},\"configOptions\":[{\"id\":\"model\",\"name\":\"Model\",\"category\":\"model\",\"type\":\"select\",\"currentValue\":\"grok-4-fast\",\"options\":[{\"value\":\"grok-4-fast\",\"name\":\"Grok 4 Fast\",\"description\":\"Fast tier\"},{\"value\":\"grok-4.5\",\"name\":\"Grok 4.5\"}]},{\"id\":\"effort\",\"name\":\"Reasoning effort\",\"category\":\"thought_level\",\"type\":\"select\",\"currentValue\":\"high\",\"options\":[{\"value\":\"low\",\"name\":\"Low\"},{\"value\":\"medium\",\"name\":\"Medium\"},{\"value\":\"high\",\"name\":\"High\"}]}]}}" + # Marker cwd: advertise commands the way claude-agent-acp does — as an + # update sent right after the session/new response, inside the handshake + # window where request_draining used to discard notifications. + if has "$line" '"cwd":"/tmp/live-commands"'; then + update '{"sessionUpdate":"available_commands_update","availableCommands":[{"name":"live","description":"From the session"}]}' + fi else exit 1 fi From 9e7d47624da1e6d9eb868bcccbd8615934b7c4df Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 14:06:26 +0400 Subject: [PATCH 05/17] test: drop the loop that never loops in expect_kind clippy's never_loop is correctness-group, so this was a hard error on `cargo clippy --workspace --all-targets`, blocking the lint gate for every crate. The loop could only ever run once: the first iteration returns on a matching frame and panics on anything else. An assert says the same thing without pretending to iterate. --- crates/sync/src/chat_client/tests.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/sync/src/chat_client/tests.rs b/crates/sync/src/chat_client/tests.rs index bd9327a2a..f159f692a 100644 --- a/crates/sync/src/chat_client/tests.rs +++ b/crates/sync/src/chat_client/tests.rs @@ -86,14 +86,14 @@ impl CheckpointFetcher for FixedFetcher { // ── server-side script helpers ────────────────────────────────────────────── async fn expect_kind(end: &mut ServerEnd, kind: u8) -> wire::WireFrame { - loop { - let bytes = end.rx.recv().await.expect("client hung up"); - let frame = decode(&bytes).expect("client sent undecodable frame"); - if frame.kind == kind { - return frame; - } - panic!("expected frame {kind:#x}, got {:#x}", frame.kind); - } + let bytes = end.rx.recv().await.expect("client hung up"); + let frame = decode(&bytes).expect("client sent undecodable frame"); + assert_eq!( + frame.kind, kind, + "expected frame {kind:#x}, got {:#x}", + frame.kind + ); + frame } async fn send(end: &ServerEnd, kind: u8, header: serde_json::Value, payload: &[u8]) { From 21a1edeeb3f230b92be240f3508a3e6d6e82fd56 Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 14:12:50 +0400 Subject: [PATCH 06/17] harness: discover slash commands for one workspace commands() takes a cwd and opens the discovery session there, so project skills are visible. The per-harness OnceCell goes: one list per harness is the wrong unit, and caching moves to the engine. --- crates/engine/src/rpc.rs | 2 +- crates/harness/src/acp/mod.rs | 48 ++++++++++++++--------- crates/harness/src/lib.rs | 9 +++-- crates/harness/tests/acp.rs | 32 +++++++++++++-- crates/harness/tests/fixtures/fake-acp.sh | 8 ++++ 5 files changed, 73 insertions(+), 26 deletions(-) diff --git a/crates/engine/src/rpc.rs b/crates/engine/src/rpc.rs index 49d870add..151ca34c9 100644 --- a/crates/engine/src/rpc.rs +++ b/crates/engine/src/rpc.rs @@ -1030,7 +1030,7 @@ impl RpcService for EngineRpc { .resolve(p.harness) .map_err(|e| RpcError::Failed(e.to_string()))?; let commands = harness - .commands() + .commands(None) .await .map_err(|e| RpcError::Failed(e.to_string()))?; RpcReply::value(&commands) diff --git a/crates/harness/src/acp/mod.rs b/crates/harness/src/acp/mod.rs index 36126d06b..f5ddb3b3f 100644 --- a/crates/harness/src/acp/mod.rs +++ b/crates/harness/src/acp/mod.rs @@ -544,8 +544,6 @@ pub struct AcpHarness { /// Bound on the initialize → session handshake; a hang past it errors the /// run instead of spinning "Working" forever. handshake_timeout: Duration, - /// Discovery result cache: the advertised commands survive across calls. - commands: tokio::sync::OnceCell>, /// Model discovery cache: only a successful, non-empty probe is cached, /// so a mis-authed agent retries on the next picker open. models_cache: tokio::sync::OnceCell>, @@ -562,7 +560,6 @@ impl AcpHarness { // (session/load replays from disk), so a hang past this is a // wedged agent, not a slow one. handshake_timeout: Duration::from_secs(120), - commands: tokio::sync::OnceCell::new(), models_cache: tokio::sync::OnceCell::new(), } } @@ -759,12 +756,19 @@ impl AcpHarness { Ok((child, stderr_tail)) } - /// Short-lived discovery run for [`Harness::commands`]: initialize, scan - /// the response, then try one unauthenticated `session/new` and wait - /// briefly for `available_commands_update`. Best-effort — an agent that - /// refuses sessions before login still surfaces whatever the handshake - /// advertised. - async fn discover_commands(&self) -> Result, HarnessError> { + /// Short-lived discovery run: initialize, scan the response, then open one + /// session in `cwd` and wait briefly for `available_commands_update`. + /// Best-effort — an agent that refuses sessions before login still + /// surfaces whatever the handshake advertised. + /// + /// The cwd rides `session/new` only. Setting it as the CHILD's working + /// directory would turn a deleted worktree into a spawn `NotFound`, which + /// this module reports as `NotInstalled` ("adapter missing") — a lie the + /// user cannot act on. + async fn discover_commands( + &self, + cwd: Option<&str>, + ) -> Result, HarnessError> { let (mut child, _stderr) = self.spawn_agent(None, false).await?; let (client, mut incoming) = match (child.stdin.take(), child.stdout.take()) { (Some(stdin), Some(stdout)) => RpcClient::new(stdin, stdout), @@ -778,11 +782,22 @@ impl AcpHarness { .request("initialize", initialize_params(self.spec.id)) .await?; let mut commands = scan_available_commands(&init); - if commands.is_empty() { - let cwd = std::env::var("HOME").unwrap_or_else(|_| "/".into()); - let session = client - .request("session/new", json!({ "cwd": cwd, "mcpServers": [] })) + let home = || std::env::var("HOME").unwrap_or_else(|_| "/".into()); + // With a workspace asked for, ALWAYS open a session: the initialize + // list is not cwd-scoped, so trusting it would make the workspace + // moot for every agent that answers initialize. + if cwd.is_some() || commands.is_empty() { + let requested = cwd.map(str::to_string).unwrap_or_else(home); + let mut session = client + .request("session/new", json!({ "cwd": requested, "mcpServers": [] })) .await; + if session.is_err() && cwd.is_some() { + // A path that no longer exists (deleted worktree): one retry + // from home, so the popup still shows the built-ins. + session = client + .request("session/new", json!({ "cwd": home(), "mcpServers": [] })) + .await; + } if session.is_ok() { // The update usually arrives within milliseconds of the // session response; 2s bounds a quiet agent. @@ -1167,11 +1182,8 @@ impl Harness for AcpHarness { } } - async fn commands(&self) -> Result, HarnessError> { - self.commands - .get_or_try_init(|| self.discover_commands()) - .await - .cloned() + async fn commands(&self, cwd: Option<&str>) -> Result, HarnessError> { + self.discover_commands(cwd).await } async fn run( diff --git a/crates/harness/src/lib.rs b/crates/harness/src/lib.rs index ef0e27ebc..264a655fc 100644 --- a/crates/harness/src/lib.rs +++ b/crates/harness/src/lib.rs @@ -67,9 +67,12 @@ pub trait Harness: Send + Sync { true } async fn models(&self) -> Result, HarnessError>; - /// Slash commands the agent advertises (ACP `availableCommands`); empty - /// for harnesses without them. May spawn a short-lived discovery process. - async fn commands(&self) -> Result, HarnessError> { + /// Slash commands the agent advertises for one workspace (ACP + /// `availableCommands`); empty for harnesses without them. `cwd` is a path + /// on THIS device; `None` means the host's home directory. May spawn a + /// short-lived discovery process — callers are expected to cache. + async fn commands(&self, cwd: Option<&str>) -> Result, HarnessError> { + let _ = cwd; Ok(Vec::new()) } /// Run one (persistent) session; the stream ends with `AgentEvent::Done`. diff --git a/crates/harness/tests/acp.rs b/crates/harness/tests/acp.rs index f876882aa..599874c6d 100644 --- a/crates/harness/tests/acp.rs +++ b/crates/harness/tests/acp.rs @@ -579,14 +579,38 @@ async fn failed_load_falls_back_to_a_fresh_session() { #[tokio::test] async fn commands_discovery_scans_the_initialize_response() { let harness = harness(); - let commands = harness.commands().await.expect("discovery"); + let commands = harness.commands(None).await.expect("discovery"); assert_eq!(commands.len(), 2, "{commands:?}"); assert_eq!(commands[0].name, "compact"); assert_eq!(commands[1].name, "goal"); assert_eq!(commands[1].input_hint.as_deref(), Some("the goal")); - // Cached: a second call must not respawn (same result, instant). - let again = harness.commands().await.expect("cached"); - assert_eq!(again, commands); +} + +#[tokio::test] +async fn commands_discovery_opens_a_session_in_the_requested_cwd() { + let harness = harness(); + let commands = harness + .commands(Some("/tmp/live-commands")) + .await + .expect("discovery"); + // The session's list replaces the initialize list, even though initialize + // advertised two commands: only the session knows the workspace. + assert_eq!( + commands.iter().map(|c| c.name.as_str()).collect::>(), + vec!["live"], + "{commands:?}" + ); +} + +#[tokio::test] +async fn a_rejected_cwd_retries_once_from_home() { + let harness = harness(); + let commands = harness + .commands(Some("/tmp/reject-cwd")) + .await + .expect("discovery falls back instead of failing"); + // The retry succeeded, so the initialize-advertised list survives. + assert_eq!(commands.len(), 2, "{commands:?}"); } #[tokio::test] diff --git a/crates/harness/tests/fixtures/fake-acp.sh b/crates/harness/tests/fixtures/fake-acp.sh index d0612e280..4faf69763 100755 --- a/crates/harness/tests/fixtures/fake-acp.sh +++ b/crates/harness/tests/fixtures/fake-acp.sh @@ -47,6 +47,14 @@ if has "$line" '"method":"session/load"'; then emit "{\"id\":$(rid "$line"),\"result\":{}}" fi elif has "$line" '"method":"session/new"'; then + # Reject one marker cwd once, then require the retry to use a different + # directory. Exercises the deleted-worktree path. + if has "$line" '"cwd":"/tmp/reject-cwd"'; then + emit "{\"id\":$(rid "$line"),\"error\":{\"code\":-32602,\"message\":\"bad cwd\"}}" + read -r line || exit 1 + has "$line" '"method":"session/new"' || exit 1 + has "$line" '"cwd":"/tmp/reject-cwd"' && exit 1 + fi has "$line" '"mcpServers":[]' || exit 1 # Advertise config options: model (current differs from the tests' request, # forcing a set) and thought_level (current high). The model config option From 5b765c399094cf04c555f01af984b9d5ca0d46c9 Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 14:21:19 +0400 Subject: [PATCH 07/17] harness: pin the reject-cwd retry to an observable command, not a count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a_rejected_cwd_retries_once_from_home only asserted commands.len() == 2, which the pre-retry initialize scan already satisfies on its own — deleting the retry block left the test green. The fixture now advertises a command only the retry's own session/new response can trigger, and the test asserts on that exact command, so the retry is now the only way to pass. --- crates/harness/tests/acp.rs | 10 ++++++++-- crates/harness/tests/fixtures/fake-acp.sh | 7 +++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/crates/harness/tests/acp.rs b/crates/harness/tests/acp.rs index 599874c6d..53576b0a8 100644 --- a/crates/harness/tests/acp.rs +++ b/crates/harness/tests/acp.rs @@ -609,8 +609,14 @@ async fn a_rejected_cwd_retries_once_from_home() { .commands(Some("/tmp/reject-cwd")) .await .expect("discovery falls back instead of failing"); - // The retry succeeded, so the initialize-advertised list survives. - assert_eq!(commands.len(), 2, "{commands:?}"); + // Only the retry's own session/new response makes the fixture send this + // update; the pre-retry error carries no commands. Pinning the exact + // list (not just its length) fails if the retry is ever deleted. + assert_eq!( + commands.iter().map(|c| c.name.as_str()).collect::>(), + vec!["home-retry"], + "{commands:?}" + ); } #[tokio::test] diff --git a/crates/harness/tests/fixtures/fake-acp.sh b/crates/harness/tests/fixtures/fake-acp.sh index 4faf69763..fa0301466 100755 --- a/crates/harness/tests/fixtures/fake-acp.sh +++ b/crates/harness/tests/fixtures/fake-acp.sh @@ -54,6 +54,7 @@ elif has "$line" '"method":"session/new"'; then read -r line || exit 1 has "$line" '"method":"session/new"' || exit 1 has "$line" '"cwd":"/tmp/reject-cwd"' && exit 1 + RETRIED=1 fi has "$line" '"mcpServers":[]' || exit 1 # Advertise config options: model (current differs from the tests' request, @@ -67,6 +68,12 @@ elif has "$line" '"method":"session/new"'; then if has "$line" '"cwd":"/tmp/live-commands"'; then update '{"sessionUpdate":"available_commands_update","availableCommands":[{"name":"live","description":"From the session"}]}' fi + # The reject-cwd retry succeeded: prove discovery actually consumed the + # retried session, not just the pre-retry error, by advertising a command + # only the retry's own session/new response can trigger. + if [ "$RETRIED" = "1" ]; then + update '{"sessionUpdate":"available_commands_update","availableCommands":[{"name":"home-retry","description":"Answered after the retry from home"}]}' + fi else exit 1 fi From 6c78309efc5910645a23100c169cee005c36900c Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 14:30:58 +0400 Subject: [PATCH 08/17] engine: cache slash commands per (harness, cwd) TTL, single-flight, and negative caching, because a cold probe spawns an agent process and runs the project's SessionStart hooks. A live session's list outranks a probe and resolves any waiter on it. --- crates/engine/src/commands.rs | 452 ++++++++++++++++++++++++++++++++++ crates/engine/src/lib.rs | 1 + crates/engine/src/sessions.rs | 2 +- 3 files changed, 454 insertions(+), 1 deletion(-) create mode 100644 crates/engine/src/commands.rs diff --git a/crates/engine/src/commands.rs b/crates/engine/src/commands.rs new file mode 100644 index 000000000..19b6584d4 --- /dev/null +++ b/crates/engine/src/commands.rs @@ -0,0 +1,452 @@ +//! Slash commands, cached per `(harness, cwd)`. +//! +//! ACP advertises commands per session, and a session is defined by its cwd: +//! project skills under `/.claude/skills` exist only for a session +//! opened there. So the cache unit is the workspace, not the harness. +//! +//! Two writers feed it. A cold read probes the harness, which spawns a +//! short-lived agent process (and, for Claude, runs that project's SessionStart +//! hooks) — hence the TTL, the single-flight, and the negative caching. A +//! running chat feeds it for free through `AgentEvent::AvailableCommands`. + +use std::collections::HashMap; +use std::future::Future; +use std::sync::Mutex; +use std::time::{Duration, Instant}; + +use tokio::sync::broadcast; +use zeron_proto::{HarnessId, SlashCommand}; + +/// Bounded because a user with many worktrees would otherwise accumulate one +/// entry per directory, forever. +const MAX_ENTRIES: usize = 16; +const FRESH_TTL: Duration = Duration::from_secs(600); +const NEGATIVE_TTL: Duration = Duration::from_secs(30); + +type Key = (HarnessId, String); +type Probed = Result, String>; + +enum Entry { + Fresh { + commands: Vec, + at: Instant, + }, + Failed { + error: String, + at: Instant, + }, + InFlight { + tx: broadcast::Sender, + }, +} + +struct Slot { + entry: Entry, + /// Move-to-front stand-in: eviction drops the least recently touched. + touched: Instant, +} + +/// What a locked lookup resolves to, decided before any mutation. +/// +/// `get`'s classifying match borrows `slot.entry` immutably; the stale-entry +/// arm needs to overwrite that same field, which the borrow checker rejects +/// while the match is live. So the match only reads and produces one of +/// these owned values, and the write happens after the match expression has +/// ended (still inside the same lock acquisition, so the decision stays +/// atomic with the write). +enum Lookup { + Fresh(Vec), + Failed(String), + InFlight(broadcast::Receiver), + /// A cold key: absent, or present but stale. The engine never serves a + /// stale list — it has no way to push a correction afterwards — so stale + /// counts as a miss. `existed` tells the write side whether the coming + /// insert grows the map (and so needs an eviction pass) or just replaces + /// a slot that was already counted. + Miss { + existed: bool, + }, +} + +pub struct CommandCache { + fresh_ttl: Duration, + negative_ttl: Duration, + slots: Mutex>, +} + +impl Default for CommandCache { + fn default() -> Self { + Self::new() + } +} + +impl CommandCache { + pub fn new() -> Self { + Self::with_ttls(FRESH_TTL, NEGATIVE_TTL) + } + + pub fn with_ttls(fresh_ttl: Duration, negative_ttl: Duration) -> Self { + Self { + fresh_ttl, + negative_ttl, + slots: Mutex::new(HashMap::new()), + } + } + + /// One spelling per directory. `None` is the host's home, which is what an + /// older client (no `cwd` field) and a project-less chat both mean. + pub fn normalize(cwd: Option<&str>) -> String { + let raw = cwd.map(str::trim).filter(|c| !c.is_empty()).unwrap_or("~"); + let expanded = crate::sessions::expand_home(raw); + let trimmed = expanded.trim_end_matches('/'); + if trimmed.is_empty() { + "/".to_string() + } else { + trimmed.to_string() + } + } + + pub fn len(&self) -> usize { + self.slots.lock().expect("cache lock").len() + } + + pub fn is_empty(&self) -> bool { + self.len() == 0 + } + + /// The list for one workspace. `probe` receives the normalized cwd and runs + /// only on a miss; concurrent readers of one cold key share a single run. + pub async fn get(&self, harness: HarnessId, cwd: Option<&str>, probe: F) -> Probed + where + F: FnOnce(String) -> Fut, + Fut: Future, + { + let key = (harness, Self::normalize(cwd)); + // One lock acquisition covers both the classify and the write: if we + // dropped the lock between them, two callers could each classify a + // cold key as a miss before either had inserted its InFlight slot, + // and both would then start their own probe — defeating single-flight. + let waiter = { + let mut slots = self.slots.lock().expect("cache lock"); + let now = Instant::now(); + // The immutable borrow of `slot.entry` this match creates lives + // only for the match expression: `lookup` holds owned values, so + // the borrow is gone by the time we reach the write below. + let lookup = match slots.get(&key) { + Some(slot) => match &slot.entry { + Entry::Fresh { commands, at } if now.duration_since(*at) < self.fresh_ttl => { + Lookup::Fresh(commands.clone()) + } + Entry::Failed { error, at } if now.duration_since(*at) < self.negative_ttl => { + Lookup::Failed(error.clone()) + } + Entry::InFlight { tx } => Lookup::InFlight(tx.subscribe()), + // Stale: a miss. The engine never serves a stale list, + // because it has no way to push a correction afterwards. + _ => Lookup::Miss { existed: true }, + }, + None => Lookup::Miss { existed: false }, + }; + match lookup { + Lookup::Fresh(commands) => { + if let Some(slot) = slots.get_mut(&key) { + slot.touched = now; + } + return Ok(commands); + } + Lookup::Failed(error) => { + if let Some(slot) = slots.get_mut(&key) { + slot.touched = now; + } + return Err(error); + } + Lookup::InFlight(rx) => { + if let Some(slot) = slots.get_mut(&key) { + slot.touched = now; + } + Some(rx) + } + Lookup::Miss { existed } => { + let (tx, _) = broadcast::channel(4); + slots.insert( + key.clone(), + Slot { + entry: Entry::InFlight { tx }, + touched: now, + }, + ); + if !existed { + self.evict_locked(&mut slots); + } + None + } + } + }; + if let Some(mut rx) = waiter { + return match rx.recv().await { + Ok(result) => result, + Err(_) => Err("command discovery was dropped".into()), + }; + } + let started = Instant::now(); + let result = probe(key.1.clone()).await; + self.commit(key, started, result) + } + + /// A running session's own list. It came from a real session in that cwd, + /// so it outranks anything a probe could produce. + pub fn note_live(&self, harness: HarnessId, cwd: &str, commands: Vec) { + if commands.is_empty() { + return; + } + let key = (harness, Self::normalize(Some(cwd))); + let mut slots = self.slots.lock().expect("cache lock"); + let now = Instant::now(); + let previous = slots.insert( + key, + Slot { + entry: Entry::Fresh { + commands: commands.clone(), + at: now, + }, + touched: now, + }, + ); + // Waiters on an in-flight probe get the better answer immediately. + if let Some(Slot { + entry: Entry::InFlight { tx }, + .. + }) = previous + { + let _ = tx.send(Ok(commands)); + } + self.evict_locked(&mut slots); + } + + fn commit(&self, key: Key, started: Instant, result: Probed) -> Probed { + let mut slots = self.slots.lock().expect("cache lock"); + let now = Instant::now(); + // A live write that landed while the probe ran is newer and better. + if let Some(Slot { + entry: Entry::Fresh { commands, at }, + .. + }) = slots.get(&key) + && *at > started + { + return Ok(commands.clone()); + } + let entry = match &result { + Ok(commands) => Entry::Fresh { + commands: commands.clone(), + at: now, + }, + Err(error) => Entry::Failed { + error: error.clone(), + at: now, + }, + }; + if let Some(Slot { + entry: Entry::InFlight { tx }, + .. + }) = slots.insert( + key, + Slot { + entry, + touched: now, + }, + ) { + let _ = tx.send(result.clone()); + } + self.evict_locked(&mut slots); + result + } + + /// Drop the least recently touched settled entries. In-flight ones are + /// skipped: evicting one orphans its waiters. + fn evict_locked(&self, slots: &mut HashMap) { + while slots.len() > MAX_ENTRIES { + let victim = slots + .iter() + .filter(|(_, slot)| !matches!(slot.entry, Entry::InFlight { .. })) + .min_by_key(|(_, slot)| slot.touched) + .map(|(key, _)| key.clone()); + match victim { + Some(key) => { + slots.remove(&key); + } + None => break, + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::atomic::{AtomicUsize, Ordering}; + + fn cmd(name: &str) -> SlashCommand { + SlashCommand { + name: name.into(), + description: String::new(), + input_hint: None, + } + } + + fn short() -> CommandCache { + CommandCache::with_ttls(Duration::from_millis(80), Duration::from_millis(80)) + } + + #[test] + fn normalize_folds_tilde_and_trailing_separator() { + let home = CommandCache::normalize(Some("~")); + assert!(home.starts_with('/'), "{home}"); + assert_eq!(CommandCache::normalize(None), home, "None means home"); + assert_eq!( + CommandCache::normalize(Some("/repo/")), + CommandCache::normalize(Some("/repo")) + ); + } + + #[tokio::test] + async fn a_fresh_entry_is_served_without_probing() { + let cache = CommandCache::new(); + let probes = AtomicUsize::new(0); + for _ in 0..2 { + let got = cache + .get(HarnessId::Mock, Some("/repo"), |_| async { + probes.fetch_add(1, Ordering::SeqCst); + Ok(vec![cmd("a")]) + }) + .await + .expect("probe ok"); + assert_eq!(got, vec![cmd("a")]); + } + assert_eq!( + probes.load(Ordering::SeqCst), + 1, + "second read must be cached" + ); + } + + #[tokio::test] + async fn a_stale_entry_is_a_miss() { + let cache = short(); + let probes = AtomicUsize::new(0); + for _ in 0..2 { + let _ = cache + .get(HarnessId::Mock, Some("/repo"), |_| async { + probes.fetch_add(1, Ordering::SeqCst); + Ok(vec![cmd("a")]) + }) + .await; + tokio::time::sleep(Duration::from_millis(120)).await; + } + assert_eq!(probes.load(Ordering::SeqCst), 2, "stale must re-probe"); + } + + #[tokio::test] + async fn a_failure_is_cached_then_expires() { + let cache = short(); + let probes = AtomicUsize::new(0); + let call = || async { + cache + .get(HarnessId::Mock, Some("/repo"), |_| async { + probes.fetch_add(1, Ordering::SeqCst); + Err::, String>("adapter missing".into()) + }) + .await + }; + assert_eq!(call().await.unwrap_err(), "adapter missing"); + assert_eq!(call().await.unwrap_err(), "adapter missing"); + assert_eq!(probes.load(Ordering::SeqCst), 1, "negative TTL holds"); + tokio::time::sleep(Duration::from_millis(120)).await; + let _ = call().await; + assert_eq!(probes.load(Ordering::SeqCst), 2, "negative TTL expires"); + } + + #[tokio::test] + async fn concurrent_reads_of_a_cold_key_probe_once() { + let cache = std::sync::Arc::new(CommandCache::new()); + let probes = std::sync::Arc::new(AtomicUsize::new(0)); + let mut tasks = Vec::new(); + for _ in 0..4 { + let cache = cache.clone(); + let probes = probes.clone(); + tasks.push(tokio::spawn(async move { + cache + .get(HarnessId::Mock, Some("/repo"), move |_| async move { + probes.fetch_add(1, Ordering::SeqCst); + tokio::time::sleep(Duration::from_millis(40)).await; + Ok(vec![cmd("a")]) + }) + .await + })); + } + for task in tasks { + assert_eq!(task.await.expect("join").expect("probe ok"), vec![cmd("a")]); + } + assert_eq!(probes.load(Ordering::SeqCst), 1, "single-flight"); + } + + #[tokio::test] + async fn a_live_write_resolves_waiters_and_beats_the_late_probe() { + let cache = std::sync::Arc::new(CommandCache::new()); + let reader = { + let cache = cache.clone(); + tokio::spawn(async move { + cache + .get(HarnessId::Mock, Some("/repo"), |_| async { + tokio::time::sleep(Duration::from_millis(80)).await; + Ok(vec![cmd("from-probe")]) + }) + .await + }) + }; + tokio::time::sleep(Duration::from_millis(20)).await; + cache.note_live(HarnessId::Mock, "/repo", vec![cmd("from-session")]); + assert_eq!( + reader.await.expect("join").expect("resolved"), + vec![cmd("from-session")], + "the waiter takes the live list" + ); + let after = cache + .get(HarnessId::Mock, Some("/repo"), |_| async { + panic!("must not probe") + }) + .await + .expect("cached"); + assert_eq!(after, vec![cmd("from-session")], "late probe discarded"); + } + + #[tokio::test] + async fn eviction_bounds_the_map_and_spares_in_flight_entries() { + let cache = std::sync::Arc::new(CommandCache::new()); + let slow = { + let cache = cache.clone(); + tokio::spawn(async move { + cache + .get(HarnessId::Mock, Some("/slow"), |_| async { + tokio::time::sleep(Duration::from_millis(200)).await; + Ok(vec![cmd("slow")]) + }) + .await + }) + }; + tokio::time::sleep(Duration::from_millis(20)).await; + for i in 0..20 { + let path = format!("/repo{i}"); + let _ = cache + .get(HarnessId::Mock, Some(&path), |_| async { + Ok(vec![cmd("x")]) + }) + .await; + } + assert!(cache.len() <= 16, "bounded, got {}", cache.len()); + assert_eq!( + slow.await.expect("join").expect("probe ok"), + vec![cmd("slow")], + "an in-flight entry must never be evicted out from under its waiters" + ); + } +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 6a3d9ee55..b4e246b95 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -18,6 +18,7 @@ use zeron_sync::DocsStore; pub mod agent_accounts; pub mod auth; pub mod chat2_host; +pub mod commands; pub mod diff_sync; pub mod doc_host; pub mod instance_lock; diff --git a/crates/engine/src/sessions.rs b/crates/engine/src/sessions.rs index 09f9b888a..e8f3fe44b 100644 --- a/crates/engine/src/sessions.rs +++ b/crates/engine/src/sessions.rs @@ -1030,7 +1030,7 @@ fn finish_segment<'a>( } /// `~` / `~/…` → this host's home directory. Anything else passes through. -fn expand_home(cwd: &str) -> String { +pub(crate) fn expand_home(cwd: &str) -> String { match cwd.strip_prefix("~") { Some("") => crate::repos::home_dir().to_string_lossy().into_owned(), Some(rest) if rest.starts_with('/') => crate::repos::home_dir() From 94f117eea7874128c43f8a9a780e15eba11762de Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 14:42:12 +0400 Subject: [PATCH 09/17] engine: close a TOCTOU window in the command cache's newest-wins check get() captured `started` with a fresh Instant::now() after releasing the lock that inserted the InFlight entry. A note_live landing in that gap could carry an earlier timestamp than `started` and lose to a later, staler probe result, inverting the newest-wins rule. Thread the lock-held insertion timestamp out as `started` instead, which closes the window to the lock's own critical section. Also exercises the live-write-beats-late-probe test under a multi-thread tokio runtime, since the current-thread default cannot preempt across the await points involved and so cannot exercise the window either the bug or the fix cares about. --- crates/engine/src/commands.rs | 69 +++++++++++++++++++++++++++++------ 1 file changed, 57 insertions(+), 12 deletions(-) diff --git a/crates/engine/src/commands.rs b/crates/engine/src/commands.rs index 19b6584d4..ac31cca7d 100644 --- a/crates/engine/src/commands.rs +++ b/crates/engine/src/commands.rs @@ -68,6 +68,17 @@ enum Lookup { }, } +/// What `get` does once its lock section has ended: either await the +/// in-flight probe someone else already started, or run its own probe. +enum NextStep { + Wait(broadcast::Receiver), + /// The `Instant` here is the InFlight entry's own insertion time, taken + /// under the lock — not a fresh `Instant::now()` after the lock is + /// released. See the comment where this variant is built for why that + /// distinction is load-bearing. + Probe(Instant), +} + pub struct CommandCache { fresh_ttl: Duration, negative_ttl: Duration, @@ -126,7 +137,7 @@ impl CommandCache { // dropped the lock between them, two callers could each classify a // cold key as a miss before either had inserted its InFlight slot, // and both would then start their own probe — defeating single-flight. - let waiter = { + let step = { let mut slots = self.slots.lock().expect("cache lock"); let now = Instant::now(); // The immutable borrow of `slot.entry` this match creates lives @@ -164,7 +175,7 @@ impl CommandCache { if let Some(slot) = slots.get_mut(&key) { slot.touched = now; } - Some(rx) + NextStep::Wait(rx) } Lookup::Miss { existed } => { let (tx, _) = broadcast::channel(4); @@ -178,19 +189,30 @@ impl CommandCache { if !existed { self.evict_locked(&mut slots); } - None + // `now` was read under this same lock, before the + // InFlight entry became visible to any other caller — so + // no `note_live` write can have an earlier timestamp and + // still lose to this probe. Reusing it as `started` + // instead of taking a fresh `Instant::now()` after the + // lock is released closes a real TOCTOU window: a + // `note_live` landing in that gap would otherwise carry + // an `at` provably before a freshly-captured `started`, + // so `commit`'s `*at > started` guard would wrongly + // discard the live write and keep the stale probe. + NextStep::Probe(now) } } }; - if let Some(mut rx) = waiter { - return match rx.recv().await { + match step { + NextStep::Wait(mut rx) => match rx.recv().await { Ok(result) => result, Err(_) => Err("command discovery was dropped".into()), - }; + }, + NextStep::Probe(started) => { + let result = probe(key.1.clone()).await; + self.commit(key, started, result) + } } - let started = Instant::now(); - let result = probe(key.1.clone()).await; - self.commit(key, started, result) } /// A running session's own list. It came from a real session in that cwd, @@ -389,9 +411,14 @@ mod tests { assert_eq!(probes.load(Ordering::SeqCst), 1, "single-flight"); } - #[tokio::test] - async fn a_live_write_resolves_waiters_and_beats_the_late_probe() { - let cache = std::sync::Arc::new(CommandCache::new()); + // Shared by both runtime flavors below: current-thread can never actually + // preempt across the await points here, so it only proves the logic is + // right, not that it holds under real thread interleaving. The + // multi-thread variant is the one that could catch a regression of the + // TOCTOU fix in `get` (the `started` timestamp threaded out of the lock). + async fn live_write_resolves_waiters_and_beats_the_late_probe( + cache: std::sync::Arc, + ) { let reader = { let cache = cache.clone(); tokio::spawn(async move { @@ -419,6 +446,24 @@ mod tests { assert_eq!(after, vec![cmd("from-session")], "late probe discarded"); } + #[tokio::test] + async fn a_live_write_resolves_waiters_and_beats_the_late_probe() { + live_write_resolves_waiters_and_beats_the_late_probe(std::sync::Arc::new( + CommandCache::new(), + )) + .await; + } + + // Same test, real OS-thread preemption: this is the flavor that could + // actually observe the TOCTOU window a current-thread runtime cannot. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn a_live_write_resolves_waiters_and_beats_the_late_probe_multi_thread() { + live_write_resolves_waiters_and_beats_the_late_probe(std::sync::Arc::new( + CommandCache::new(), + )) + .await; + } + #[tokio::test] async fn eviction_bounds_the_map_and_spares_in_flight_entries() { let cache = std::sync::Arc::new(CommandCache::new()); From 9f022d7c7879f5f11e8548dc41077163490b4591 Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 14:50:09 +0400 Subject: [PATCH 10/17] engine: serve ListCommands per workspace The RPC carries a cwd and reads the cache; a running session writes its own list into it, so an active chat never pays for a probe. --- crates/doc/src/parts.rs | 4 +- crates/engine/src/rpc.rs | 31 +++++++--- crates/engine/src/sessions.rs | 17 ++++++ crates/engine/tests/command_cache.rs | 88 ++++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 9 deletions(-) create mode 100644 crates/engine/tests/command_cache.rs diff --git a/crates/doc/src/parts.rs b/crates/doc/src/parts.rs index 74a30b50d..3afc9d697 100644 --- a/crates/doc/src/parts.rs +++ b/crates/doc/src/parts.rs @@ -336,8 +336,8 @@ pub fn fold_event_into_parts(out: &mut Vec, event: &AgentEvent) { }); } } - // AvailableCommands feeds the engine's per-harness command cache, not - // the transcript. + // AvailableCommands feeds the engine's per-workspace command cache + // (`engine::commands`), not the transcript. AgentEvent::AssistantMessageCompleted { .. } | AgentEvent::Usage { .. } | AgentEvent::AvailableCommands { .. } => {} diff --git a/crates/engine/src/rpc.rs b/crates/engine/src/rpc.rs index 151ca34c9..100d1eded 100644 --- a/crates/engine/src/rpc.rs +++ b/crates/engine/src/rpc.rs @@ -86,6 +86,16 @@ struct ListModelsParams { harness: HarnessId, } +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct ListCommandsParams { + harness: HarnessId, + /// A path on the HOST device. Absent means the host's home directory, + /// which is what an engine older than this field always answered. + #[serde(default)] + cwd: Option, +} + #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] struct SetHarnessEnabledParams { @@ -1021,18 +1031,25 @@ impl RpcService for EngineRpc { RpcReply::value(&models) } methods::LIST_COMMANDS => { - // Same shape as ListModels: forces a lazy resolve, then the - // harness's own (cached) discovery. Non-ACP harnesses return - // an empty list from the trait default. - let p: ListModelsParams = parse_params(params)?; + // Commands are per workspace, not per harness: project skills + // exist only for a session opened in the project. The cache + // absorbs the cost — a probe spawns an agent process. + let p: ListCommandsParams = parse_params(params)?; let harness = self .registry .resolve(p.harness) .map_err(|e| RpcError::Failed(e.to_string()))?; - let commands = harness - .commands(None) + let commands = self + .sessions + .command_cache() + .get(p.harness, p.cwd.as_deref(), |cwd| async move { + harness + .commands(Some(&cwd)) + .await + .map_err(|e| e.to_string()) + }) .await - .map_err(|e| RpcError::Failed(e.to_string()))?; + .map_err(RpcError::Failed)?; RpcReply::value(&commands) } methods::QUEUE_COMMAND => { diff --git a/crates/engine/src/sessions.rs b/crates/engine/src/sessions.rs index e8f3fe44b..4e00ad33d 100644 --- a/crates/engine/src/sessions.rs +++ b/crates/engine/src/sessions.rs @@ -98,6 +98,9 @@ struct Inner { device_id: String, journal: Arc, registry: Arc, + /// Slash commands per workspace. Fed by discovery probes (via the RPC) and + /// by any running session's own `AvailableCommands`. + commands: Arc, /// Set-once (first wins), cleared on runtime retirement: sessions and /// doc-host reference each other through Arcs, so this back-edge must be /// severable for a replaced engine graph to drop. @@ -148,6 +151,7 @@ impl SessionsEngine { device_id, journal, registry, + commands: Arc::new(crate::commands::CommandCache::new()), doc_host: Mutex::new(None), runs: Mutex::new(HashMap::new()), hubs: Mutex::new(HashMap::new()), @@ -171,6 +175,11 @@ impl SessionsEngine { } } + /// Shared with the RPC surface: `ListCommands` reads it, runs write it. + pub fn command_cache(&self) -> Arc { + self.inner.commands.clone() + } + /// Sever the doc-host back-edge (runtime retirement; the doc host's /// `shutdown_workers` clears its own sessions edge). Every access site /// already treats a missing doc host as "not wired". @@ -1592,6 +1601,14 @@ async fn drive_run( AgentEvent::InputResolved { .. } => { inner.set_status(&chat_id, SessionStatus::Working, false); } + AgentEvent::AvailableCommands { commands } => { + // `run_cwd` is already home-expanded (dispatch does it at the + // top), which is the same normalization the cache applies to a + // `ListCommands` cwd — so both writers land on one key. + inner + .commands + .note_live(harness_id, &run_cwd, commands.clone()); + } _ => {} } diff --git a/crates/engine/tests/command_cache.rs b/crates/engine/tests/command_cache.rs new file mode 100644 index 000000000..c4ccfffab --- /dev/null +++ b/crates/engine/tests/command_cache.rs @@ -0,0 +1,88 @@ +//! A running session feeds the command cache, so an active chat never pays for +//! a discovery probe. + +use std::sync::Arc; + +use zeron_engine::{EngineCore, HarnessRegistry}; +use zeron_harness::Harness; +use zeron_harness::mock::MockHarness; +use zeron_proto::{AgentEvent, DoneStatus, HarnessId, RunRequest, SandboxLevel, SlashCommand}; + +const CHAT: &str = "chat-commands"; + +fn registry_with(harness: Arc) -> Arc { + let registry = HarnessRegistry::new(); + registry.register(harness); + Arc::new(registry) +} + +fn script() -> Vec { + vec![ + AgentEvent::SessionStarted { + harness: HarnessId::Mock, + model: "mock-1".into(), + tools: vec![], + cwd: "/tmp/project".into(), + session_id: "hs-1".into(), + assistant_message_id: "a-1".into(), + }, + AgentEvent::AvailableCommands { + commands: vec![SlashCommand { + name: "ask-matt".into(), + description: "A project skill".into(), + input_hint: None, + }], + }, + AgentEvent::Done { + status: DoneStatus::Completed, + result: None, + error: None, + session_id: Some("hs-1".into()), + }, + ] +} + +#[tokio::test] +async fn a_running_session_fills_the_command_cache() { + let dir = tempfile::tempdir().expect("tempdir"); + let harness: Arc = Arc::new(MockHarness { script: script() }); + let core = EngineCore::assemble(dir.path(), registry_with(harness), HarnessId::Mock, None) + .expect("engine core assembles"); + + let request = RunRequest { + prompt: "hi".into(), + harness: None, + model: None, + reasoning: None, + model_options: Default::default(), + cwd: "/tmp/project".into(), + sandbox: SandboxLevel::WorkspaceWrite, + auto_approve: true, + attachments: Vec::new(), + resume: None, + }; + core.sessions + .dispatch(CHAT, HarnessId::Mock, request, None) + .await + .expect("dispatch"); + + let cache = core.sessions.command_cache(); + let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(10); + loop { + let cached = cache + .get(HarnessId::Mock, Some("/tmp/project"), |_| async { + Err::, String>("probe".into()) + }) + .await; + if let Ok(commands) = cached { + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "ask-matt"); + break; + } + assert!( + tokio::time::Instant::now() < deadline, + "the run never fed the cache" + ); + tokio::time::sleep(std::time::Duration::from_millis(15)).await; + } +} From bf1d93adf8a261b898d24dd2dde69f2e762c6cf9 Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 15:04:13 +0400 Subject: [PATCH 11/17] ui: ask for the project's slash commands The popup keys its cache by harness, device, and cwd, and sends the cwd with ListCommands. A cached list renders at once while the request revalidates. --- crates/ui/src/composer.rs | 177 ++++++++++++++++++++++++++++++-------- 1 file changed, 141 insertions(+), 36 deletions(-) diff --git a/crates/ui/src/composer.rs b/crates/ui/src/composer.rs index ca27a9d77..1246a355a 100644 --- a/crates/ui/src/composer.rs +++ b/crates/ui/src/composer.rs @@ -3218,17 +3218,55 @@ fn slash_token(text: &str, cursor: usize) -> Option { }) } +/// Cache identity for one command list. The device belongs in the key because +/// every project-less chat, on every device, shares the path `~`. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +struct SlashCacheKey { + harness: HarnessId, + device: Option, + cwd: String, +} + +fn slash_cache_key(harness: HarnessId, device: Option<&str>, cwd: &str) -> SlashCacheKey { + SlashCacheKey { + harness, + device: device.map(str::to_string), + cwd: cwd.to_string(), + } +} + +/// Where the popup's commands come from: the chat's own directory, else the +/// picked project's folder, else the host's home. Mirrors the send path's rule +/// (`queue_send`), minus the checkout plan — a fresh worktree has no directory +/// yet when the popup opens, and a worktree of the same repo carries the same +/// tracked skills anyway. +fn slash_cwd(chat: Option<&zeron_proto::Chat>, space: Option<&zeron_proto::Space>) -> String { + if let Some(chat) = chat { + return chat + .cwd + .clone() + .filter(|c| !c.trim().is_empty()) + .unwrap_or_else(|| "~".to_string()); + } + space + .map(|s| s.path.clone()) + .filter(|p| !p.trim().is_empty()) + .unwrap_or_else(|| "~".to_string()) +} + /// Slash-command completion state: like [`FileMentionState`] but the -/// candidate list is fetched once per harness (`ListCommands`) and filtered -/// locally per keystroke — no RPC, debounce, or skeleton churn while typing. +/// candidate list is filtered locally per keystroke. Every edit still +/// revalidates via `ListCommands` — no debounce or skeleton churn, because +/// the engine's own cache (per harness + cwd) decides whether that costs a +/// probe or just a cache hit. #[derive(Debug, Clone, Default)] struct SlashState { token: Option, /// Indices into the cached command list, filter-ranked for the query. filtered: Vec, active: Option, - /// Harness the popup is showing commands for (cache key). - harness: Option, + /// Harness + device + cwd the popup is showing commands for (cache key). + key: Option, request: u64, loading: bool, error: Option, @@ -3309,9 +3347,10 @@ pub struct Composer { mention: FileMentionState, slash_task: Option>, slash: SlashState, - /// Advertised commands per harness (one `ListCommands` per harness per - /// composer lifetime; the engine caches discovery on its side too). - slash_cache: HashMap>, + /// Advertised commands per (harness, device, cwd): stale-while-revalidate, + /// no TTL of its own — the engine owns expiry and decides whether a + /// `ListCommands` costs a real probe. + slash_cache: HashMap>, current_key: String, sending: bool, failure: Option, @@ -3996,13 +4035,23 @@ impl Composer { } self.slash.dismissed = None; let harness = self.pickers.read(cx).resolved(cx).harness; - let harness_changed = self.slash.harness != harness; - if token == self.slash.token && !harness_changed { + let (cwd, device) = { + let state = self.state.read(cx); + let chat = state.selected_chat_row(); + let space = state.selected_space_row(); + let device = chat + .map(|c| c.device_id.clone()) + .or_else(|| space.map(|s| s.device_id.clone())); + (slash_cwd(chat, space), device) + }; + let key = harness.map(|h| slash_cache_key(h, device.as_deref(), &cwd)); + let key_changed = self.slash.key != key; + if token == self.slash.token && !key_changed { self.refilter_slash(cx); return; } self.slash.token = token.clone(); - self.slash.harness = harness; + self.slash.key = key.clone(); self.slash.error = None; if token.is_none() { self.slash.active = None; @@ -4010,36 +4059,26 @@ impl Composer { return; } // No resolved harness (catalog still loading): empty popup, no fetch. - let Some(harness) = harness else { + let Some(key) = key else { self.slash.loading = false; self.refilter_slash(cx); return; }; - if self.slash_cache.contains_key(&harness) { - self.slash.loading = false; - self.refilter_slash(cx); - return; - } - // First open for this harness: one ListCommands, targeted like file - // search (the chat/space host device owns the agent binary). + // Stale while revalidate: a cached list renders instantly with no + // spinner, and the request below refreshes it. The engine owns expiry, + // so the popup never has to guess when a skill was installed. + let cached = self.slash_cache.contains_key(&key); self.slash.request = self.slash.request.wrapping_add(1); - self.slash.loading = true; + self.slash.loading = !cached; self.refilter_slash(cx); let Some(engine) = self.state.read(cx).engine().cloned() else { self.slash.loading = false; return; }; - let target = { - let state = self.state.read(cx); - state - .selected_chat_row() - .map(|chat| chat.device_id.clone()) - .or_else(|| state.selected_space_row().map(|s| s.device_id.clone())) - }; let request = self.slash.request; self.slash_task = Some(cx.spawn(async move |this, cx| { - let mut params = serde_json::json!({ "harness": harness }); - if let (Some(target), Some(object)) = (&target, params.as_object_mut()) { + let mut params = serde_json::json!({ "harness": key.harness, "cwd": key.cwd }); + if let (Some(target), Some(object)) = (&key.device, params.as_object_mut()) { object.insert("targetDeviceId".into(), target.clone().into()); } let result = engine.client().call(methods::LIST_COMMANDS, params).await; @@ -4051,7 +4090,7 @@ impl Composer { match result { Ok(value) => match serde_json::from_value::>(value) { Ok(commands) => { - composer.slash_cache.insert(harness, commands); + composer.slash_cache.insert(key.clone(), commands); } Err(err) => tracing::warn!(%err, "slash command decode failed"), }, @@ -4077,8 +4116,9 @@ impl Composer { .unwrap_or_default(); let commands = self .slash - .harness - .and_then(|h| self.slash_cache.get(&h)) + .key + .as_ref() + .and_then(|k| self.slash_cache.get(k)) .map(Vec::as_slice) .unwrap_or_default(); let names: Vec<&str> = commands.iter().map(|c| c.name.as_str()).collect(); @@ -4117,8 +4157,9 @@ impl Composer { .and_then(|active| self.slash.filtered.get(active)) .and_then(|&ix| { self.slash - .harness - .and_then(|h| self.slash_cache.get(&h)) + .key + .as_ref() + .and_then(|k| self.slash_cache.get(k)) .and_then(|c| c.get(ix)) }) .cloned() @@ -4139,7 +4180,7 @@ impl Composer { self.slash = SlashState { request, dismissed, - harness: self.slash.harness, + key: self.slash.key.clone(), ..SlashState::default() }; self.sync_mention_controls(cx); @@ -4153,8 +4194,9 @@ impl Composer { let token = self.slash.token.as_ref()?; let commands = self .slash - .harness - .and_then(|h| self.slash_cache.get(&h)) + .key + .as_ref() + .and_then(|k| self.slash_cache.get(k)) .map(Vec::as_slice) .unwrap_or_default(); let mut card = crate::popover::popover_card(theme) @@ -6414,4 +6456,67 @@ mod tests { assert!(input_request_resolved(&t, "r1")); assert!(!input_request_resolved(&t, "other")); } + + // `Chat` and `Space` derive no Default, so these build full literals. + fn chat_row(cwd: Option<&str>) -> zeron_proto::Chat { + zeron_proto::Chat { + id: "c1".into(), + device_id: "dev-a".into(), + title: None, + archived: false, + cwd: cwd.map(str::to_string), + branch: None, + checkout_id: None, + config: None, + last_message_preview: None, + last_message_at: None, + created_at: chrono::Utc::now(), + harness_session_id: None, + harness_session_cwd: None, + space_id: None, + last_seen_at: None, + room_gen: None, + } + } + + fn space_row(path: &str) -> zeron_proto::Space { + zeron_proto::Space { + id: "s1".into(), + device_id: "dev-a".into(), + path: path.into(), + name: None, + git_detected: false, + git_checked_at: None, + checkout_id: None, + created_at: chrono::Utc::now(), + } + } + + #[test] + fn slash_cwd_prefers_the_chats_own_directory() { + assert_eq!(slash_cwd(Some(&chat_row(Some("/repo"))), None), "/repo"); + } + + #[test] + fn slash_cwd_falls_back_to_home_when_the_chat_has_none() { + // `Chat::cwd` is optional; a project-less chat runs from the host home. + assert_eq!(slash_cwd(Some(&chat_row(None)), None), "~"); + } + + #[test] + fn slash_cwd_uses_the_space_for_a_new_chat() { + assert_eq!(slash_cwd(None, Some(&space_row("/space"))), "/space"); + } + + #[test] + fn slash_cwd_is_home_without_a_chat_or_a_space() { + assert_eq!(slash_cwd(None, None), "~"); + } + + #[test] + fn the_cache_key_separates_devices_sharing_one_path() { + let a = slash_cache_key(HarnessId::ClaudeCode, Some("dev-a"), "~"); + let b = slash_cache_key(HarnessId::ClaudeCode, Some("dev-b"), "~"); + assert_ne!(a, b, "every project-less chat shares the path `~`"); + } } From 7f55d3d9cd27b0ca69b814451212e315447d6a24 Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 15:07:32 +0400 Subject: [PATCH 12/17] ui: fetch slash commands only on popup open or workspace change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MentionToken carries the query, so gating the ListCommands fetch on token equality alone fired one request per keystroke once a slash token was open — a network round trip per character for a chat hosted on another device. Gate on opening the popup or the cache key changing instead; every other edit refilters the already-cached list locally, which is free. --- crates/ui/src/composer.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/crates/ui/src/composer.rs b/crates/ui/src/composer.rs index 1246a355a..bef704c49 100644 --- a/crates/ui/src/composer.rs +++ b/crates/ui/src/composer.rs @@ -3255,10 +3255,9 @@ fn slash_cwd(chat: Option<&zeron_proto::Chat>, space: Option<&zeron_proto::Space } /// Slash-command completion state: like [`FileMentionState`] but the -/// candidate list is filtered locally per keystroke. Every edit still -/// revalidates via `ListCommands` — no debounce or skeleton churn, because -/// the engine's own cache (per harness + cwd) decides whether that costs a -/// probe or just a cache hit. +/// candidate list is fetched once per popup open (or when the workspace +/// changes underneath it) via `ListCommands`, and filtered locally on every +/// other keystroke — no RPC, debounce, or skeleton churn while typing. #[derive(Debug, Clone, Default)] struct SlashState { token: Option, @@ -4050,6 +4049,9 @@ impl Composer { self.refilter_slash(cx); return; } + // Captured before the token below is overwritten: was the popup + // closed (no token yet) prior to this edit, i.e. is this the open? + let opening = self.slash.token.is_none(); self.slash.token = token.clone(); self.slash.key = key.clone(); self.slash.error = None; @@ -4064,6 +4066,15 @@ impl Composer { self.refilter_slash(cx); return; }; + // Fetch only on open or when the workspace (cache key) changes. The + // token carries the query, so gating on token equality alone would + // issue a `ListCommands` on every keystroke — the list is already + // cached and narrowing it to the query is a local, free filter. + if !opening && !key_changed { + self.slash.loading = false; + self.refilter_slash(cx); + return; + } // Stale while revalidate: a cached list renders instantly with no // spinner, and the request below refreshes it. The engine owns expiry, // so the popup never has to guess when a skill was installed. From 0d277fbbcf1f654247f01bc2e7591c30cc19394d Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 15:16:22 +0400 Subject: [PATCH 13/17] ui: don't clear the loading flag before an in-flight slash fetch lands The gate that skips a refetch on ordinary keystrokes also cleared `loading`, racing the pending request's own completion handler. While a fetch was in flight (opening the popup against an uncached workspace) the next keystroke flipped `loading` to false early, so the popup rendered "no commands" for the whole round trip before self-correcting when the response arrived. `loading` needs no touch here: true belongs to the in-flight request, false already holds when nothing is in flight. --- crates/ui/src/composer.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/ui/src/composer.rs b/crates/ui/src/composer.rs index bef704c49..09c816c87 100644 --- a/crates/ui/src/composer.rs +++ b/crates/ui/src/composer.rs @@ -4070,8 +4070,11 @@ impl Composer { // token carries the query, so gating on token equality alone would // issue a `ListCommands` on every keystroke — the list is already // cached and narrowing it to the query is a local, free filter. + // `loading` is untouched here: with a fetch in flight it is already + // `true` and belongs to that request's own completion handler, which + // clears it when the response lands; without one it is already + // `false` from the `!cached` computation below. if !opening && !key_changed { - self.slash.loading = false; self.refilter_slash(cx); return; } From 12113572eb86a2d2a762d948edf1bb3c0620051b Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 15:29:49 +0400 Subject: [PATCH 14/17] ui: don't let a keystroke silently swallow a slash-discovery error The error field cleared on every token change, but the no-fetch gate skips issuing a new request on ordinary keystrokes. One character typed after a failed discovery cleared the error with nothing in flight to replace it, so the popup fell through to "This agent has no slash commands" for the rest of the engine's negative-cache window instead of showing the real failure. Clear the error only where a fetch is actually issued (or explicitly ruled out by an unresolved harness), not on every token change. --- crates/ui/src/composer.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/crates/ui/src/composer.rs b/crates/ui/src/composer.rs index 09c816c87..3b07859e4 100644 --- a/crates/ui/src/composer.rs +++ b/crates/ui/src/composer.rs @@ -4054,14 +4054,16 @@ impl Composer { let opening = self.slash.token.is_none(); self.slash.token = token.clone(); self.slash.key = key.clone(); - self.slash.error = None; if token.is_none() { self.slash.active = None; self.sync_mention_controls(cx); return; } // No resolved harness (catalog still loading): empty popup, no fetch. + // Clear a stale error here too — an unresolved harness must not show + // the previous workspace's failure message. let Some(key) = key else { + self.slash.error = None; self.slash.loading = false; self.refilter_slash(cx); return; @@ -4081,6 +4083,12 @@ impl Composer { // Stale while revalidate: a cached list renders instantly with no // spinner, and the request below refreshes it. The engine owns expiry, // so the popup never has to guess when a skill was installed. + // `error` clears here, not on every token change: a fetch is about to + // be issued, so a previous failure is either about to be superseded + // or about to be reissued — either way it must not linger through + // keystrokes that don't refetch and silently downgrade to "no + // commands". + self.slash.error = None; let cached = self.slash_cache.contains_key(&key); self.slash.request = self.slash.request.wrapping_add(1); self.slash.loading = !cached; From 7b7c690ee72b094a8952595f4a5ef3a82e99a901 Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 15:29:53 +0400 Subject: [PATCH 15/17] harness: don't let an empty commands update erase the initialize list The discovery probe always opens a session now that a cwd is available, so an agent that advertises built-ins at initialize and then sends an empty available_commands_update for a skill-less project would lose them. Align the probe loop with capture_available_commands on the run path: only overwrite the discovered list when the parsed update is non-empty. --- crates/harness/src/acp/mod.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/harness/src/acp/mod.rs b/crates/harness/src/acp/mod.rs index f5ddb3b3f..da481c36f 100644 --- a/crates/harness/src/acp/mod.rs +++ b/crates/harness/src/acp/mod.rs @@ -813,7 +813,14 @@ impl AcpHarness { if update.get("sessionUpdate").and_then(Value::as_str) == Some("available_commands_update") { - commands = parse_commands(update.get("availableCommands")); + // Mirrors `capture_available_commands` on the run + // path: an empty update (skill-less project) must + // not erase the built-ins the initialize list + // already gave us. + let parsed = parse_commands(update.get("availableCommands")); + if !parsed.is_empty() { + commands = parsed; + } break; } } From 5128782713ae02ed821508e58bfcab1b2053b144 Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Sun, 16 Aug 2026 15:29:55 +0400 Subject: [PATCH 16/17] docs: mark slash-commands spec implemented, not yet merged The design landed on slash-commands-per-workspace across six tasks; the status line still read PLANNED. --- docs/slash-commands.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/slash-commands.md b/docs/slash-commands.md index 3c682d9e9..515c25d37 100644 --- a/docs/slash-commands.md +++ b/docs/slash-commands.md @@ -1,6 +1,6 @@ # Slash commands: per-workspace discovery -Status: PLANNED · 2026-08-16 investigation (project skills missing from the composer popup). +Status: IMPLEMENTED on `slash-commands-per-workspace`, not yet merged · 2026-08-16 investigation (project skills missing from the composer popup). ## Why From be9c732226794fd73d881a09e971ad32eefd6b33 Mon Sep 17 00:00:00 2001 From: Beka Demuradze Date: Wed, 19 Aug 2026 09:57:54 +0400 Subject: [PATCH 17/17] harness: scope claude's slash discovery to the workspace Upstream #160 gave claude a native command probe, and #136 had already moved claude off ACP. So the per-workspace work on this branch reached every agent EXCEPT the one the bug was reported against: project skills still never appeared for claude-code. The claude CLI resolves /.claude/skills relative to where the process runs, and has no session/new field to carry a cwd instead. The probe never set current_dir, so every workspace got the engine's own directory. Measured on 2.1.228 against a project holding two marker skills: 81 commands from the project, 79 from a bare directory, and the two markers are the difference. Through zeron, both read 79. - discover_commands takes the cwd and sets current_dir. This is the opposite of the ACP probe, which must NOT set it, and for the opposite reason: ACP carries the cwd in session/new, claude has nowhere to put it but the process. - An absent directory is dropped rather than passed on. spawn would fail it NotFound, which this driver maps to NotInstalled -- "claude is not installed", a lie the user cannot act on. The fallback loses the project's commands and keeps the built-ins, the same trade the ACP probe makes on a rejected cwd. - #160's per-harness OnceCell is deleted. It would pin whichever workspace probed first and serve that list to every other one. The engine's (harness, cwd) cache owns caching here too. codex is deliberately left per-harness: its skills/list already answers with per-cwd groups that parse_skill_commands flattens, so scoping it means filtering those groups, not setting current_dir, and the group's cwd field could not be confirmed without the real binary. Verified against a real engine over IPC, same params the composer sends: project 81 commands (both markers), bare 79, deleted worktree 79 with no error, repeat call 0ms from cache. --- crates/harness/src/claude/mod.rs | 45 +++++++++++++------- crates/harness/src/codex/mod.rs | 12 ++++-- crates/harness/tests/claude.rs | 45 +++++++++++++++++--- crates/harness/tests/fixtures/fake-claude.sh | 11 ++++- docs/slash-commands.md | 45 ++++++++++++++++---- 5 files changed, 124 insertions(+), 34 deletions(-) diff --git a/crates/harness/src/claude/mod.rs b/crates/harness/src/claude/mod.rs index d5c84c6d5..fcc5d4f6c 100644 --- a/crates/harness/src/claude/mod.rs +++ b/crates/harness/src/claude/mod.rs @@ -119,9 +119,6 @@ pub struct ClaudeHarness { interrupt_grace: Duration, /// Grace between SIGTERM and SIGKILL. kill_grace: Duration, - /// Command discovery cache: only a successful probe is cached, so a - /// broken CLI retries on the next picker open (ACP-harness parity). - commands: tokio::sync::OnceCell>, } impl Default for ClaudeHarness { @@ -130,7 +127,6 @@ impl Default for ClaudeHarness { executable: None, interrupt_grace: Duration::from_secs(2), kill_grace: Duration::from_secs(3), - commands: tokio::sync::OnceCell::new(), } } } @@ -250,10 +246,30 @@ impl ClaudeHarness { /// control_response. No user message is ever written, so no turn (and no /// API call) happens; the child is torn down as soon as the response /// lands. - async fn discover_commands(&self) -> Result, HarnessError> { + /// + /// Unlike the ACP path, the workspace here IS the child's working + /// directory: the CLI resolves `/.claude/skills` and the project's + /// own commands relative to where it runs, and it has no `session/new` + /// field to carry a cwd instead. Measured on 2.1.228 against a project + /// holding two marker skills: 81 commands from the project, 79 from a bare + /// directory, and the two markers are the difference. + /// + /// A directory that no longer exists (a deleted worktree) is dropped + /// rather than passed on. `spawn` would fail it with `NotFound`, which the + /// arm below reports as `NotInstalled` — "claude is not installed", a lie + /// the user cannot act on. Falling back to the engine's own directory + /// costs the project's commands and keeps the built-ins, which is the same + /// trade the ACP probe makes when a cwd is rejected. + async fn discover_commands( + &self, + cwd: Option<&str>, + ) -> Result, HarnessError> { let exe = self.resolve_executable()?; let mut cmd = Command::new(&exe); crate::compose_child_path(&mut cmd, &exe); + if let Some(dir) = cwd.filter(|d| std::path::Path::new(d).is_dir()) { + cmd.current_dir(dir); + } cmd.args([ "--print", "--input-format", @@ -400,19 +416,16 @@ impl Harness for ClaudeHarness { /// Slash commands from the CLI's `initialize` control-request handshake — /// the same channel the Claude Agent SDK's `query()` opens. The response /// carries every command with description + argument hint and involves no - /// model turn (verified live, 2.1.228: the control_response is the first - /// stdout line, well before any API traffic). Cached on success. + /// model turn (verified live, 2.1.228: the control_response arrives on + /// stdout well before any API traffic), scoped to `cwd` — see + /// [`Self::discover_commands`]. /// - /// The `cwd` this fork's trait carries is accepted and ignored here. Wiring - /// project-scoped discovery into the native drivers is a feature, not a - /// merge resolution — the ACP path is the only one that scopes per - /// workspace today. + /// No cache lives here. #160 kept a per-harness `OnceCell`, which would now + /// pin whichever workspace probed first and serve its list to every other + /// one. The engine's `(harness, cwd)` cache owns the caching contract + /// instead: TTL, negative TTL, and single-flight, same as the ACP path. async fn commands(&self, cwd: Option<&str>) -> Result, HarnessError> { - let _ = cwd; - self.commands - .get_or_try_init(|| self.discover_commands()) - .await - .cloned() + self.discover_commands(cwd).await } async fn run( diff --git a/crates/harness/src/codex/mod.rs b/crates/harness/src/codex/mod.rs index 27a4b3b0a..817a03ffa 100644 --- a/crates/harness/src/codex/mod.rs +++ b/crates/harness/src/codex/mod.rs @@ -308,10 +308,14 @@ impl Harness for CodexHarness { /// Skills from a short-lived `skills/list` probe (see /// [`Self::discover_commands`]); cached on success. /// - /// The `cwd` this fork's trait carries is accepted and ignored here. Wiring - /// project-scoped discovery into the native drivers is a feature, not a - /// merge resolution — the ACP path is the only one that scopes per - /// workspace today. + /// The `cwd` is accepted and ignored here, unlike the ACP and claude + /// probes. Scoping codex needs a different change, not the same one: + /// `skills/list` already answers with per-cwd GROUPS under `data`, which + /// [`parse_skill_commands`] flattens and dedupes, so the fix is to filter + /// those groups by the requested workspace rather than to set + /// `current_dir`. The group's own cwd field could not be confirmed without + /// the real binary, so this stays per-harness for now. The engine's + /// `(harness, cwd)` cache keys the answer per workspace regardless. async fn commands(&self, cwd: Option<&str>) -> Result, HarnessError> { let _ = cwd; self.commands diff --git a/crates/harness/tests/claude.rs b/crates/harness/tests/claude.rs index f6ef47003..09edae93b 100644 --- a/crates/harness/tests/claude.rs +++ b/crates/harness/tests/claude.rs @@ -635,11 +635,46 @@ async fn commands_come_from_the_initialize_control_request() { assert_eq!(commands[1].name, "compact"); assert_eq!(commands[1].input_hint, None, "empty hint reads as None"); - // Cached: the second call reuses the first probe's result (the fake has - // exited; a re-probe against a dead binary path would still work here, - // but object identity of the cached list is the cheap assertion). - let again = h.commands(None).await.expect("cache hit"); - assert_eq!(again, commands); + // No cwd asked for, so the marker project's extra command cannot appear. + assert!( + !commands.iter().any(|c| c.name == "project-skill"), + "{commands:?}" + ); +} + +/// The probe must RUN in the requested workspace: the claude CLI resolves +/// `/.claude/skills` relative to its own working directory, and has no +/// protocol field to carry a cwd instead. +#[tokio::test] +async fn commands_discovery_runs_in_the_requested_cwd() { + let dir = std::env::temp_dir().join("zeron-marker-project"); + std::fs::create_dir_all(&dir).expect("create marker project"); + let h = harness(); + let commands = h + .commands(Some(dir.to_str().expect("utf8 path"))) + .await + .expect("discovery"); + // Pinned by name, not by count: a bare length assertion would still pass + // if the fixture were run from the wrong directory and happened to grow. + assert!( + commands.iter().any(|c| c.name == "project-skill"), + "the marker project's command is missing, so the child ran elsewhere: {commands:?}" + ); + // The workspace adds to the built-ins, it does not replace them. + assert!(commands.iter().any(|c| c.name == "review"), "{commands:?}"); +} + +/// A deleted worktree must not read as "claude is not installed". `spawn` with +/// a missing `current_dir` fails `NotFound`, which the driver otherwise maps to +/// `NotInstalled` — so the probe drops an absent directory and still answers. +#[tokio::test] +async fn a_missing_cwd_falls_back_instead_of_reporting_a_missing_cli() { + let h = harness(); + let commands = h + .commands(Some("/tmp/zeron-deleted-worktree-does-not-exist")) + .await + .expect("a deleted worktree still lists the built-ins"); + assert!(commands.iter().any(|c| c.name == "review"), "{commands:?}"); } /// Live smoke against the real CLI: `cargo test -p zeron-harness --test diff --git a/crates/harness/tests/fixtures/fake-claude.sh b/crates/harness/tests/fixtures/fake-claude.sh index af27e5e63..fad2175f0 100755 --- a/crates/harness/tests/fixtures/fake-claude.sh +++ b/crates/harness/tests/fixtures/fake-claude.sh @@ -106,7 +106,16 @@ case "$first" in # stdin line (no user message ever follows). Shape mirrors 2.1.228's # control_response: commands under response.response. rid=$(printf '%s\n' "$first" | sed 's/.*"request_id":"\([^"]*\)".*/\1/') - emit "{\"type\":\"control_response\",\"response\":{\"subtype\":\"success\",\"request_id\":\"$rid\",\"response\":{\"commands\":[{\"name\":\"review\",\"description\":\"Review a pull request\",\"argumentHint\":\"[pr number]\"},{\"name\":\"compact\",\"description\":\"Compact the conversation\",\"argumentHint\":\"\"},{\"name\":\"\",\"description\":\"nameless: dropped\"}],\"output_style\":\"default\"}}}" + # The real CLI resolves /.claude/skills relative to where it RUNS, so + # the working directory is the only signal that a workspace was requested. + # A project-marker directory adds one command no other cwd can produce. + project="" + case "$(basename "$PWD")" in + zeron-marker-project) + project=',{"name":"project-skill","description":"Only in the marker project","argumentHint":""}' + ;; + esac + emit "{\"type\":\"control_response\",\"response\":{\"subtype\":\"success\",\"request_id\":\"$rid\",\"response\":{\"commands\":[{\"name\":\"review\",\"description\":\"Review a pull request\",\"argumentHint\":\"[pr number]\"},{\"name\":\"compact\",\"description\":\"Compact the conversation\",\"argumentHint\":\"\"},{\"name\":\"\",\"description\":\"nameless: dropped\"}$project],\"output_style\":\"default\"}}}" # Stay alive until the driver tears us down, like the real CLI would. exec sleep 30 ;; diff --git a/docs/slash-commands.md b/docs/slash-commands.md index b1ceaa317..828ea0900 100644 --- a/docs/slash-commands.md +++ b/docs/slash-commands.md @@ -134,9 +134,35 @@ async fn commands(&self, cwd: Option<&str>) -> Result, Harness one identical list. - `cwd: None` means `$HOME`. That is today's behavior, kept for callers with no workspace. - The trait default still returns an empty list for harnesses whose wire carries no - listing. Since #160 the native `claude` and `codex` drivers override it with their own - per-harness discovery; they accept the `cwd` and ignore it. Scoping a native probe to a - workspace is a feature of its own, not part of this change — see Non-goals. + listing. Since #160 the native `claude` and `codex` drivers override it. `claude` is + scoped here too (below); `codex` is not (see Non-goals). + +### The claude probe + +`claude` is not an ACP agent, so there is no `session/new` to carry a cwd. The CLI resolves +`/.claude/skills` and the project's own commands relative to **where the process +runs**, so for this driver the workspace IS the child's working directory. + +Measured on CLI 2.1.228, driving the same `initialize` control request by hand and varying +only the directory: + +| probe cwd | commands | +|---|---| +| a project holding two marker skills | **81** | +| a bare directory | 79 | + +The two markers are exactly the difference. + +- `discover_commands` takes the cwd and sets `current_dir` — the opposite of the ACP probe, + for the reason above. +- An absent directory (a deleted worktree) is dropped rather than passed on. `spawn` fails + it with `ErrorKind::NotFound`, which this driver maps to `HarnessError::NotInstalled`, so + the user would be told claude is not installed. Falling back to the engine's own + directory loses the project's commands and keeps the built-ins, the same trade the ACP + probe makes when a cwd is rejected. +- #160's per-harness `OnceCell` is deleted. It would pin whichever workspace probed first + and serve that list to every other one. The engine cache owns caching for this driver + too, exactly as it does for ACP. `discover_models` keeps its own `OnceCell` and its `$HOME` cwd. Models are not treated as workspace-scoped in this spec. See Non-goals. @@ -316,11 +342,14 @@ The tests cover that function, next to the existing pure-function tests at - **Models.** They keep the `$HOME` probe. `ListCommandsParams` and the cache key are shaped so models can join later without another interface change. -- **Per-workspace discovery for the native drivers.** #160 gave `claude` (an `initialize` - control request) and `codex` (`skills/list`) their own probes, each cached per harness. - Both now take the `cwd` and ignore it, so the engine cache keys their answer per - workspace while the answer itself is still workspace-blind. Teaching those two wires to - scope by directory is separate work; the interface is already in place for it. +- **Per-workspace discovery for codex.** `claude` is scoped (above). `codex` is not, and it + needs a different change rather than the same one: its `skills/list` reply is already an + array of per-cwd groups under `data`, which `parse_skill_commands` flattens and dedupes. + Scoping it means filtering those groups by the requested workspace, not setting + `current_dir`, and the group's own cwd field could not be confirmed here without the real + binary. `codex` therefore keeps #160's per-harness `OnceCell` and takes the `cwd` and + ignores it, documented in place. The engine cache keys its answer per workspace anyway, + so nothing regresses. - **File watchers on `.claude`.** The TTL plus the live event covers the real workflow. - **Cleaning the 666 stale directories.** Separate chore. - **Changing how an agent resolves skills.** Symlinked skills work correctly once the cwd