From 70bc0bbed5a0a2f9f705f4b119c4081148aa4c08 Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Wed, 29 Jul 2026 16:30:51 +0200 Subject: [PATCH 01/15] docs: design for flow-participant-aware agent commands Protects review and review-flow agents from accidental attach-injection and stop, enforced daemon-side. Read-only attach, stop refuses without --force, stop --all skips and says so. Refs AI-1557, #379. Co-Authored-By: Claude Opus 5 --- ...participant-aware-agent-commands-design.md | 140 ++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-29-ai1557-flow-participant-aware-agent-commands-design.md diff --git a/docs/superpowers/specs/2026-07-29-ai1557-flow-participant-aware-agent-commands-design.md b/docs/superpowers/specs/2026-07-29-ai1557-flow-participant-aware-agent-commands-design.md new file mode 100644 index 0000000000..a208fb7007 --- /dev/null +++ b/docs/superpowers/specs/2026-07-29-ai1557-flow-participant-aware-agent-commands-design.md @@ -0,0 +1,140 @@ +# Flow-participant-aware `kcap agent` commands — design + +Date: 2026-07-29 +Status: proposed +Issue: AI-1557 / [#379](https://github.com/kurrent-io/kcap-cli/issues/379) + +## Motivation + +`kcap agent ls|attach|stop` treats every agent the daemon hosts identically. +`HandleLocalListAsync` returns all of `_agents` unfiltered and `HandleLocalAttachAsync` attaches +to any id in it, so an agent the daemon launched as a **review-flow participant** — a reviewer +mid-round — is indistinguishable from one the user started with `kcap agent start`. + +Two concrete failures follow: + +1. **Attach injects turns into a flow.** Participants are addressed through the flow protocol + (`send_to_participant`), never by typing at them. `kcap agent attach ` hands the + user raw PTY stdin, with no round, no routing, and no record of the interjection. +2. **Stop kills a participant silently.** `kcap agent stop --all` stops every agent in the map. + A reviewer vanishes mid-round and the flow has no idea why. + +The second one arrived with `agent stop`, which is why this was deferred rather than solved in +the same change: the read path was already blind, and the write path made the blindness bite. + +The daemon already has everything needed. `AgentInstance` carries `Kind` +(`LaunchKind.Default`/`Review`/`ReviewFlow`), plus `FlowRunId`/`FlowRole` for flow launches. None +of it reaches the CLI. + +## Goal + +The CLI can tell a review or flow agent from the user's own, shows it, and refuses to mutate it +by accident. + +## Decisions + +1. **The protected set is `Kind != Default`** — `ReviewFlow` participants and `Review` agents. + Plain web-UI-launched agents (`Default`, not locally spawned) stay unprotected: they are the + user's own work by another route, and protecting them would make `stop --all` nearly useless. + +2. **Attach to a protected agent is read-only**, not refused. Watching a reviewer work is a real + debugging need with no other local equivalent, and read-only preserves the flow invariant + exactly — output flows out, nothing flows in. + +3. **A read-only viewer does not resize the PTY.** It is never added to + `AgentInstance.ClientDims`, so it cannot shrink a participant's terminal through the + min-clamp. The cost lands on the observer: if their terminal differs from the participant's, + their own view wraps or clips. That is the correct side to put it on. + +4. **`stop` on a protected agent refuses unless `--force`.** `stop --all` stops only unprotected + agents and *states* what it skipped — a silent omission reads as "stopped everything". + +5. **Enforcement is daemon-side, not client-side.** The daemon owns the protection for both + attach and stop, so a stale or hand-rolled client cannot bypass it, and the two subcommands + share one model. This costs a wire change that a client-side filter would not. + +6. **Against an older daemon, no protection applies.** Accepted, not mitigated — see Version + skew. + +## Wire protocol + +`FrameType` is append-only. One new value: + +| Value | Direction | Payload | +|---|---|---| +| `StopV2 = 10` | client → daemon | `mode⇥agentId` — mode `normal`\|`force`; empty id = all eligible | + +Two payload extensions to existing frames: + +- **`AgentList`** rows become `id⇥status⇥repo⇥kind⇥flowRunId⇥flowRole`. `kind` is + `agent`\|`review`\|`review-flow`; the flow fields are empty for non-flow agents. +- **`StopAck`** rows keep the `id⇥status` shape, with `status` gaining `skipped` alongside the + existing `stopped`\|`failed`. +- **`Attached`** gains a trailing read-only flag and reason, appended after the existing payload. + This follows the precedent `FrameCodec.Spawn` already set for `isPrivate` ("APPENDED after + args: older parsers ignore trailing bytes"). + +`Stop = 8` stays decodable so nothing regresses mid-upgrade, but the CLI only sends `StopV2`. + +## Daemon changes + +- `HandleLocalListAsync` emits the three new columns from `Kind`/`FlowRunId`/`FlowRole`. +- `HandleLocalAttachAsync` resolves the agent's `Kind`. For a protected agent it runs + `AttachClientLoopAsync` in read-only mode: the sink is registered as today, the `Stdin` arm is + dropped, the `Resize` arm is dropped, and the client is never entered into `ClientDims`. +- A new `HandleLocalStopV2Async(mode, agentId, …)` applies decision 4: + - explicit id, protected, `normal` → `Error` naming the kind and flow; + - explicit id, protected, `force` → stop; + - empty id, `normal` → stop unprotected agents; protected ones appear in the ack as `skipped`; + - empty id, `force` → stop everything, as `stop --all` does today. +- `LocalControlServer` routes `FrameType.StopV2`. + +The `IsPrivate` guard on `HandleStopAgent` and the server-origin path are untouched. + +## Client changes + +- `AgentRow` gains `Kind`, `FlowRunId`, `FlowRole`. The parser accepts **3 or more** columns and + defaults the new ones, so a running older daemon still lists correctly. +- `ls` renders a `KIND` column, with the flow role appended for flow agents. +- `attach` prints the read-only banner when the daemon reports the flag, and skips its own stdin + pump — cosmetic, since the daemon already drops it. +- `stop` accepts `--force`, sends `StopV2`, and reports `skipped` rows distinctly from + `stopped`. With `--all --force`, the confirmation prompt lists protected agents in their own + labelled group, so the blast radius is visible before the user answers. + +## Version skew + +An older daemon replies with three-column `AgentList` rows, so every agent reads as `agent` and +**no protection engages** — `stop --all` behaves exactly as it does today. + +This is deliberately not mitigated. A three-column reply from an old daemon is indistinguishable +from a new daemon reporting three unprotected agents, so there is nothing to detect without +adding a version probe. The window closes on its own: a daemon self-restarts onto the new binary +when idle after `kcap update`. `StopV2` is the one place skew is visible, and it already +surfaces through the existing "this daemon is too old for `agent stop`" message. + +## Documentation + +- `help-agent.txt` — read-only attach, `stop --force`, and what `stop --all` skips. +- `README.md` — the `### Local agents (kcap agent)` section. Its current `--all` warning + (added when this gap was deferred, pointing at #379) is replaced by the real behaviour. + +## Testing + +- Daemon: `SeedAgentForTest` already takes `kind`/`flowRunId`/`flowRole`, so protection tests are + cheap. Cover — protected attach drops stdin and resize and never enters `ClientDims`; + protected stop without force yields `Error`; with force stops; stop-all marks protected rows + `skipped` and stops the rest. +- Client: `AgentRow` parsing for 3-column (old daemon) and 6-column rows; `ls` rendering; + `skipped` rows reported distinctly. +- `AgentVerbDispatchTests` gains a case pinning that a protected agent refuses `stop` without + `--force`. + +## Out of scope + +- **The `--all` confirmation TOCTOU** — the client lists and prompts, then the daemon + re-enumerates, so an agent launched during the prompt is stopped unlisted. Tracked separately; + this design narrows but does not close it. +- **Telling the flow that a participant was force-stopped.** The flow still sees a participant + vanish; attributing that needs server-side work. +- **Plain web-UI-launched agents**, per decision 1. From bb17cd0a2faeb088f356ad2472769f24d0b24288 Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Wed, 29 Jul 2026 17:06:10 +0200 Subject: [PATCH 02/15] docs: implementation plan for flow-participant-aware agent commands Also corrects the spec's Attached mechanism: its payload ends with an unbounded snapshot, so a trailing flag would be painted onto the terminal rather than parsed. Uses a separate AttachedReadOnly frame instead, which also fails closed against an older client. Refs AI-1557, #379. Co-Authored-By: Claude Opus 5 --- ...7-flow-participant-aware-agent-commands.md | 1030 +++++++++++++++++ ...participant-aware-agent-commands-design.md | 19 +- 2 files changed, 1043 insertions(+), 6 deletions(-) create mode 100644 docs/superpowers/plans/2026-07-29-ai1557-flow-participant-aware-agent-commands.md diff --git a/docs/superpowers/plans/2026-07-29-ai1557-flow-participant-aware-agent-commands.md b/docs/superpowers/plans/2026-07-29-ai1557-flow-participant-aware-agent-commands.md new file mode 100644 index 0000000000..9ef90762fe --- /dev/null +++ b/docs/superpowers/plans/2026-07-29-ai1557-flow-participant-aware-agent-commands.md @@ -0,0 +1,1030 @@ +# Flow-Participant-Aware `kcap agent` Commands Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make `kcap agent ls|attach|stop` recognise review and review-flow agents, show them, attach to them read-only, and refuse to stop them without `--force`. + +**Architecture:** The daemon already knows each agent's `LaunchKind` and flow identity; it just never tells the CLI. This carries that across the local socket (three new `AgentList` columns) and enforces protection **daemon-side** for both attach (a new `AttachedReadOnly` frame plus a no-input attach loop) and stop (a new `StopV2` frame carrying a force flag, plus a `skipped` status in `StopAck`). + +**Tech Stack:** .NET 10, NativeAOT, TUnit on Microsoft Testing Platform, custom length-prefixed binary IPC over a Unix domain socket. + +**Spec:** `docs/superpowers/specs/2026-07-29-ai1557-flow-participant-aware-agent-commands-design.md` + +## Global Constraints + +- **The protected set is `Kind != LaunchKind.Default`** — `Review` and `ReviewFlow`. Plain web-UI-launched agents (`Default`) are deliberately NOT protected. +- **Enforcement is daemon-side.** The CLI may mirror a check for a better message, but the daemon must refuse independently — a stale or hand-rolled client cannot bypass protection. +- **`FrameType` is append-only.** Never renumber or reuse. New values: `StopV2 = 10`, `AttachedReadOnly = 71`. +- **A read-only viewer never resizes the PTY.** It must not be entered into `AgentInstance.ClientDims`, and its `Resize` frames are ignored. +- **Do not touch** the `if (agent.IsPrivate) return;` guard in `HandleStopAgent`, or any server-origin stop behaviour. +- **`AgentRow` parsing accepts 3 or more columns**, defaulting the new ones — a running older daemon must still list correctly. +- Comment style: self-explanatory code over prose; no Linear issue numbers in comments (GitHub numbers like #379 are fine). +- **AOT:** verify with `dotnet publish -c Release`, not `dotnet build`. Note `[]` has no natural type, so `var x = cond ? [] : arr;` will not compile — use an explicit type. +- **Docs land in the same PR**: `README.md` *and* `help-agent.txt`. +- Run tests as executables: `dotnet run --project test//.csproj`. Filter with `--treenode-filter` (glob syntax), **never** `--filter`. +- **Known test baseline:** the unit suite has ~42 pre-existing failures in `CodexHookCommandTests` and the uninstall/`config.toml` area, confirmed at the branch's merge base. The gate is **no NEW failures**; the count wobbles 42-43. + +--- + +### Task 1: Carry and show the agent's kind + +**Files:** +- Modify: `src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs` (`HandleLocalListAsync`) +- Modify: `src/Capacitor.Cli/Commands/AgentCommand.cs` (`AgentRow`, `FetchAgentsAsync`, `ListAsync`) +- Test: `test/Capacitor.Cli.Tests.Unit/AgentIdResolutionTests.cs` +- Test: `test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs` + +**Interfaces:** +- Produces: `internal readonly record struct AgentRow(string Id, string Status, string Repo, string Kind, string FlowRunId, string FlowRole)`; `internal static AgentRow ParseAgentRow(string line)` on `AgentCommand`; `internal static bool IsProtectedKind(string kind)` on `AgentCommand`; `static string KindText(LaunchKind kind)` on `AgentOrchestrator`. +- Consumes: existing `AgentInstance.Kind` / `.FlowRunId` / `.FlowRole`, and `LaunchKind { Default, Review, ReviewFlow }` from `Capacitor.Cli.Core.Models`. + +- [ ] **Step 1: Write the failing CLI parse tests** + +Append to `test/Capacitor.Cli.Tests.Unit/AgentIdResolutionTests.cs`, inside the class: + +```csharp + [Test] + public async Task Row_from_a_current_daemon_carries_kind_and_flow() { + var row = AgentCommand.ParseAgentRow("ab12\tRunning\t/repo\treview-flow\tflow-7f3a\treviewer"); + await Assert.That(row.Id).IsEqualTo("ab12"); + await Assert.That(row.Status).IsEqualTo("Running"); + await Assert.That(row.Repo).IsEqualTo("/repo"); + await Assert.That(row.Kind).IsEqualTo("review-flow"); + await Assert.That(row.FlowRunId).IsEqualTo("flow-7f3a"); + await Assert.That(row.FlowRole).IsEqualTo("reviewer"); + } + + [Test] + public async Task Row_from_an_older_daemon_defaults_to_an_unprotected_agent() { + // An older daemon sends three columns. Treating the missing kind as `agent` is what + // makes the group keep working against it — protection simply does not engage. + var row = AgentCommand.ParseAgentRow("ab12\tRunning\t/repo"); + await Assert.That(row.Kind).IsEqualTo("agent"); + await Assert.That(row.FlowRunId).IsEqualTo(""); + await Assert.That(row.FlowRole).IsEqualTo(""); + await Assert.That(AgentCommand.IsProtectedKind(row.Kind)).IsFalse(); + } + + [Test] + public async Task Review_and_review_flow_are_protected_and_agent_is_not() { + await Assert.That(AgentCommand.IsProtectedKind("review")).IsTrue(); + await Assert.That(AgentCommand.IsProtectedKind("review-flow")).IsTrue(); + await Assert.That(AgentCommand.IsProtectedKind("agent")).IsFalse(); + } +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj --treenode-filter "/*/*/AgentIdResolutionTests/*"` +Expected: FAIL — compile error, `ParseAgentRow` and `IsProtectedKind` do not exist. + +- [ ] **Step 3: Widen `AgentRow` and add the parser** + +In `src/Capacitor.Cli/Commands/AgentCommand.cs`, replace the `AgentRow` declaration: + +```csharp +/// One row of the daemon's agent table (`id\tstatus\trepo\tkind\tflowRunId\tflowRole` on the +/// wire). A daemon older than #379 sends only the first three; the rest default. +internal readonly record struct AgentRow( + string Id, string Status, string Repo, string Kind, string FlowRunId, string FlowRole); +``` + +Then add to the `AgentCommand` class, next to `ResolveAgentId`: + +```csharp + /// Kinds the CLI refuses to mutate by accident: a reviewer mid-round is not the user's to + /// type at or stop. Mirrors LaunchKind — anything that is not a plain agent is protected. + internal static bool IsProtectedKind(string kind) => kind is "review" or "review-flow"; + + /// Tolerates a short row from an older daemon by defaulting the newer columns. + internal static AgentRow ParseAgentRow(string line) { + var p = line.Split('\t'); + + return new AgentRow( + p[0], + p.Length > 1 ? p[1] : "", + p.Length > 2 ? p[2] : "", + p.Length > 3 && p[3].Length > 0 ? p[3] : "agent", + p.Length > 4 ? p[4] : "", + p.Length > 5 ? p[5] : ""); + } +``` + +- [ ] **Step 4: Route `FetchAgentsAsync` through the parser** + +In `FetchAgentsAsync`, replace the projection: + +```csharp + return [.. resp.Text.Split('\n') + .Where(l => l.Length > 0) + .Select(ParseAgentRow)]; +``` + +- [ ] **Step 5: Run the tests to verify they pass** + +Run: `dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj --treenode-filter "/*/*/AgentIdResolutionTests/*"` +Expected: PASS (10 tests). + +- [ ] **Step 6: Write the failing daemon-side list test** + +Append to `test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs`, inside the `AgentOrchestratorVendorTests` partial class: + +```csharp + [Test] + public async Task Local_list_reports_each_agent_kind_and_flow_identity() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("plain-1"); + orch.SeedAgentForTest("rev-1", kind: LaunchKind.Review); + orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + + using var client = new DuplexTestStream(new MemoryStream(), new MemoryStream()); + await orch.HandleLocalListAsync(client, default); + client.WrittenStream.Position = 0; + var reply = await FrameCodec.ReadAsync(client.WrittenStream, default); + + var rows = reply!.Text.Split('\n').Select(l => l.Split('\t')).ToDictionary(p => p[0], p => p); + + await Assert.That(rows["plain-1"][3]).IsEqualTo("agent"); + await Assert.That(rows["rev-1"][3]).IsEqualTo("review"); + await Assert.That(rows["flow-1"][3]).IsEqualTo("review-flow"); + await Assert.That(rows["flow-1"][4]).IsEqualTo("flow-7f3a"); + await Assert.That(rows["flow-1"][5]).IsEqualTo("reviewer"); + } +``` + +- [ ] **Step 7: Run it to verify it fails** + +Run: `dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj --treenode-filter "/*/*/AgentOrchestratorVendorTests/Local_list_reports*"` +Expected: FAIL — the reply has only three columns, so `rows["plain-1"][3]` throws `IndexOutOfRangeException`. + +- [ ] **Step 8: Emit the new columns** + +In `src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs`, replace `HandleLocalListAsync` and add the mapper: + +```csharp + /// Reply to a kcap agent ls request with a tab-separated agent table. + public Task HandleLocalListAsync(Stream stream, CancellationToken ct) { + var lines = _agents.Values.Select(a => + $"{a.Id}\t{a.Status}\t{a.RepoPath}\t{KindText(a.Kind)}\t{a.FlowRunId ?? ""}\t{a.FlowRole ?? ""}"); + + return FrameCodec.WriteAsync(stream, new LocalFrame(FrameType.AgentList) { Text = string.Join('\n', lines) }, ct); + } + + /// Wire spelling of . Kept separate from the enum name so the table + /// reads as a CLI column rather than a .NET identifier. + static string KindText(LaunchKind kind) => kind switch { + LaunchKind.Review => "review", + LaunchKind.ReviewFlow => "review-flow", + _ => "agent", + }; +``` + +- [ ] **Step 9: Run it to verify it passes** + +Run: `dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj --treenode-filter "/*/*/AgentOrchestratorVendorTests/Local_list_reports*"` +Expected: PASS. + +- [ ] **Step 10: Render the KIND column** + +In `AgentCommand.ListAsync`, replace the two output lines: + +```csharp + Console.WriteLine($"{"AGENT",-34} {"STATUS",-10} {"KIND",-12} REPO"); + foreach (var a in agents) { + var role = a.FlowRole.Length > 0 ? $" [{a.FlowRole}]" : ""; + Console.WriteLine($"{a.Id,-34} {a.Status,-10} {a.Kind,-12} {a.Repo}{role}"); + } +``` + +- [ ] **Step 11: Verify the build and the full unit suite** + +```bash +dotnet build src/Capacitor.Cli/Capacitor.Cli.csproj +dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj +``` + +Expected: build succeeds; no new failures beyond the ~42 baseline. + +- [ ] **Step 12: Commit** + +```bash +git add src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs src/Capacitor.Cli/Commands/AgentCommand.cs test/Capacitor.Cli.Tests.Unit/AgentIdResolutionTests.cs test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs +git commit -m "feat(agent): carry and show each agent's launch kind" +``` + +--- + +### Task 2: `StopV2` and `AttachedReadOnly` frames + +**Files:** +- Modify: `src/Capacitor.Cli.Core/LocalIpc/FrameType.cs` +- Modify: `src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs` +- Modify: `src/Capacitor.Cli.Core/LocalIpc/LocalFrame.cs` +- Test: `test/Capacitor.Cli.Tests.Unit/FrameCodecTests.cs` + +**Interfaces:** +- Produces: `FrameType.StopV2 = 10`, `FrameType.AttachedReadOnly = 71`; `LocalFrame.StopV2(bool force, string agentId)`; `FrameCodec.StopV2(LocalFrame f)` returning `(bool force, string agentId)`; `FrameCodec.AttachedReadOnly(string agentId, string reason, byte[] snapshot)` and its decoder `FrameCodec.AttachedReadOnly(LocalFrame f)` returning `(string agentId, string reason, byte[] snapshot)`. + +- [ ] **Step 1: Write the failing round-trip tests** + +Append to `test/Capacitor.Cli.Tests.Unit/FrameCodecTests.cs`, inside the class: + +```csharp + [Test] + [Arguments(true, "ab12")] + [Arguments(false, "ab12")] + [Arguments(false, "")] + public async Task StopV2_round_trips_mode_and_id(bool force, string agentId) { + var r = await RoundTrip(LocalFrame.StopV2(force, agentId)); + await Assert.That(r.Type).IsEqualTo(FrameType.StopV2); + + var (gotForce, gotId) = FrameCodec.StopV2(r); + await Assert.That(gotForce).IsEqualTo(force); + await Assert.That(gotId).IsEqualTo(agentId); + } + + [Test] + public async Task AttachedReadOnly_round_trips_id_reason_and_snapshot() { + var snapshot = new byte[] { 0x1b, 0x5b, 0x41, 0x00, 0xff }; + var r = await RoundTrip(FrameCodec.AttachedReadOnly("ab12", "review-flow reviewer", snapshot)); + await Assert.That(r.Type).IsEqualTo(FrameType.AttachedReadOnly); + + var (id, reason, got) = FrameCodec.AttachedReadOnly(r); + await Assert.That(id).IsEqualTo("ab12"); + await Assert.That(reason).IsEqualTo("review-flow reviewer"); + await Assert.That(got).IsEquivalentTo(snapshot); + } + + [Test] + public async Task AttachedReadOnly_round_trips_an_empty_snapshot() { + var r = await RoundTrip(FrameCodec.AttachedReadOnly("ab12", "review agent", [])); + var (_, _, got) = FrameCodec.AttachedReadOnly(r); + await Assert.That(got).IsEmpty(); + } +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj --treenode-filter "/*/*/FrameCodecTests/*"` +Expected: FAIL — compile error, `LocalFrame.StopV2` and `FrameCodec.AttachedReadOnly` do not exist. + +- [ ] **Step 3: Add the frame types** + +In `src/Capacitor.Cli.Core/LocalIpc/FrameType.cs`, add `StopV2` after `Stop` and `AttachedReadOnly` after `StopAck`: + +```csharp + Stop = 8, // stop an agent (Text = agent id; empty = every agent this daemon hosts) + StopV2 = 10, // stop with a force flag (see FrameCodec.StopV2); supersedes Stop +``` + +```csharp + StopAck = 70, // acknowledgement for Stop (Text = `id\tstatus` per line) + AttachedReadOnly = 71, // Attached for a protected agent: id + reason + snapshot, no input accepted +``` + +- [ ] **Step 4: Teach the codec both frames** + +In `src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs`, add `StopV2` and `AttachedReadOnly` to the pre-encoded arms of `Encode` and `Decode`, alongside `Attached`/`Spawn`: + +```csharp + FrameType.Attached or FrameType.Spawn + or FrameType.StopV2 or FrameType.AttachedReadOnly => f.Bytes, // pre-encoded by the helpers below +``` + +```csharp + FrameType.Attached or FrameType.Spawn + or FrameType.StopV2 or FrameType.AttachedReadOnly => new(t) { Bytes = p }, +``` + +Then add the structured helpers next to the existing `Attached` pair: + +```csharp + // --- StopV2 structured payload --- + public static LocalFrame StopV2(bool force, string agentId) { + using var ms = new MemoryStream(); + ms.WriteByte((byte)(force ? 1 : 0)); + WriteLp(ms, agentId); + return new(FrameType.StopV2) { Bytes = ms.ToArray(), Text = agentId }; + } + public static (bool force, string agentId) StopV2(LocalFrame f) { + var o = 1; + return (f.Bytes[0] == 1, ReadLp(f.Bytes, ref o)); + } + + // --- AttachedReadOnly structured payload --- + // Length-prefixed id AND reason before the snapshot: the snapshot is the unbounded tail, so + // anything appended after it would be painted onto the user's terminal instead of parsed. + public static LocalFrame AttachedReadOnly(string agentId, string reason, byte[] snapshot) { + using var ms = new MemoryStream(); + WriteLp(ms, agentId); + WriteLp(ms, reason); + ms.Write(snapshot); + return new(FrameType.AttachedReadOnly) { Bytes = ms.ToArray(), Text = agentId }; + } + public static (string agentId, string reason, byte[] snapshot) AttachedReadOnly(LocalFrame f) { + var o = 0; + var id = ReadLp(f.Bytes, ref o); + var reason = ReadLp(f.Bytes, ref o); + return (id, reason, f.Bytes[o..]); + } +``` + +- [ ] **Step 5: Add the `LocalFrame` factory** + +In `src/Capacitor.Cli.Core/LocalIpc/LocalFrame.cs`, after the `StopAck` factory: + +```csharp + public static LocalFrame StopV2(bool force, string agentId) => FrameCodec.StopV2(force, agentId); +``` + +- [ ] **Step 6: Run the tests to verify they pass** + +Run: `dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj --treenode-filter "/*/*/FrameCodecTests/*"` +Expected: PASS, including the five new cases. + +- [ ] **Step 7: Commit** + +```bash +git add src/Capacitor.Cli.Core/LocalIpc/ test/Capacitor.Cli.Tests.Unit/FrameCodecTests.cs +git commit -m "feat(ipc): add StopV2 and AttachedReadOnly frames" +``` + +--- + +### Task 3: Daemon-side read-only attach + +**Files:** +- Modify: `src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs` (`HandleLocalAttachAsync`, `AttachClientLoopAsync`) +- Test: `test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs` + +**Interfaces:** +- Consumes: `FrameCodec.AttachedReadOnly(string, string, byte[])` (Task 2); `AgentInstance.Kind` / `.FlowRunId` / `.FlowRole`. +- Produces: `AttachClientLoopAsync(AgentInstance agent, Stream stream, CancellationToken ct, bool readOnly = false)`; `static string ProtectionReason(AgentInstance agent)` on `AgentOrchestrator`. + +- [ ] **Step 1: Write the failing tests** + +Append to `test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs`, inside the `AgentOrchestratorVendorTests` partial class: + +```csharp + [Test] + public async Task Attaching_to_a_flow_participant_is_read_only() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + var agent = orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + + // Client sends input, then a resize, then detaches. None of the first two may land. + var readBuf = new MemoryStream(); + await FrameCodec.WriteAsync(readBuf, LocalFrame.Stdin("hello"u8.ToArray()), default); + await FrameCodec.WriteAsync(readBuf, LocalFrame.Resize(40, 10), default); + await FrameCodec.WriteAsync(readBuf, LocalFrame.Detach(), default); + readBuf.Position = 0; + using var client = new DuplexTestStream(readBuf, new MemoryStream()); + + await orch.HandleLocalAttachAsync("flow-1", client, default); + + client.WrittenStream.Position = 0; + var first = await FrameCodec.ReadAsync(client.WrittenStream, default); + + await Assert.That(first!.Type).IsEqualTo(FrameType.AttachedReadOnly); + var (_, reason, _) = FrameCodec.AttachedReadOnly(first); + await Assert.That(reason).Contains("review-flow"); + await Assert.That(reason).Contains("reviewer"); + + // The resize must not have been recorded, so the PTY is never clamped to the viewer. + await Assert.That(agent.ClientDims).IsEmpty(); + } + + [Test] + public async Task Attaching_to_a_plain_agent_stays_read_write() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("plain-1"); + + var readBuf = new MemoryStream(); + await FrameCodec.WriteAsync(readBuf, LocalFrame.Detach(), default); + readBuf.Position = 0; + using var client = new DuplexTestStream(readBuf, new MemoryStream()); + + await orch.HandleLocalAttachAsync("plain-1", client, default); + + client.WrittenStream.Position = 0; + var first = await FrameCodec.ReadAsync(client.WrittenStream, default); + await Assert.That(first!.Type).IsEqualTo(FrameType.Attached); + } +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj --treenode-filter "/*/*/AgentOrchestratorVendorTests/Attaching_to*"` +Expected: FAIL — the flow case gets `FrameType.Attached`, not `AttachedReadOnly`. + +- [ ] **Step 3: Add the protection reason and route the attach** + +In `src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs`, replace `HandleLocalAttachAsync`: + +```csharp + /// Attach an existing agent to a local client (used by kcap agent attach). + public Task HandleLocalAttachAsync(string agentId, Stream stream, CancellationToken ct) { + if (!_agents.TryGetValue(agentId, out var agent)) + return FrameCodec.WriteAsync(stream, LocalFrame.Error($"no such agent {agentId}"), ct); + + // A review or flow agent is addressed through the flow protocol, never by typing at it, + // so the daemon — not the client — decides this attach carries no input. + return AttachClientLoopAsync(agent, stream, ct, readOnly: agent.Kind != LaunchKind.Default); + } + + /// Human-readable "why is this read-only", carried on the AttachedReadOnly frame. + static string ProtectionReason(AgentInstance agent) { + var kind = agent.Kind == LaunchKind.ReviewFlow ? "review-flow" : "review"; + var role = string.IsNullOrEmpty(agent.FlowRole) ? "" : $", role {agent.FlowRole}"; + var flow = string.IsNullOrEmpty(agent.FlowRunId) ? "" : $" (flow {agent.FlowRunId}{role})"; + + return $"{kind} agent{flow}"; + } +``` + +- [ ] **Step 4: Make the attach loop honour read-only** + +In the same file, change `AttachClientLoopAsync`'s signature and three points inside it. + +Signature: + +```csharp + internal async Task AttachClientLoopAsync( + AgentInstance agent, Stream stream, CancellationToken ct, bool readOnly = false) { +``` + +Replace the `Attached` send with a conditional one (the line currently reading `await Send(FrameCodec.Attached(agent.Id, snapshot));`): + +```csharp + await Send(readOnly + ? FrameCodec.AttachedReadOnly(agent.Id, ProtectionReason(agent), snapshot) + : FrameCodec.Attached(agent.Id, snapshot)); +``` + +Guard the two input arms in the read loop: + +```csharp + if (f.Type == FrameType.Stdin) { + if (readOnly) continue; // protected agent: input is never delivered + + try { +``` + +```csharp + } else if (f.Type == FrameType.Resize) { + // A read-only viewer must not enter ClientDims, or the min-clamp would + // let an observer shrink the participant's terminal. + if (!readOnly) ApplyResizeClamp(agent, sink, f.Cols, f.Rows); + } +``` + +- [ ] **Step 5: Run the tests to verify they pass** + +Run: `dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj --treenode-filter "/*/*/AgentOrchestratorVendorTests/Attaching_to*"` +Expected: PASS (2 tests). + +- [ ] **Step 6: Run the whole local-attach family for regressions** + +Run: `dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj --treenode-filter "/*/*/AgentOrchestratorVendorTests/*"` +Expected: PASS. The existing spawn-then-attach tests go through `AttachClientLoopAsync` with the new default `readOnly: false` and must be unaffected. + +- [ ] **Step 7: Commit** + +```bash +git add src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs +git commit -m "feat(daemon): attach to review and flow agents read-only" +``` + +--- + +### Task 4: CLI read-only attach banner + +**Files:** +- Modify: `src/Capacitor.Cli/Local/LocalAgentClient.cs` + +**Interfaces:** +- Consumes: `FrameType.AttachedReadOnly` and `FrameCodec.AttachedReadOnly(LocalFrame)` (Task 2). + +- [ ] **Step 1: Handle the frame in the output pump** + +In `src/Capacitor.Cli/Local/LocalAgentClient.cs`, add a `readOnly` flag beside the other loop state (next to `var detached = false;`): + +```csharp + var readOnly = false; // daemon attached us to a protected agent; input is dropped +``` + +Then add a case to the `switch (f.Type)` in `outPump`, directly after the `FrameType.Attached` case: + +```csharp + case FrameType.AttachedReadOnly: + readOnly = true; + var (_, reason, roSnapshot) = FrameCodec.AttachedReadOnly(f); + if (roSnapshot.Length > 0) TerminalRawMode.WriteStdout(roSnapshot, roSnapshot.Length); + + var banner = "\r\n— read-only: " + reason + ".\r\n" + + " Input is not delivered — address it with the flow tools." + + " Ctrl-Q d to detach. —\r\n"; + var bannerBytes = System.Text.Encoding.UTF8.GetBytes(banner); + TerminalRawMode.WriteStdout(bannerBytes, bannerBytes.Length); + + // No SizeFrame nudge: the daemon ignores our size for a protected + // agent, so asking for a repaint at our dimensions would mislead. + break; +``` + +- [ ] **Step 2: Stop the input and resize pumps from sending** + +In the `resizePump` loop, skip sending while read-only — replace its body's send line: + +```csharp + if (cur != last) { last = cur; if (!readOnly) await Send(SizeFrame()); } +``` + +In the `stdinPump` loop, forward nothing but keep the detach sequence working — replace the forward line: + +```csharp + var (forward, detach) = scanner.Process(buf.AsSpan(0, n)); + if (forward.Length > 0 && !readOnly) await Send(LocalFrame.Stdin(forward)); +``` + +- [ ] **Step 3: Verify the build** + +```bash +dotnet build src/Capacitor.Cli/Capacitor.Cli.csproj +``` + +Expected: builds with no errors. + +- [ ] **Step 4: Verify end to end against a live daemon** + +```bash +dotnet run --project src/Capacitor.Cli/Capacitor.Cli.csproj -- daemon start -d +dotnet run --project src/Capacitor.Cli/Capacitor.Cli.csproj -- agent start claude --detach +dotnet run --project src/Capacitor.Cli/Capacitor.Cli.csproj -- agent +dotnet run --project src/Capacitor.Cli/Capacitor.Cli.csproj -- agent attach +``` + +Expected: `agent` lists the agent with KIND `agent`; attaching is read-write as before (typing works, `Ctrl-Q d` detaches). A protected agent cannot be produced locally — `agent start` always creates `LaunchKind.Default` — so the read-only banner is covered by the daemon unit tests rather than this smoke check. Say so in your report rather than fabricating a flow. + +- [ ] **Step 5: Commit** + +```bash +git add src/Capacitor.Cli/Local/LocalAgentClient.cs +git commit -m "feat(cli): print the read-only banner and stop sending input" +``` + +--- + +### Task 5: Daemon-side stop protection + +**Files:** +- Modify: `src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs` +- Modify: `src/Capacitor.Cli.Daemon/Services/LocalControlServer.cs` +- Test: `test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs` + +**Interfaces:** +- Consumes: `FrameCodec.StopV2(LocalFrame)` → `(bool force, string agentId)` (Task 2); `ProtectionReason(AgentInstance)` (Task 3). +- Produces: `public Task HandleLocalStopV2Async(bool force, string agentId, Stream stream, CancellationToken ct)`. + +- [ ] **Step 1: Write the failing tests** + +Append to `test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs`, inside the `AgentOrchestratorVendorTests` partial class: + +```csharp + static async Task StopV2AndReadReply(AgentOrchestrator orch, bool force, string agentId) { + using var client = new DuplexTestStream(new MemoryStream(), new MemoryStream()); + await orch.HandleLocalStopV2Async(force, agentId, client, default); + client.WrittenStream.Position = 0; + + return await FrameCodec.ReadAsync(client.WrittenStream, default); + } + + [Test] + public async Task Stopping_a_flow_participant_without_force_is_refused() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + + var reply = await StopV2AndReadReply(orch, force: false, "flow-1"); + + await Assert.That(reply!.Type).IsEqualTo(FrameType.Error); + await Assert.That(reply.Text).Contains("review-flow"); + await Assert.That(reply.Text).Contains("--force"); + await Assert.That(orch.GetAgentForTest("flow-1")!.Status).IsNotEqualTo("Completed"); + } + + [Test] + public async Task Stopping_a_flow_participant_with_force_succeeds() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + + var reply = await StopV2AndReadReply(orch, force: true, "flow-1"); + + await Assert.That(reply!.Type).IsEqualTo(FrameType.StopAck); + await Assert.That(reply.Text).IsEqualTo("flow-1\tstopped"); + await Assert.That(orch.GetAgentForTest("flow-1")!.Status).IsEqualTo("Completed"); + } + + [Test] + public async Task Stop_all_without_force_skips_protected_agents_and_says_so() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("plain-1"); + orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + + var reply = await StopV2AndReadReply(orch, force: false, ""); + + var rows = reply!.Text.Split('\n').Select(l => l.Split('\t')).ToDictionary(p => p[0], p => p[1]); + await Assert.That(rows["plain-1"]).IsEqualTo("stopped"); + await Assert.That(rows["flow-1"]).IsEqualTo("skipped"); + await Assert.That(orch.GetAgentForTest("flow-1")!.Status).IsNotEqualTo("Completed"); + } + + [Test] + public async Task Stop_all_with_force_includes_protected_agents() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("plain-1"); + orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + + var reply = await StopV2AndReadReply(orch, force: true, ""); + + var rows = reply!.Text.Split('\n').Select(l => l.Split('\t')).ToDictionary(p => p[0], p => p[1]); + await Assert.That(rows["plain-1"]).IsEqualTo("stopped"); + await Assert.That(rows["flow-1"]).IsEqualTo("stopped"); + } +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj --treenode-filter "/*/*/AgentOrchestratorVendorTests/Stop*"` +Expected: FAIL — compile error, `HandleLocalStopV2Async` does not exist. + +- [ ] **Step 3: Add the V2 handler** + +In `src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs`, add after `HandleLocalStopAsync`: + +```csharp + /// + /// `kcap agent stop` with protection. A review or flow agent is refused unless the user + /// passed --force; a stop-all reports them as `skipped` rather than silently omitting them. + /// + public async Task HandleLocalStopV2Async(bool force, string agentId, Stream stream, CancellationToken ct) { + if (agentId.Length == 0) { + var all = _agents.Values.ToList(); + var eligible = all.Where(a => force || a.Kind == LaunchKind.Default).ToList(); + var results = await Task.WhenAll(eligible.Select(StopAgentCoreAsync)); + var stopped = eligible.Zip(results, (a, ok) => $"{a.Id}\t{StatusText(ok)}"); + var skipped = all.Except(eligible).Select(a => $"{a.Id}\tskipped"); + + await FrameCodec.WriteAsync(stream, LocalFrame.StopAck(string.Join('\n', stopped.Concat(skipped))), ct); + + return; + } + + if (_agents.TryGetValue(agentId, out var agent)) { + if (!force && agent.Kind != LaunchKind.Default) { + await FrameCodec.WriteAsync(stream, LocalFrame.Error( + $"{agentId} is a {ProtectionReason(agent)}. Stopping it mid-round leaves the flow " + + "without a participant. Pass --force to stop it anyway."), ct); + + return; + } + + var ok = await StopAgentCoreAsync(agent); + await FrameCodec.WriteAsync(stream, LocalFrame.StopAck($"{agentId}\t{StatusText(ok)}"), ct); + + return; + } + + // Not live here — it may be a survivor of a previous daemon incarnation, which the PID + // record can still reap. Kind is unknown for those, so protection cannot apply. + var reaped = await TryStopByPidRecordAsync(agentId); + + await FrameCodec.WriteAsync( + stream, + reaped ? LocalFrame.StopAck($"{agentId}\t{StatusText(true)}") : LocalFrame.Error($"no such agent {agentId}"), + ct); + } +``` + +- [ ] **Step 4: Route the frame** + +In `src/Capacitor.Cli.Daemon/Services/LocalControlServer.cs`, add a case after the `Stop` one and update the fallback message: + +```csharp + case FrameType.Stop: await orchestrator.HandleLocalStopAsync(first.Text, stream, ct); break; + case FrameType.StopV2: { + var (force, id) = FrameCodec.StopV2(first); + await orchestrator.HandleLocalStopV2Async(force, id, stream, ct); + + break; + } + case FrameType.Restart: await HandleRestartAsync(first.Text, stream, ct); break; + default: await FrameCodec.WriteAsync(stream, LocalFrame.Error($"expected Spawn/Attach/List/Stop/StopV2/Restart, got {first.Type}"), ct); break; +``` + +- [ ] **Step 5: Run the tests to verify they pass** + +Run: `dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj --treenode-filter "/*/*/AgentOrchestratorVendorTests/Stop*"` +Expected: PASS (the four new tests plus the existing `Stop`-path ones). + +- [ ] **Step 6: Commit** + +```bash +git add src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs src/Capacitor.Cli.Daemon/Services/LocalControlServer.cs test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs +git commit -m "feat(daemon): refuse to stop review and flow agents without force" +``` + +--- + +### Task 6: `kcap agent stop --force` + +**Files:** +- Modify: `src/Capacitor.Cli/Commands/AgentCommand.cs` (`StopAsync`, `SendStopAsync`) +- Test: `test/Capacitor.Cli.Tests.Unit/AgentCommandRoutingTests.cs` + +**Interfaces:** +- Consumes: `LocalFrame.StopV2(bool, string)` (Task 2); `AgentRow.Kind` and `AgentCommand.IsProtectedKind` (Task 1). +- Produces: `internal static (string[] Stoppable, string[] Protected) PartitionByProtection(IReadOnlyList agents)`. + +- [ ] **Step 1: Write the failing partition test** + +Append to `test/Capacitor.Cli.Tests.Unit/AgentCommandRoutingTests.cs`, inside the class: + +```csharp + [Test] + public async Task Protected_agents_are_partitioned_out_for_the_confirmation_prompt() { + AgentRow[] agents = [ + new("a1", "Running", "/r1", "agent", "", ""), + new("f1", "Running", "/r2", "review-flow", "flow-7f3a", "reviewer"), + new("v1", "Running", "/r3", "review", "", ""), + ]; + + var (stoppable, prot) = AgentCommand.PartitionByProtection(agents); + + await Assert.That(stoppable).IsEquivalentTo(new[] { "a1" }); + await Assert.That(prot).IsEquivalentTo(new[] { "f1", "v1" }); + } +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj --treenode-filter "/*/*/AgentCommandRoutingTests/*"` +Expected: FAIL — compile error, `PartitionByProtection` does not exist. + +- [ ] **Step 3: Add the partition helper** + +In `src/Capacitor.Cli/Commands/AgentCommand.cs`, next to `IsProtectedKind`: + +```csharp + /// Splits an agent list into what `stop --all` will stop and what it will skip. + internal static (string[] Stoppable, string[] Protected) PartitionByProtection(IReadOnlyList agents) => ( + [.. agents.Where(a => !IsProtectedKind(a.Kind)).Select(a => a.Id)], + [.. agents.Where(a => IsProtectedKind(a.Kind)).Select(a => a.Id)] + ); +``` + +- [ ] **Step 4: Run it to verify it passes** + +Run: `dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj --treenode-filter "/*/*/AgentCommandRoutingTests/*"` +Expected: PASS. + +- [ ] **Step 5: Accept `--force` and group the prompt** + +In `StopAsync`, add the flag beside the existing `all`/`yes` reads: + +```csharp + var force = args.Contains("--force"); +``` + +Replace the usage block so `--force` appears: + +```csharp + if (!all && !hasId) { + await Console.Error.WriteLineAsync("usage: kcap agent stop [--force] [--daemon ]"); + await Console.Error.WriteLineAsync(" kcap agent stop --all [-y] [--force] [--daemon ]"); + + return 1; + } +``` + +Replace the `--all` listing block (the `Console.WriteLine($"Found {agents.Count} agents:")` loop) with a grouped one: + +```csharp + var (stoppable, protectedIds) = PartitionByProtection(agents); + var targets = force ? agents.Select(a => a.Id).ToArray() : stoppable; + + if (targets.Length == 0) { + Console.WriteLine(protectedIds.Length > 0 + ? $"No agents to stop. {protectedIds.Length} review agent(s) skipped — pass --force to include them." + : "No agents."); + + return 0; + } + + Console.WriteLine($"Found {targets.Length} agents:"); + foreach (var a in agents.Where(a => targets.Contains(a.Id))) Console.WriteLine($" • {a.Id} {a.Repo}"); + + if (!force && protectedIds.Length > 0) { + Console.WriteLine($"Skipping {protectedIds.Length} review agent(s) — pass --force to include them:"); + foreach (var a in agents.Where(a => protectedIds.Contains(a.Id))) + Console.WriteLine($" • {a.Id} {a.Kind} {a.Repo}"); + } + + if (!yes) { + await Console.Out.WriteAsync($"Stop {targets.Length}? [y/N] "); + var reply = await Console.In.ReadLineAsync(); + + if (!string.Equals(reply?.Trim(), "y", StringComparison.OrdinalIgnoreCase)) { + await Console.Out.WriteLineAsync("Cancelled."); + + return 0; + } + } + + target = ""; +``` + +- [ ] **Step 6: Send `StopV2` and report `skipped`** + +Change the `SendStopAsync` call and signature to carry the flag: + +```csharp + return await SendStopAsync(sock, target, name, force); +``` + +```csharp + static async Task SendStopAsync(string sock, string agentId, string daemonName, bool force) { +``` + +Inside it, replace the write with the V2 frame: + +```csharp + await FrameCodec.WriteAsync(stream, LocalFrame.StopV2(force, agentId), default); +``` + +And replace the `StopAck` arm so `skipped` reads differently from `stopped`/`failed`: + +```csharp + case FrameType.StopAck: + string[] lines = resp.Text.Length == 0 ? [] : resp.Text.Split('\n'); + if (lines.Length == 0) { Console.WriteLine("No agents."); return 0; } + + var failed = 0; + var skipped = 0; + + foreach (var line in lines) { + var parts = line.Split('\t'); + var id = parts[0]; + var status = parts.Length > 1 ? parts[1] : "failed"; + + switch (status) { + case "stopped": Console.WriteLine($"Stopped {id}."); break; + case "skipped": Console.WriteLine($"Skipped {id} — review agent; pass --force to stop it."); skipped++; break; + default: Console.Error.WriteLine($"Failed to stop {id} — see `kcap daemon logs`."); failed++; break; + } + } + + if (skipped > 0) + Console.WriteLine($"{skipped} review agent(s) left running — pass --force to stop them."); + + // Skipping is the documented default, not a failure, so it does not affect + // the exit code. + return failed > 0 ? 1 : 0; +``` + +- [ ] **Step 7: Verify the build and the full unit suite** + +```bash +dotnet build src/Capacitor.Cli/Capacitor.Cli.csproj +dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj +``` + +Expected: build succeeds; no new failures beyond the ~42 baseline. + +- [ ] **Step 8: Verify end to end** + +```bash +dotnet run --project src/Capacitor.Cli/Capacitor.Cli.csproj -- daemon start -d +dotnet run --project src/Capacitor.Cli/Capacitor.Cli.csproj -- agent start claude --detach +dotnet run --project src/Capacitor.Cli/Capacitor.Cli.csproj -- agent stop --all +``` + +Expected: the prompt lists the one agent, stopping prints `Stopped .`, exit 0. Protected agents can't be created locally, so their paths are covered by the Task 5 daemon tests — say so in your report rather than inventing a flow. + +- [ ] **Step 9: Commit** + +```bash +git add src/Capacitor.Cli/Commands/AgentCommand.cs test/Capacitor.Cli.Tests.Unit/AgentCommandRoutingTests.cs +git commit -m "feat(cli): add `agent stop --force` and report skipped agents" +``` + +--- + +### Task 7: Documentation and final verification + +**Files:** +- Modify: `src/Capacitor.Cli.Core/Resources/help-agent.txt` +- Modify: `README.md` +- Test: `test/Capacitor.Cli.Tests.Integration/AgentVerbDispatchTests.cs` + +- [ ] **Step 1: Update the per-command help** + +In `src/Capacitor.Cli.Core/Resources/help-agent.txt`, replace the `Options for stop` block: + +``` +Options for stop: + --all Stop every agent, including --private ones. Review and + review-flow agents are skipped unless --force is given. + --force Include review and review-flow agents. Stopping a flow + participant mid-round leaves the flow without it. + --yes, -y Skip the confirmation prompt for --all +``` + +And add a section after the `Agent ids:` block: + +``` +Review and flow agents: + Agents the daemon runs as part of a review flow (KIND `review-flow`) or a PR + review (`review`) are not yours to drive directly — they are addressed through + the flow tools. `attach` gives you a read-only view of one: you see its output, + your input is not delivered. `stop` refuses unless you pass --force. +``` + +- [ ] **Step 2: Update the README** + +In `README.md`, in the `### Local agents (`kcap agent`)` section, replace the paragraph beginning "Agent ids are long" (it currently carries the `--all` warning added when this gap was deferred, pointing at #379): + +```markdown +Agent ids are long, so `attach` and `stop` accept **any unique prefix** — an ambiguous one lists the candidates instead of guessing. `stop --all` includes `--private` agents and prompts for confirmation unless you pass `--yes`/`-y`; a stop that cannot be confirmed prints a per-agent failure line and exits non-zero. + +**Agents that aren't yours.** `kcap agent ls` shows a `KIND` column: `agent` for ones you started, `review` for PR-review agents, and `review-flow` for review-flow participants (with their role). The daemon protects the latter two, because they are driven by the flow protocol rather than by you: + +- `kcap agent attach` on one is **read-only** — you see its output, your keystrokes are not delivered, and your terminal size is not applied to it. +- `kcap agent stop` on one is **refused** unless you pass `--force`, and `stop --all` skips them and says how many it skipped. + +Enforcement is in the daemon, so this holds regardless of client version. It does not apply when talking to a daemon older than this feature — that daemon reports no kind, every agent reads as `agent`, and the protections do not engage until it restarts onto the new binary. +``` + +- [ ] **Step 3: Add an integration case for the refusal** + +Append to `test/Capacitor.Cli.Tests.Integration/AgentVerbDispatchTests.cs`, inside the class: + +```csharp + [Test] + public async Task Stop_accepts_the_force_flag_without_treating_it_as_an_id() { + // --force must parse as a flag, not as a positional agent id, or the usage line fires. + var (_, stderr, _) = await RunCli("agent stop --all --force -y --daemon kcap-dispatch-test-absent"); + + await Assert.That(stderr).DoesNotContain("cannot combine an agent id with --all"); + await Assert.That(stderr).DoesNotContain("usage: kcap agent stop"); + } +``` + +- [ ] **Step 4: Run both suites** + +```bash +dotnet run --project test/Capacitor.Cli.Tests.Unit/Capacitor.Cli.Tests.Unit.csproj +dotnet run --project test/Capacitor.Cli.Tests.Integration/Capacitor.Cli.Tests.Integration.csproj +``` + +Expected: unit has no new failures beyond the ~42 baseline; integration fully passes. + +- [ ] **Step 5: Verify AOT publishes clean** + +```bash +dotnet publish src/Capacitor.Cli/Capacitor.Cli.csproj -c Release 2>&1 | grep -E 'IL[23][01][0-9]{2}' +dotnet publish src/Capacitor.Cli.Daemon/Capacitor.Cli.Daemon.csproj -c Release 2>&1 | grep -E 'IL[23][01][0-9]{2}' +``` + +Expected: no output from either. + +- [ ] **Step 6: Verify the help renders** + +```bash +dotnet run --project src/Capacitor.Cli/Capacitor.Cli.csproj -- agent --help +``` + +Expected: the new `Options for stop` and `Review and flow agents` text appears. + +- [ ] **Step 7: Commit** + +```bash +git add src/Capacitor.Cli.Core/Resources/help-agent.txt README.md test/Capacitor.Cli.Tests.Integration/AgentVerbDispatchTests.cs +git commit -m "docs: document read-only attach and stop --force for review agents" +``` + +--- + +## PR + +Reference both trackers in the description (title stays clean): + +- `Closes #379` +- `AI-1557` + +Call out in the body that protection does **not** engage against a daemon older than this change (it reports no kind), and that `--force` still leaves the flow unaware its participant was stopped — attributing that needs server-side work. diff --git a/docs/superpowers/specs/2026-07-29-ai1557-flow-participant-aware-agent-commands-design.md b/docs/superpowers/specs/2026-07-29-ai1557-flow-participant-aware-agent-commands-design.md index a208fb7007..1f2e01ffb0 100644 --- a/docs/superpowers/specs/2026-07-29-ai1557-flow-participant-aware-agent-commands-design.md +++ b/docs/superpowers/specs/2026-07-29-ai1557-flow-participant-aware-agent-commands-design.md @@ -63,6 +63,7 @@ by accident. | Value | Direction | Payload | |---|---|---| | `StopV2 = 10` | client → daemon | `mode⇥agentId` — mode `normal`\|`force`; empty id = all eligible | +| `AttachedReadOnly = 71` | daemon → client | `lp(agentId) + lp(reason) + snapshot` | Two payload extensions to existing frames: @@ -70,11 +71,16 @@ Two payload extensions to existing frames: `agent`\|`review`\|`review-flow`; the flow fields are empty for non-flow agents. - **`StopAck`** rows keep the `id⇥status` shape, with `status` gaining `skipped` alongside the existing `stopped`\|`failed`. -- **`Attached`** gains a trailing read-only flag and reason, appended after the existing payload. - This follows the precedent `FrameCodec.Spawn` already set for `isPrivate` ("APPENDED after - args: older parsers ignore trailing bytes"). -`Stop = 8` stays decodable so nothing regresses mid-upgrade, but the CLI only sends `StopV2`. +**Why `AttachedReadOnly` is a new frame rather than a flag on `Attached`.** `Attached`'s payload +is `lp(agentId) + snapshot`, and the decoder returns `f.Bytes[o..]` — the snapshot is *everything +remaining*. A trailing flag would be swallowed into the replay buffer and painted onto the user's +terminal. The `isPrivate` trick `FrameCodec.Spawn` uses works only because Spawn's payload is +fully counted, so it does not transfer. A separate frame also fails closed: an older CLI cannot +decode type 71, so it errors out instead of silently pumping stdin at a participant. + +`Stop = 8` and `Attached = 64` stay decodable so nothing regresses mid-upgrade; the CLI sends +`StopV2` and the daemon sends `AttachedReadOnly` only for protected agents. ## Daemon changes @@ -96,8 +102,9 @@ The `IsPrivate` guard on `HandleStopAgent` and the server-origin path are untouc - `AgentRow` gains `Kind`, `FlowRunId`, `FlowRole`. The parser accepts **3 or more** columns and defaults the new ones, so a running older daemon still lists correctly. - `ls` renders a `KIND` column, with the flow role appended for flow agents. -- `attach` prints the read-only banner when the daemon reports the flag, and skips its own stdin - pump — cosmetic, since the daemon already drops it. +- `attach` prints the read-only banner on `AttachedReadOnly`, and skips its own stdin pump, its + resize pump, and the `SizeFrame()` nudge it normally sends on attach. All three are cosmetic — + the daemon ignores them for a protected agent — but sending them would be misleading. - `stop` accepts `--force`, sends `StopV2`, and reports `skipped` rows distinctly from `stopped`. With `--all --force`, the confirmation prompt lists protected agents in their own labelled group, so the blast radius is visible before the user answers. From 139c682951fdc16fc536239b7958f4178cb0d85e Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Wed, 29 Jul 2026 17:26:25 +0200 Subject: [PATCH 03/15] feat(agent): carry and show each agent's launch kind --- .../Services/AgentOrchestrator.LocalIpc.cs | 12 ++++++- src/Capacitor.Cli/Commands/AgentCommand.cs | 35 +++++++++++++++---- .../AgentIdResolutionTests.cs | 35 +++++++++++++++++-- .../AgentOrchestratorLocalAttachTests.cs | 22 ++++++++++++ 4 files changed, 93 insertions(+), 11 deletions(-) diff --git a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs index a14353f88f..a898c67959 100644 --- a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs +++ b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs @@ -1,3 +1,4 @@ +using Capacitor.Cli.Core; using Capacitor.Cli.Core.LocalIpc; namespace Capacitor.Cli.Daemon.Services; @@ -6,11 +7,20 @@ namespace Capacitor.Cli.Daemon.Services; internal partial class AgentOrchestrator { /// Reply to a kcap agent ls request with a tab-separated agent table. public Task HandleLocalListAsync(Stream stream, CancellationToken ct) { - var lines = _agents.Values.Select(a => $"{a.Id}\t{a.Status}\t{a.RepoPath}"); + var lines = _agents.Values.Select(a => + $"{a.Id}\t{a.Status}\t{a.RepoPath}\t{KindText(a.Kind)}\t{a.FlowRunId ?? ""}\t{a.FlowRole ?? ""}"); return FrameCodec.WriteAsync(stream, new LocalFrame(FrameType.AgentList) { Text = string.Join('\n', lines) }, ct); } + /// Wire spelling of . Kept separate from the enum name so the table + /// reads as a CLI column rather than a .NET identifier. + static string KindText(LaunchKind kind) => kind switch { + LaunchKind.Review => "review", + LaunchKind.ReviewFlow => "review-flow", + _ => "agent", + }; + /// /// Stop one agent (or every agent, when is empty) on behalf of /// `kcap agent stop`. Calls the stop core directly rather than HandleStopAgent: the diff --git a/src/Capacitor.Cli/Commands/AgentCommand.cs b/src/Capacitor.Cli/Commands/AgentCommand.cs index 1943215d5a..556d669089 100644 --- a/src/Capacitor.Cli/Commands/AgentCommand.cs +++ b/src/Capacitor.Cli/Commands/AgentCommand.cs @@ -6,8 +6,10 @@ namespace Capacitor.Cli.Commands; -/// One row of the daemon's agent table (`id\tstatus\trepo` on the wire). -internal readonly record struct AgentRow(string Id, string Status, string Repo); +/// One row of the daemon's agent table (`id\tstatus\trepo\tkind\tflowRunId\tflowRole` on the +/// wire). A daemon older than #379 sends only the first three; the rest default. +internal readonly record struct AgentRow( + string Id, string Status, string Repo, string Kind, string FlowRunId, string FlowRole); /// /// `kcap agent start|ls|stop|attach` — drive daemon-hosted agents from the local @@ -286,8 +288,11 @@ static async Task ListAsync(string[] args) { return 0; } - Console.WriteLine($"{"AGENT",-34} {"STATUS",-10} REPO"); - foreach (var a in agents) Console.WriteLine($"{a.Id,-34} {a.Status,-10} {a.Repo}"); + Console.WriteLine($"{"AGENT",-34} {"STATUS",-10} {"KIND",-12} REPO"); + foreach (var a in agents) { + var role = a.FlowRole.Length > 0 ? $" [{a.FlowRole}]" : ""; + Console.WriteLine($"{a.Id,-34} {a.Status,-10} {a.Kind,-12} {a.Repo}{role}"); + } return 0; } @@ -327,9 +332,8 @@ static async Task ListAsync(string[] args) { if (resp.Text.Length == 0) return []; return [.. resp.Text.Split('\n') - .Select(l => l.Split('\t')) - .Where(p => p.Length == 3) - .Select(p => new AgentRow(p[0], p[1], p[2]))]; + .Where(l => l.Length > 0) + .Select(ParseAgentRow)]; } catch (Exception ex) when (ex is SocketException or IOException) { await Console.Error.WriteLineAsync($"kcap: cannot reach daemon: {ex.Message}"); @@ -420,6 +424,23 @@ internal static (string? Name, string? Error) DaemonNameFrom(string[] args) { : (next, null); } + /// Kinds the CLI refuses to mutate by accident: a reviewer mid-round is not the user's to + /// type at or stop. Mirrors LaunchKind — anything that is not a plain agent is protected. + internal static bool IsProtectedKind(string kind) => kind is "review" or "review-flow"; + + /// Tolerates a short row from an older daemon by defaulting the newer columns. + internal static AgentRow ParseAgentRow(string line) { + var p = line.Split('\t'); + + return new AgentRow( + p[0], + p.Length > 1 ? p[1] : "", + p.Length > 2 ? p[2] : "", + p.Length > 3 && p[3].Length > 0 ? p[3] : "agent", + p.Length > 4 ? p[4] : "", + p.Length > 5 ? p[5] : ""); + } + /// A full agent id as minted by `Guid.NewGuid().ToString("N")`. internal static bool IsFullAgentId(string s) => s.Length == 32 && s.All(char.IsAsciiHexDigit); diff --git a/test/Capacitor.Cli.Tests.Unit/AgentIdResolutionTests.cs b/test/Capacitor.Cli.Tests.Unit/AgentIdResolutionTests.cs index 2ef26c5fc6..3e55247eb3 100644 --- a/test/Capacitor.Cli.Tests.Unit/AgentIdResolutionTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/AgentIdResolutionTests.cs @@ -4,9 +4,9 @@ namespace Capacitor.Cli.Tests.Unit; public class AgentIdResolutionTests { static readonly AgentRow[] Agents = [ - new("ab12cd34ef56ab78cd90ef12ab34cd56", "Running", "/repo/one"), - new("ab99887766554433221100aabbccddee", "Running", "/repo/two"), - new("ff00112233445566778899aabbccddee", "Completed", "/repo/three"), + new("ab12cd34ef56ab78cd90ef12ab34cd56", "Running", "/repo/one", "agent", "", ""), + new("ab99887766554433221100aabbccddee", "Running", "/repo/two", "agent", "", ""), + new("ff00112233445566778899aabbccddee", "Completed", "/repo/three", "agent", "", ""), ]; [Test] @@ -63,4 +63,33 @@ public async Task An_uppercase_full_id_is_lowercased_so_the_ordinal_daemon_looku await Assert.That(id).IsEqualTo("0123456789abcdef0123456789abcdef"); await Assert.That(err).IsNull(); } + + [Test] + public async Task Row_from_a_current_daemon_carries_kind_and_flow() { + var row = AgentCommand.ParseAgentRow("ab12\tRunning\t/repo\treview-flow\tflow-7f3a\treviewer"); + await Assert.That(row.Id).IsEqualTo("ab12"); + await Assert.That(row.Status).IsEqualTo("Running"); + await Assert.That(row.Repo).IsEqualTo("/repo"); + await Assert.That(row.Kind).IsEqualTo("review-flow"); + await Assert.That(row.FlowRunId).IsEqualTo("flow-7f3a"); + await Assert.That(row.FlowRole).IsEqualTo("reviewer"); + } + + [Test] + public async Task Row_from_an_older_daemon_defaults_to_an_unprotected_agent() { + // An older daemon sends three columns. Treating the missing kind as `agent` is what + // makes the group keep working against it — protection simply does not engage. + var row = AgentCommand.ParseAgentRow("ab12\tRunning\t/repo"); + await Assert.That(row.Kind).IsEqualTo("agent"); + await Assert.That(row.FlowRunId).IsEqualTo(""); + await Assert.That(row.FlowRole).IsEqualTo(""); + await Assert.That(AgentCommand.IsProtectedKind(row.Kind)).IsFalse(); + } + + [Test] + public async Task Review_and_review_flow_are_protected_and_agent_is_not() { + await Assert.That(AgentCommand.IsProtectedKind("review")).IsTrue(); + await Assert.That(AgentCommand.IsProtectedKind("review-flow")).IsTrue(); + await Assert.That(AgentCommand.IsProtectedKind("agent")).IsFalse(); + } } diff --git a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs index 547f858d4c..ded9109b61 100644 --- a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs @@ -515,6 +515,28 @@ public async Task Local_socket_list_round_trips_registered_agents_over_a_real_so } } + [Test] + public async Task Local_list_reports_each_agent_kind_and_flow_identity() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("plain-1"); + orch.SeedAgentForTest("rev-1", kind: LaunchKind.Review); + orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + + using var client = new DuplexTestStream(new MemoryStream(), new MemoryStream()); + await orch.HandleLocalListAsync(client, default); + client.WrittenStream.Position = 0; + var reply = await FrameCodec.ReadAsync(client.WrittenStream, default); + + var rows = reply!.Text.Split('\n').Select(l => l.Split('\t')).ToDictionary(p => p[0], p => p); + + await Assert.That(rows["plain-1"][3]).IsEqualTo("agent"); + await Assert.That(rows["rev-1"][3]).IsEqualTo("review"); + await Assert.That(rows["flow-1"][3]).IsEqualTo("review-flow"); + await Assert.That(rows["flow-1"][4]).IsEqualTo("flow-7f3a"); + await Assert.That(rows["flow-1"][5]).IsEqualTo("reviewer"); + } + // ── Test doubles for the local-spawn lifecycle ────────────────────── sealed class EnvCapturingPtyFactory : IPtyProcessFactory { From 652fcd81759fc883648734bcbe413a79636db67c Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Wed, 29 Jul 2026 17:39:05 +0200 Subject: [PATCH 04/15] feat(ipc): add StopV2 and AttachedReadOnly frames --- src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs | 35 +++++++++++++++++-- src/Capacitor.Cli.Core/LocalIpc/FrameType.cs | 2 ++ src/Capacitor.Cli.Core/LocalIpc/LocalFrame.cs | 1 + .../FrameCodecTests.cs | 32 +++++++++++++++++ 4 files changed, 68 insertions(+), 2 deletions(-) diff --git a/src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs b/src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs index 65f4eb6da6..6abb15ddeb 100644 --- a/src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs +++ b/src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs @@ -50,7 +50,8 @@ static async Task ReadExactlyOrEof(Stream s, byte[] buf, CancellationToken FrameType.Error or FrameType.Attach or FrameType.AgentList or FrameType.Restart or FrameType.RestartAck or FrameType.Stop or FrameType.StopAck => Encoding.UTF8.GetBytes(f.Text), - FrameType.Attached or FrameType.Spawn => f.Bytes, // pre-encoded by Attached(...)/Spawn(...) + FrameType.Attached or FrameType.Spawn + or FrameType.StopV2 or FrameType.AttachedReadOnly => f.Bytes, // pre-encoded by the helpers below _ => throw new InvalidDataException($"unencodable frame {f.Type}"), }; @@ -62,7 +63,8 @@ or FrameType.Restart or FrameType.RestartAck FrameType.Error or FrameType.Attach or FrameType.AgentList or FrameType.Restart or FrameType.RestartAck or FrameType.Stop or FrameType.StopAck => new(t) { Text = Encoding.UTF8.GetString(p) }, - FrameType.Attached or FrameType.Spawn => new(t) { Bytes = p }, + FrameType.Attached or FrameType.Spawn + or FrameType.StopV2 or FrameType.AttachedReadOnly => new(t) { Bytes = p }, _ => throw new InvalidDataException($"undecodable frame {t}"), }; @@ -125,6 +127,35 @@ public static (string agentId, byte[] snapshot) Attached(LocalFrame f) { return (id, f.Bytes[o..]); } + // --- StopV2 structured payload --- + public static LocalFrame StopV2(bool force, string agentId) { + using var ms = new MemoryStream(); + ms.WriteByte((byte)(force ? 1 : 0)); + WriteLp(ms, agentId); + return new(FrameType.StopV2) { Bytes = ms.ToArray(), Text = agentId }; + } + public static (bool force, string agentId) StopV2(LocalFrame f) { + var o = 1; + return (f.Bytes[0] == 1, ReadLp(f.Bytes, ref o)); + } + + // --- AttachedReadOnly structured payload --- + // Length-prefixed id AND reason before the snapshot: the snapshot is the unbounded tail, so + // anything appended after it would be painted onto the user's terminal instead of parsed. + public static LocalFrame AttachedReadOnly(string agentId, string reason, byte[] snapshot) { + using var ms = new MemoryStream(); + WriteLp(ms, agentId); + WriteLp(ms, reason); + ms.Write(snapshot); + return new(FrameType.AttachedReadOnly) { Bytes = ms.ToArray(), Text = agentId }; + } + public static (string agentId, string reason, byte[] snapshot) AttachedReadOnly(LocalFrame f) { + var o = 0; + var id = ReadLp(f.Bytes, ref o); + var reason = ReadLp(f.Bytes, ref o); + return (id, reason, f.Bytes[o..]); + } + // --- byte helpers --- static byte[] Dims(ushort c, ushort r) { var b = new byte[4]; BinaryPrimitives.WriteUInt16BigEndian(b, c); BinaryPrimitives.WriteUInt16BigEndian(b.AsSpan(2), r); return b; } static byte[] BeInt(int v) { var b = new byte[4]; BinaryPrimitives.WriteInt32BigEndian(b, v); return b; } diff --git a/src/Capacitor.Cli.Core/LocalIpc/FrameType.cs b/src/Capacitor.Cli.Core/LocalIpc/FrameType.cs index 6c408e1055..6e9f923b71 100644 --- a/src/Capacitor.Cli.Core/LocalIpc/FrameType.cs +++ b/src/Capacitor.Cli.Core/LocalIpc/FrameType.cs @@ -12,6 +12,7 @@ public enum FrameType : byte { List = 6, // request the daemon's agent list (for `kcap agent ls`) Restart = 7, // request restart-after-update (Text = "when-idle"|"now"|"force") Stop = 8, // stop an agent (Text = agent id; empty = every agent this daemon hosts) + StopV2 = 10, // stop with a force flag (see FrameCodec.StopV2); supersedes Stop // daemon → client Attached = 64, Stdout = 65, @@ -20,4 +21,5 @@ public enum FrameType : byte { AgentList = 68, // UTF-8 table payload: one `id\tstatus\tcwd` line per agent RestartAck = 69, // acknowledgement for Restart (Text = short status) StopAck = 70, // acknowledgement for Stop (Text = one `id\tstatus` line per agent; status is "stopped" or "failed") + AttachedReadOnly = 71, // Attached for a protected agent: id + reason + snapshot, no input accepted } diff --git a/src/Capacitor.Cli.Core/LocalIpc/LocalFrame.cs b/src/Capacitor.Cli.Core/LocalIpc/LocalFrame.cs index 9019d11f89..2a1679a2d9 100644 --- a/src/Capacitor.Cli.Core/LocalIpc/LocalFrame.cs +++ b/src/Capacitor.Cli.Core/LocalIpc/LocalFrame.cs @@ -22,4 +22,5 @@ public sealed record LocalFrame(FrameType Type) { public static LocalFrame RestartAck(string status) => new(FrameType.RestartAck) { Text = status }; public static LocalFrame Stop(string agentId) => new(FrameType.Stop) { Text = agentId }; public static LocalFrame StopAck(string payload) => new(FrameType.StopAck) { Text = payload }; + public static LocalFrame StopV2(bool force, string agentId) => FrameCodec.StopV2(force, agentId); } diff --git a/test/Capacitor.Cli.Tests.Unit/FrameCodecTests.cs b/test/Capacitor.Cli.Tests.Unit/FrameCodecTests.cs index afc99aad5a..7d73acd78a 100644 --- a/test/Capacitor.Cli.Tests.Unit/FrameCodecTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/FrameCodecTests.cs @@ -130,6 +130,38 @@ public async Task StopAck_round_trips_a_newline_separated_id_status_list() { await Assert.That(r.Type).IsEqualTo(FrameType.StopAck); await Assert.That(r.Text.Split('\n')).IsEquivalentTo(new[] { "id-one\tstopped", "id-two\tfailed" }); } + + [Test] + [Arguments(true, "ab12")] + [Arguments(false, "ab12")] + [Arguments(false, "")] + public async Task StopV2_round_trips_mode_and_id(bool force, string agentId) { + var r = await RoundTrip(LocalFrame.StopV2(force, agentId)); + await Assert.That(r.Type).IsEqualTo(FrameType.StopV2); + + var (gotForce, gotId) = FrameCodec.StopV2(r); + await Assert.That(gotForce).IsEqualTo(force); + await Assert.That(gotId).IsEqualTo(agentId); + } + + [Test] + public async Task AttachedReadOnly_round_trips_id_reason_and_snapshot() { + var snapshot = new byte[] { 0x1b, 0x5b, 0x41, 0x00, 0xff }; + var r = await RoundTrip(FrameCodec.AttachedReadOnly("ab12", "review-flow reviewer", snapshot)); + await Assert.That(r.Type).IsEqualTo(FrameType.AttachedReadOnly); + + var (id, reason, got) = FrameCodec.AttachedReadOnly(r); + await Assert.That(id).IsEqualTo("ab12"); + await Assert.That(reason).IsEqualTo("review-flow reviewer"); + await Assert.That(got).IsEquivalentTo(snapshot); + } + + [Test] + public async Task AttachedReadOnly_round_trips_an_empty_snapshot() { + var r = await RoundTrip(FrameCodec.AttachedReadOnly("ab12", "review agent", [])); + var (_, _, got) = FrameCodec.AttachedReadOnly(r); + await Assert.That(got).IsEmpty(); + } } /// Stream that returns at most `chunk` bytes per ReadAsync to simulate partial socket reads. From 93145b9536f77632fc987951d4377e8518f55bb5 Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Wed, 29 Jul 2026 17:53:34 +0200 Subject: [PATCH 05/15] feat(daemon): attach to review and flow agents read-only --- .../Services/AgentOrchestrator.LocalIpc.cs | 26 +++++++++-- .../AgentOrchestratorLocalAttachTests.cs | 46 +++++++++++++++++++ 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs index a898c67959..cda2e20084 100644 --- a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs +++ b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs @@ -148,7 +148,18 @@ public Task HandleLocalAttachAsync(string agentId, Stream stream, CancellationTo if (!_agents.TryGetValue(agentId, out var agent)) return FrameCodec.WriteAsync(stream, LocalFrame.Error($"no such agent {agentId}"), ct); - return AttachClientLoopAsync(agent, stream, ct); + // A review or flow agent is addressed through the flow protocol, never by typing at it, + // so the daemon — not the client — decides this attach carries no input. + return AttachClientLoopAsync(agent, stream, ct, readOnly: agent.Kind != LaunchKind.Default); + } + + /// Human-readable "why is this read-only", carried on the AttachedReadOnly frame. + static string ProtectionReason(AgentInstance agent) { + var kind = agent.Kind == LaunchKind.ReviewFlow ? "review-flow" : "review"; + var role = string.IsNullOrEmpty(agent.FlowRole) ? "" : $", role {agent.FlowRole}"; + var flow = string.IsNullOrEmpty(agent.FlowRunId) ? "" : $" (flow {agent.FlowRunId}{role})"; + + return $"{kind} agent{flow}"; } /// @@ -156,7 +167,8 @@ public Task HandleLocalAttachAsync(string agentId, Stream stream, CancellationTo /// client's input (stdin/resize) until it detaches or disconnects. The agent keeps /// running either way — the sink is just removed. /// - internal async Task AttachClientLoopAsync(AgentInstance agent, Stream stream, CancellationToken ct) { + internal async Task AttachClientLoopAsync( + AgentInstance agent, Stream stream, CancellationToken ct, bool readOnly = false) { // One NetworkStream, two writers (the sink's Stdout frames + Attached/Exited here): // serialise all writes through this lock. Reads (the input loop) are independent. var writeLock = new SemaphoreSlim(1, 1); @@ -179,7 +191,9 @@ async Task Send(LocalFrame f) { try { // Bounded replay BEFORE any live chunk so the client paints a coherent screen. - await Send(FrameCodec.Attached(agent.Id, snapshot)); + await Send(readOnly + ? FrameCodec.AttachedReadOnly(agent.Id, ProtectionReason(agent), snapshot) + : FrameCodec.Attached(agent.Id, snapshot)); var pump = sink.RunAsync(ct); // Break this read loop when the agent exits on its own (CleanupAgentAsync trips @@ -203,6 +217,8 @@ async Task Send(LocalFrame f) { if (f is null || f.Type == FrameType.Detach) break; if (f.Type == FrameType.Stdin) { + if (readOnly) continue; // protected agent: input is never delivered + try { await agent.Runtime.SendRawInputAsync(f.Bytes); } catch (NotSupportedException) { @@ -215,7 +231,9 @@ async Task Send(LocalFrame f) { break; } } else if (f.Type == FrameType.Resize) { - ApplyResizeClamp(agent, sink, f.Cols, f.Rows); + // A read-only viewer must not enter ClientDims, or the min-clamp would + // let an observer shrink the participant's terminal. + if (!readOnly) ApplyResizeClamp(agent, sink, f.Cols, f.Rows); } } } catch (Exception ex) when (ex is EndOfStreamException or IOException or OperationCanceledException) { diff --git a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs index ded9109b61..c9218e880c 100644 --- a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs @@ -537,6 +537,52 @@ public async Task Local_list_reports_each_agent_kind_and_flow_identity() { await Assert.That(rows["flow-1"][5]).IsEqualTo("reviewer"); } + [Test] + public async Task Attaching_to_a_flow_participant_is_read_only() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + var agent = orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + + // Client sends input, then a resize, then detaches. None of the first two may land. + var readBuf = new MemoryStream(); + await FrameCodec.WriteAsync(readBuf, LocalFrame.Stdin("hello"u8.ToArray()), default); + await FrameCodec.WriteAsync(readBuf, LocalFrame.Resize(40, 10), default); + await FrameCodec.WriteAsync(readBuf, LocalFrame.Detach(), default); + readBuf.Position = 0; + using var client = new DuplexTestStream(readBuf, new MemoryStream()); + + await orch.HandleLocalAttachAsync("flow-1", client, default); + + client.WrittenStream.Position = 0; + var first = await FrameCodec.ReadAsync(client.WrittenStream, default); + + await Assert.That(first!.Type).IsEqualTo(FrameType.AttachedReadOnly); + var (_, reason, _) = FrameCodec.AttachedReadOnly(first); + await Assert.That(reason).Contains("review-flow"); + await Assert.That(reason).Contains("reviewer"); + + // The resize must not have been recorded, so the PTY is never clamped to the viewer. + await Assert.That(agent.ClientDims).IsEmpty(); + } + + [Test] + public async Task Attaching_to_a_plain_agent_stays_read_write() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("plain-1"); + + var readBuf = new MemoryStream(); + await FrameCodec.WriteAsync(readBuf, LocalFrame.Detach(), default); + readBuf.Position = 0; + using var client = new DuplexTestStream(readBuf, new MemoryStream()); + + await orch.HandleLocalAttachAsync("plain-1", client, default); + + client.WrittenStream.Position = 0; + var first = await FrameCodec.ReadAsync(client.WrittenStream, default); + await Assert.That(first!.Type).IsEqualTo(FrameType.Attached); + } + // ── Test doubles for the local-spawn lifecycle ────────────────────── sealed class EnvCapturingPtyFactory : IPtyProcessFactory { From 5e52f226756a98736a54f418d9d9096c72c9a377 Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Wed, 29 Jul 2026 18:01:44 +0200 Subject: [PATCH 06/15] test(daemon): prove the read-only attach test discriminates on stdin delivery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attaching_to_a_flow_participant_is_read_only previously only asserted the frame type and empty ClientDims — neither observes the Stdin arm, so NoopPtyProcess silently swallowed writes and the test stayed green even with the readOnly guard deleted. Seed the agent with a recording PTY double instead and assert zero writes; add the mirror assertion to the plain-agent case so the pair brackets the behaviour. --- .../AgentOrchestratorLocalAttachTests.cs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs index c9218e880c..0ed7634bf9 100644 --- a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs @@ -541,7 +541,8 @@ public async Task Local_list_reports_each_agent_kind_and_flow_identity() { public async Task Attaching_to_a_flow_participant_is_read_only() { var server = new TripwireServerConnection(); await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); - var agent = orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + var pty = new RecordingPtyProcess(); + var agent = orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer", pty: pty); // Client sends input, then a resize, then detaches. None of the first two may land. var readBuf = new MemoryStream(); @@ -563,15 +564,21 @@ public async Task Attaching_to_a_flow_participant_is_read_only() { // The resize must not have been recorded, so the PTY is never clamped to the viewer. await Assert.That(agent.ClientDims).IsEmpty(); + + // The stdin frame must never reach the runtime — this is the daemon-side guarantee + // itself, not just the client-observable frame type above. + await Assert.That(pty.Writes).IsEmpty(); } [Test] public async Task Attaching_to_a_plain_agent_stays_read_write() { var server = new TripwireServerConnection(); await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); - orch.SeedAgentForTest("plain-1"); + var pty = new RecordingPtyProcess(); + orch.SeedAgentForTest("plain-1", pty: pty); var readBuf = new MemoryStream(); + await FrameCodec.WriteAsync(readBuf, LocalFrame.Stdin("hello"u8.ToArray()), default); await FrameCodec.WriteAsync(readBuf, LocalFrame.Detach(), default); readBuf.Position = 0; using var client = new DuplexTestStream(readBuf, new MemoryStream()); @@ -581,6 +588,9 @@ public async Task Attaching_to_a_plain_agent_stays_read_write() { client.WrittenStream.Position = 0; var first = await FrameCodec.ReadAsync(client.WrittenStream, default); await Assert.That(first!.Type).IsEqualTo(FrameType.Attached); + + // Mirrors the read-only test: an unprotected agent's stdin really does reach the PTY. + await Assert.That(pty.Writes).IsEquivalentTo(new[] { "hello" }); } // ── Test doubles for the local-spawn lifecycle ────────────────────── From fea1e83795a8c9a3341db11ddcc8a165b608d38d Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Wed, 29 Jul 2026 18:10:52 +0200 Subject: [PATCH 07/15] feat(cli): print the read-only banner and stop sending input --- src/Capacitor.Cli/Local/LocalAgentClient.cs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/Capacitor.Cli/Local/LocalAgentClient.cs b/src/Capacitor.Cli/Local/LocalAgentClient.cs index 94fcd37fcc..230e72be18 100644 --- a/src/Capacitor.Cli/Local/LocalAgentClient.cs +++ b/src/Capacitor.Cli/Local/LocalAgentClient.cs @@ -38,6 +38,7 @@ public static async Task RunAsync(string socketPath, LocalFrame opening, Ca var errored = false; // daemon sent an Error frame (already printed) var detached = false; // user pressed the detach sequence var inputClosed = false; // local stdin reached EOF + var readOnly = false; // daemon attached us to a protected agent; input is dropped async Task Send(LocalFrame f) { await writeLock.WaitAsync(ct); @@ -62,6 +63,20 @@ async Task Send(LocalFrame f) { await Send(SizeFrame()); // nudge a clean repaint at our size + break; + case FrameType.AttachedReadOnly: + readOnly = true; + var (_, reason, roSnapshot) = FrameCodec.AttachedReadOnly(f); + if (roSnapshot.Length > 0) TerminalRawMode.WriteStdout(roSnapshot, roSnapshot.Length); + + var banner = "\r\n— read-only: " + reason + ".\r\n" + + " Input is not delivered — address it with the flow tools." + + " Ctrl-Q d to detach. —\r\n"; + var bannerBytes = System.Text.Encoding.UTF8.GetBytes(banner); + TerminalRawMode.WriteStdout(bannerBytes, bannerBytes.Length); + + // No SizeFrame nudge: the daemon ignores our size for a protected + // agent, so asking for a repaint at our dimensions would mislead. break; case FrameType.Exited: exitCode = f.ExitCode; @@ -89,7 +104,7 @@ async Task Send(LocalFrame f) { while (!ct.IsCancellationRequested && !outPump.IsCompleted) { await Task.Delay(300, ct); var cur = TrySize(); - if (cur != last) { last = cur; await Send(SizeFrame()); } + if (cur != last) { last = cur; if (!readOnly) await Send(SizeFrame()); } } } catch (Exception ex) when (ex is OperationCanceledException or IOException) { /* shutting down */ @@ -108,7 +123,7 @@ async Task Send(LocalFrame f) { if (n <= 0) { inputClosed = true; break; } var (forward, detach) = scanner.Process(buf.AsSpan(0, n)); - if (forward.Length > 0) await Send(LocalFrame.Stdin(forward)); + if (forward.Length > 0 && !readOnly) await Send(LocalFrame.Stdin(forward)); if (detach) { detached = true; await Send(LocalFrame.Detach()); break; } } } catch (Exception ex) when (ex is OperationCanceledException or IOException) { From 8cccb36f9ef7abf9eefe2fb63abe1263f37a8dda Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Wed, 29 Jul 2026 18:29:08 +0200 Subject: [PATCH 08/15] feat(daemon): refuse to stop review and flow agents without force --- .../Services/AgentOrchestrator.LocalIpc.cs | 43 +++++++++---- .../Services/LocalControlServer.cs | 8 ++- .../AgentOrchestratorLocalAttachTests.cs | 64 +++++++++++++++++++ 3 files changed, 103 insertions(+), 12 deletions(-) diff --git a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs index cda2e20084..6792c662e4 100644 --- a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs +++ b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs @@ -22,23 +22,44 @@ public Task HandleLocalListAsync(Stream stream, CancellationToken ct) { }; /// - /// Stop one agent (or every agent, when is empty) on behalf of - /// `kcap agent stop`. Calls the stop core directly rather than HandleStopAgent: the - /// private-agent guard there defends against server-origin commands, and a request arriving - /// on the daemon's own 0600 socket is the owner's. Stops run concurrently — each can take up - /// to 25s (graceful wait plus terminate), so serial teardown would be unusable. + /// Serves the legacy Stop frame from older clients that predate --force. That frame + /// has no force concept, so it always behaves as if --force were passed: an older client + /// gets exactly its previous (unprotected) behaviour, and gains no new refusals it has no + /// way to override. /// - public async Task HandleLocalStopAsync(string agentId, Stream stream, CancellationToken ct) { + public Task HandleLocalStopAsync(string agentId, Stream stream, CancellationToken ct) => + HandleLocalStopV2Async(force: true, agentId, stream, ct); + + /// + /// `kcap agent stop` with protection. A review or flow agent is refused unless the user + /// passed --force; a stop-all reports them as `skipped` rather than silently omitting them. + /// Calls the stop core directly rather than HandleStopAgent: the private-agent guard + /// there defends against server-origin commands, and a request arriving on the daemon's own + /// 0600 socket is the owner's. Stops run concurrently — each can take up to 25s (graceful + /// wait plus terminate), so serial teardown would be unusable. + /// + public async Task HandleLocalStopV2Async(bool force, string agentId, Stream stream, CancellationToken ct) { if (agentId.Length == 0) { - var all = _agents.Values.ToList(); - var results = await Task.WhenAll(all.Select(StopAgentCoreAsync)); - var lines = all.Zip(results, (a, ok) => $"{a.Id}\t{StatusText(ok)}"); - await FrameCodec.WriteAsync(stream, LocalFrame.StopAck(string.Join('\n', lines)), ct); + var all = _agents.Values.ToList(); + var eligible = all.Where(a => force || a.Kind == LaunchKind.Default).ToList(); + var results = await Task.WhenAll(eligible.Select(StopAgentCoreAsync)); + var stopped = eligible.Zip(results, (a, ok) => $"{a.Id}\t{StatusText(ok)}"); + var skipped = all.Except(eligible).Select(a => $"{a.Id}\tskipped"); + + await FrameCodec.WriteAsync(stream, LocalFrame.StopAck(string.Join('\n', stopped.Concat(skipped))), ct); return; } if (_agents.TryGetValue(agentId, out var agent)) { + if (!force && agent.Kind != LaunchKind.Default) { + await FrameCodec.WriteAsync(stream, LocalFrame.Error( + $"{agentId} is a {ProtectionReason(agent)}. Stopping it mid-round leaves the flow " + + "without a participant. Pass --force to stop it anyway."), ct); + + return; + } + var ok = await StopAgentCoreAsync(agent); await FrameCodec.WriteAsync(stream, LocalFrame.StopAck($"{agentId}\t{StatusText(ok)}"), ct); @@ -46,7 +67,7 @@ public async Task HandleLocalStopAsync(string agentId, Stream stream, Cancellati } // Not live here — it may be a survivor of a previous daemon incarnation, which the PID - // record can still reap. This is why the client sends full ids verbatim. + // record can still reap. Kind is unknown for those, so protection cannot apply. var reaped = await TryStopByPidRecordAsync(agentId); await FrameCodec.WriteAsync( diff --git a/src/Capacitor.Cli.Daemon/Services/LocalControlServer.cs b/src/Capacitor.Cli.Daemon/Services/LocalControlServer.cs index 2a39756858..77424f99cc 100644 --- a/src/Capacitor.Cli.Daemon/Services/LocalControlServer.cs +++ b/src/Capacitor.Cli.Daemon/Services/LocalControlServer.cs @@ -44,8 +44,14 @@ async Task HandleConnectionAsync(Socket conn, CancellationToken ct) { case FrameType.Attach: await orchestrator.HandleLocalAttachAsync(first.Text, stream, ct); break; case FrameType.List: await orchestrator.HandleLocalListAsync(stream, ct); break; case FrameType.Stop: await orchestrator.HandleLocalStopAsync(first.Text, stream, ct); break; + case FrameType.StopV2: { + var (force, id) = FrameCodec.StopV2(first); + await orchestrator.HandleLocalStopV2Async(force, id, stream, ct); + + break; + } case FrameType.Restart: await HandleRestartAsync(first.Text, stream, ct); break; - default: await FrameCodec.WriteAsync(stream, LocalFrame.Error($"expected Spawn/Attach/List/Stop/Restart, got {first.Type}"), ct); break; + default: await FrameCodec.WriteAsync(stream, LocalFrame.Error($"expected Spawn/Attach/List/Stop/StopV2/Restart, got {first.Type}"), ct); break; } } catch (Exception ex) when (ex is not OperationCanceledException) { LogConnectionError(ex); diff --git a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs index 0ed7634bf9..1db4032778 100644 --- a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs @@ -752,6 +752,70 @@ public async Task Local_stop_reports_stopped_when_the_reap_lands_just_after_the_ await Assert.That(reply.Text).IsEqualTo("reaped-1\tstopped"); } + static async Task StopV2AndReadReply(AgentOrchestrator orch, bool force, string agentId) { + using var client = new DuplexTestStream(new MemoryStream(), new MemoryStream()); + await orch.HandleLocalStopV2Async(force, agentId, client, default); + client.WrittenStream.Position = 0; + + return await FrameCodec.ReadAsync(client.WrittenStream, default); + } + + [Test] + public async Task Stopping_a_flow_participant_without_force_is_refused() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + + var reply = await StopV2AndReadReply(orch, force: false, "flow-1"); + + await Assert.That(reply!.Type).IsEqualTo(FrameType.Error); + await Assert.That(reply.Text).Contains("review-flow"); + await Assert.That(reply.Text).Contains("--force"); + await Assert.That(orch.GetAgentForTest("flow-1")!.Status).IsNotEqualTo("Completed"); + } + + [Test] + public async Task Stopping_a_flow_participant_with_force_succeeds() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + + var reply = await StopV2AndReadReply(orch, force: true, "flow-1"); + + await Assert.That(reply!.Type).IsEqualTo(FrameType.StopAck); + await Assert.That(reply.Text).IsEqualTo("flow-1\tstopped"); + await Assert.That(orch.GetAgentForTest("flow-1")!.Status).IsEqualTo("Completed"); + } + + [Test] + public async Task Stop_all_without_force_skips_protected_agents_and_says_so() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("plain-1"); + orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + + var reply = await StopV2AndReadReply(orch, force: false, ""); + + var rows = reply!.Text.Split('\n').Select(l => l.Split('\t')).ToDictionary(p => p[0], p => p[1]); + await Assert.That(rows["plain-1"]).IsEqualTo("stopped"); + await Assert.That(rows["flow-1"]).IsEqualTo("skipped"); + await Assert.That(orch.GetAgentForTest("flow-1")!.Status).IsNotEqualTo("Completed"); + } + + [Test] + public async Task Stop_all_with_force_includes_protected_agents() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("plain-1"); + orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + + var reply = await StopV2AndReadReply(orch, force: true, ""); + + var rows = reply!.Text.Split('\n').Select(l => l.Split('\t')).ToDictionary(p => p[0], p => p[1]); + await Assert.That(rows["plain-1"]).IsEqualTo("stopped"); + await Assert.That(rows["flow-1"]).IsEqualTo("stopped"); + } + /// A pty double whose process never exits — HasExited stays false even after /// TerminateAsync — so a test can drive the "stop could not be confirmed" path without a /// real hung process. From 51512c6482edfc2df6397589eaf6d8219502d0d8 Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Wed, 29 Jul 2026 18:38:47 +0200 Subject: [PATCH 09/15] fix(daemon): stop-all skip uses predicate negation, not record Except MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Except hashed AgentInstance's mutable teardown fields (Status, LastOutputAt, ...) rather than identity, and was enumerated after the concurrent stops had already started mutating them — a stopped agent could be misreported as skipped too, with a duplicate row in the ack. Also corrects the refusal message for a plain (non-flow) review agent, which previously claimed a flow round it doesn't have. --- .../Services/AgentOrchestrator.LocalIpc.cs | 19 +++++++++++++++---- .../AgentOrchestratorLocalAttachTests.cs | 17 +++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs index 6792c662e4..1fc0b0c11b 100644 --- a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs +++ b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs @@ -44,7 +44,12 @@ public async Task HandleLocalStopV2Async(bool force, string agentId, Stream stre var eligible = all.Where(a => force || a.Kind == LaunchKind.Default).ToList(); var results = await Task.WhenAll(eligible.Select(StopAgentCoreAsync)); var stopped = eligible.Zip(results, (a, ok) => $"{a.Id}\t{StatusText(ok)}"); - var skipped = all.Except(eligible).Select(a => $"{a.Id}\tskipped"); + + // The exact negation of the eligible predicate above — not a set difference. + // AgentInstance is a record with mutable fields (Status, LastOutputAt, ...), so + // Except would hash teardown-mutable state while the eligible agents are still + // draining from the WhenAll above, and could misreport a stopped agent as skipped too. + var skipped = all.Where(a => !force && a.Kind != LaunchKind.Default).Select(a => $"{a.Id}\tskipped"); await FrameCodec.WriteAsync(stream, LocalFrame.StopAck(string.Join('\n', stopped.Concat(skipped))), ct); @@ -53,9 +58,14 @@ public async Task HandleLocalStopV2Async(bool force, string agentId, Stream stre if (_agents.TryGetValue(agentId, out var agent)) { if (!force && agent.Kind != LaunchKind.Default) { + // A flow participant going away mid-round strands the flow; a plain hosted + // review has no round to strand, just a result that will never come back. + var consequence = agent.Kind == LaunchKind.ReviewFlow + ? "Stopping it mid-round leaves the flow without a participant." + : "Stopping it discards the review before it can report back."; + await FrameCodec.WriteAsync(stream, LocalFrame.Error( - $"{agentId} is a {ProtectionReason(agent)}. Stopping it mid-round leaves the flow " - + "without a participant. Pass --force to stop it anyway."), ct); + $"{agentId} is a {ProtectionReason(agent)}. {consequence} Pass --force to stop it anyway."), ct); return; } @@ -67,7 +77,8 @@ await FrameCodec.WriteAsync(stream, LocalFrame.Error( } // Not live here — it may be a survivor of a previous daemon incarnation, which the PID - // record can still reap. Kind is unknown for those, so protection cannot apply. + // record can still reap. This is why the client sends full ids verbatim. Kind is + // unknown for those, so protection cannot apply. var reaped = await TryStopByPidRecordAsync(agentId); await FrameCodec.WriteAsync( diff --git a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs index 1db4032778..ad075f29f0 100644 --- a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs @@ -787,6 +787,23 @@ public async Task Stopping_a_flow_participant_with_force_succeeds() { await Assert.That(orch.GetAgentForTest("flow-1")!.Status).IsEqualTo("Completed"); } + [Test] + public async Task Stopping_a_non_flow_review_agent_without_force_is_refused_with_an_accurate_message() { + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("rev-1", kind: LaunchKind.Review); + + var reply = await StopV2AndReadReply(orch, force: false, "rev-1"); + + await Assert.That(reply!.Type).IsEqualTo(FrameType.Error); + await Assert.That(reply.Text).Contains("review agent"); + // Unlike a flow participant, a plain hosted review has no round or flow to strand — + // the refusal must not claim one. + await Assert.That(reply.Text).DoesNotContain("flow"); + await Assert.That(reply.Text).Contains("--force"); + await Assert.That(orch.GetAgentForTest("rev-1")!.Status).IsNotEqualTo("Completed"); + } + [Test] public async Task Stop_all_without_force_skips_protected_agents_and_says_so() { var server = new TripwireServerConnection(); From ac749b27f4821296a4a9c9e875fcfe9a4bc4666e Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Wed, 29 Jul 2026 19:08:31 +0200 Subject: [PATCH 10/15] feat(cli): add `agent stop --force` and report skipped agents Switches the client onto the StopV2 frame the daemon has enforced protection against since #378: `stop --all` now partitions review/review-flow agents out of the confirmation prompt and reports them as skipped (exit 0) rather than stopping everything unconditionally, and `--force` opts back in. Also pins LocalControlServer's StopV2 decode-and-dispatch over a real socket, the one hop the codec round-trip and handler tests don't individually cover. --- src/Capacitor.Cli/Commands/AgentCommand.cs | 83 ++++++++++++------- .../AgentCommandRoutingTests.cs | 14 ++++ .../AgentOrchestratorLocalAttachTests.cs | 65 +++++++++++++++ 3 files changed, 134 insertions(+), 28 deletions(-) diff --git a/src/Capacitor.Cli/Commands/AgentCommand.cs b/src/Capacitor.Cli/Commands/AgentCommand.cs index 556d669089..cb5921ff56 100644 --- a/src/Capacitor.Cli/Commands/AgentCommand.cs +++ b/src/Capacitor.Cli/Commands/AgentCommand.cs @@ -124,6 +124,7 @@ static async Task AttachAsync(string[] args) { static async Task StopAsync(string[] args) { var all = args.Contains("--all"); var yes = args.Contains("--yes") || args.Contains("-y"); + var force = args.Contains("--force"); var hasId = args.Length > 0 && !args[0].StartsWith('-'); if (all && hasId) { @@ -133,8 +134,8 @@ static async Task StopAsync(string[] args) { } if (!all && !hasId) { - await Console.Error.WriteLineAsync("usage: kcap agent stop [--daemon ]"); - await Console.Error.WriteLineAsync(" kcap agent stop --all [-y] [--daemon ]"); + await Console.Error.WriteLineAsync("usage: kcap agent stop [--force] [--daemon ]"); + await Console.Error.WriteLineAsync(" kcap agent stop --all [-y] [--force] [--daemon ]"); return 1; } @@ -167,11 +168,28 @@ static async Task StopAsync(string[] args) { return 0; } - Console.WriteLine($"Found {agents.Count} agents:"); - foreach (var a in agents) Console.WriteLine($" • {a.Id} {a.Repo}"); + var (stoppable, protectedIds) = PartitionByProtection(agents); + var targets = force ? agents.Select(a => a.Id).ToArray() : stoppable; + + if (targets.Length == 0) { + Console.WriteLine(protectedIds.Length > 0 + ? $"No agents to stop. {protectedIds.Length} review agent(s) skipped — pass --force to include them." + : "No agents."); + + return 0; + } + + Console.WriteLine($"Found {targets.Length} agents:"); + foreach (var a in agents.Where(a => targets.Contains(a.Id))) Console.WriteLine($" • {a.Id} {a.Repo}"); + + if (!force && protectedIds.Length > 0) { + Console.WriteLine($"Skipping {protectedIds.Length} review agent(s) — pass --force to include them:"); + foreach (var a in agents.Where(a => protectedIds.Contains(a.Id))) + Console.WriteLine($" • {a.Id} {a.Kind} {a.Repo}"); + } if (!yes) { - await Console.Out.WriteAsync($"Stop all {agents.Count}? [y/N] "); + await Console.Out.WriteAsync($"Stop {targets.Length}? [y/N] "); var reply = await Console.In.ReadLineAsync(); if (!string.Equals(reply?.Trim(), "y", StringComparison.OrdinalIgnoreCase)) { @@ -189,51 +207,54 @@ static async Task StopAsync(string[] args) { target = resolved; } - return await SendStopAsync(sock, target, name); + return await SendStopAsync(sock, target, name, force); } - static async Task SendStopAsync(string sock, string agentId, string daemonName) { + static async Task SendStopAsync(string sock, string agentId, string daemonName, bool force) { try { using var socket = new Socket(AddressFamily.Unix, SocketType.Stream, ProtocolType.Unspecified); await socket.ConnectAsync(new UnixDomainSocketEndPoint(sock)); await using var stream = new NetworkStream(socket, ownsSocket: false); - await FrameCodec.WriteAsync(stream, LocalFrame.Stop(agentId), default); + await FrameCodec.WriteAsync(stream, LocalFrame.StopV2(force, agentId), default); var resp = await FrameCodec.ReadAsync(stream, default); switch (resp?.Type) { case FrameType.StopAck: - if (resp.Text.Length == 0) { - Console.WriteLine("No agents."); - - return 0; - } + string[] lines = resp.Text.Length == 0 ? [] : resp.Text.Split('\n'); + if (lines.Length == 0) { Console.WriteLine("No agents."); return 0; } - var lines = resp.Text.Split('\n'); - var anyFailed = false; + var failed = 0; + var skipped = 0; foreach (var line in lines) { - var parts = line.Split('\t'); - var id = parts[0]; - - if (parts is [_, "stopped"]) { - Console.WriteLine($"Stopped {id}."); - } else { - Console.WriteLine($"Failed to stop {id} — see `kcap daemon logs`."); - anyFailed = true; + var parts = line.Split('\t'); + var id = parts[0]; + var status = parts.Length > 1 ? parts[1] : "failed"; + + switch (status) { + case "stopped": Console.WriteLine($"Stopped {id}."); break; + case "skipped": Console.WriteLine($"Skipped {id} — review agent; pass --force to stop it."); skipped++; break; + default: Console.Error.WriteLine($"Failed to stop {id} — see `kcap daemon logs`."); failed++; break; } } - return anyFailed ? 1 : 0; + if (skipped > 0) + Console.WriteLine($"{skipped} review agent(s) left running — pass --force to stop them."); + + // Skipping is the documented default, not a failure, so it does not affect + // the exit code. + return failed > 0 ? 1 : 0; case FrameType.Error: await Console.Error.WriteLineAsync($"kcap: {resp.Text}"); return 1; case null: - // An older daemon can't decode frame type 8: it faults before its frame - // switch and closes without replying, so we see a clean EOF. `--force` is - // the only restart mode that bypasses the busy check, and this daemon is - // guaranteed busy — the agent we're trying to stop is still running. + // An older daemon can't decode frame type 10 (StopV2): it faults before its + // frame switch and closes without replying, so we see a clean EOF. `--force` + // here is the restart flag, not the stop flag — this daemon is guaranteed + // busy since the stop it never understood never ran, so `restart` needs its + // own override to proceed. await Console.Error.WriteLineAsync( $"kcap: this daemon is too old for `agent stop` — restart it with `kcap daemon restart --force --name {daemonName}`"); @@ -428,6 +449,12 @@ internal static (string? Name, string? Error) DaemonNameFrom(string[] args) { /// type at or stop. Mirrors LaunchKind — anything that is not a plain agent is protected. internal static bool IsProtectedKind(string kind) => kind is "review" or "review-flow"; + /// Splits an agent list into what `stop --all` will stop and what it will skip. + internal static (string[] Stoppable, string[] Protected) PartitionByProtection(IReadOnlyList agents) => ( + [.. agents.Where(a => !IsProtectedKind(a.Kind)).Select(a => a.Id)], + [.. agents.Where(a => IsProtectedKind(a.Kind)).Select(a => a.Id)] + ); + /// Tolerates a short row from an older daemon by defaulting the newer columns. internal static AgentRow ParseAgentRow(string line) { var p = line.Split('\t'); diff --git a/test/Capacitor.Cli.Tests.Unit/AgentCommandRoutingTests.cs b/test/Capacitor.Cli.Tests.Unit/AgentCommandRoutingTests.cs index e100b54885..539702d694 100644 --- a/test/Capacitor.Cli.Tests.Unit/AgentCommandRoutingTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/AgentCommandRoutingTests.cs @@ -88,4 +88,18 @@ public async Task DaemonNameFrom_empty_string_value_is_an_error_not_a_silent_def await Assert.That(name).IsNull(); await Assert.That(error).IsNotNull(); } + + [Test] + public async Task Protected_agents_are_partitioned_out_for_the_confirmation_prompt() { + AgentRow[] agents = [ + new("a1", "Running", "/r1", "agent", "", ""), + new("f1", "Running", "/r2", "review-flow", "flow-7f3a", "reviewer"), + new("v1", "Running", "/r3", "review", "", ""), + ]; + + var (stoppable, prot) = AgentCommand.PartitionByProtection(agents); + + await Assert.That(stoppable).IsEquivalentTo(new[] { "a1" }); + await Assert.That(prot).IsEquivalentTo(new[] { "f1", "v1" }); + } } diff --git a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs index ad075f29f0..cfd313ef97 100644 --- a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs @@ -469,6 +469,7 @@ public void Resize(ushort cols, ushort rows) { } } [Test] + [NotInParallel(nameof(DaemonLockPaths) + ".OverrideDirectoryForTesting")] public async Task Local_socket_list_round_trips_registered_agents_over_a_real_socket() { if (OperatingSystem.IsWindows()) return; // Unix-domain socket path @@ -515,6 +516,70 @@ public async Task Local_socket_list_round_trips_registered_agents_over_a_real_so } } + /// + /// Pins the one hop nothing else exercises: decoding a raw + /// StopV2 frame off a real socket and forwarding its force flag to the orchestrator. The codec + /// round-trip (FrameCodecTests) and the handler (StopV2AndReadReply above) are each covered in + /// isolation; only a real connection proves the server's frame switch wires them together. + /// + static async Task StopV2OverRealSocketAsync(string daemonName, bool force, string agentId) { + var sockDir = Directory.CreateTempSubdirectory("kcap-sock-"); + DaemonLockPaths.OverrideDirectoryForTesting(sockDir.FullName); + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(15)); + + LocalControlServer? listener = null; + AgentOrchestrator? orch = null; + + try { + orch = BuildOrchestrator(new TripwireServerConnection(), new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("flow-1", kind: LaunchKind.ReviewFlow, flowRunId: "flow-7f3a", flowRole: "reviewer"); + + var config = new DaemonConfig { Name = daemonName, ServerUrl = "http://127.0.0.1:1" }; + listener = new LocalControlServer(config, orch, TestCoordinator(), NullLogger.Instance); + await listener.StartAsync(cts.Token); + + var sockPath = LocalSocketPaths.Socket(daemonName); + var deadline = DateTime.UtcNow.AddSeconds(5); + while (!File.Exists(sockPath) && DateTime.UtcNow < deadline) await Task.Delay(20, cts.Token); + await Assert.That(File.Exists(sockPath)).IsTrue(); + + using var sock = new Socket(AddressFamily.Unix, SocketType.Stream, ProtocolType.Unspecified); + await sock.ConnectAsync(new UnixDomainSocketEndPoint(sockPath), cts.Token); + await using var stream = new NetworkStream(sock, ownsSocket: false); + + await FrameCodec.WriteAsync(stream, LocalFrame.StopV2(force, agentId), cts.Token); + + return await FrameCodec.ReadAsync(stream, cts.Token); + } finally { + if (orch is not null) await orch.DisposeAsync(); + if (listener is not null) { await listener.StopAsync(CancellationToken.None); listener.Dispose(); } + DaemonLockPaths.OverrideDirectoryForTesting(null); + try { Directory.Delete(sockDir.FullName, true); } catch { /* best-effort */ } + } + } + + [Test] + [NotInParallel(nameof(DaemonLockPaths) + ".OverrideDirectoryForTesting")] + public async Task Local_socket_stopv2_without_force_refuses_a_protected_agent_end_to_end() { + if (OperatingSystem.IsWindows()) return; // Unix-domain socket path + + var resp = await StopV2OverRealSocketAsync("test-stopv2-refuse", force: false, "flow-1"); + + await Assert.That(resp!.Type).IsEqualTo(FrameType.Error); + await Assert.That(resp.Text).Contains("--force"); + } + + [Test] + [NotInParallel(nameof(DaemonLockPaths) + ".OverrideDirectoryForTesting")] + public async Task Local_socket_stopv2_with_force_stops_a_protected_agent_end_to_end() { + if (OperatingSystem.IsWindows()) return; // Unix-domain socket path + + var resp = await StopV2OverRealSocketAsync("test-stopv2-force", force: true, "flow-1"); + + await Assert.That(resp!.Type).IsEqualTo(FrameType.StopAck); + await Assert.That(resp.Text).IsEqualTo("flow-1\tstopped"); + } + [Test] public async Task Local_list_reports_each_agent_kind_and_flow_identity() { var server = new TripwireServerConnection(); From d4aeb2c20928981f53e787d8a53bd9aa0e5c0a4f Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Wed, 29 Jul 2026 19:27:59 +0200 Subject: [PATCH 11/15] docs: document read-only attach and stop --force for review agents --- README.md | 9 ++++++++- src/Capacitor.Cli.Core/LocalIpc/FrameType.cs | 2 +- src/Capacitor.Cli.Core/Resources/help-agent.txt | 16 ++++++++++------ src/Capacitor.Cli.Core/Resources/help-usage.txt | 2 +- .../AgentVerbDispatchTests.cs | 9 +++++++++ 5 files changed, 29 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index e6ff2d9eb8..ee64765bdb 100644 --- a/README.md +++ b/README.md @@ -954,7 +954,14 @@ kcap agent stop ab12 # graceful /exit, then terminate kcap agent stop --all -y # stop every agent this daemon hosts, no prompt ``` -Agent ids are long, so `attach` and `stop` accept **any unique prefix** — an ambiguous one lists the candidates instead of guessing. `stop --all` includes `--private` agents and prompts for confirmation unless you pass `--yes`/`-y`. `--all` stops **every** agent this daemon hosts, not just the ones you started from this CLI — including agents launched from the web UI and review-flow participants (e.g. reviewers mid-round). Making these commands flow-participant aware is tracked in [#379](https://github.com/kurrent-io/kcap-cli/issues/379). If a stop can't be confirmed (e.g. an agent that never exits, even after SIGKILL), `stop` prints a per-agent "Failed to stop `` — see `kcap daemon logs`." line and exits non-zero; `stop --all` still stops the rest and reports each agent's outcome individually. +Agent ids are long, so `attach` and `stop` accept **any unique prefix** — an ambiguous one lists the candidates instead of guessing. `stop --all` includes `--private` agents and prompts for confirmation unless you pass `--yes`/`-y`; a stop that cannot be confirmed prints a per-agent failure line and exits non-zero. + +**Agents that aren't yours.** `kcap agent ls` shows a `KIND` column: `agent` for ones you started, `review` for PR-review agents, and `review-flow` for review-flow participants (with their role). The daemon protects the latter two, because they are driven by the flow protocol rather than by you: + +- `kcap agent attach` on one is **read-only** — you see its output, your keystrokes are not delivered, and your terminal size is not applied to it. +- `kcap agent stop` on one is **refused** unless you pass `--force`, and `stop --all` skips them and says how many it skipped. + +Enforcement is in the daemon, so this holds regardless of client version. It does not apply when talking to a daemon older than this feature — that daemon reports no kind, every agent reads as `agent`, and the protections do not engage until it restarts onto the new binary. `agent start` auto-starts the daemon if one isn't already running, and needs a configured server for the daemon to record to. `ls`, `attach`, and `stop` only talk to the local socket, so they work without one. A locally-started agent appears in **your own** web UI (owner-only until you share it from the web UI); use `--private` to opt out of registration entirely. Unix only for now. diff --git a/src/Capacitor.Cli.Core/LocalIpc/FrameType.cs b/src/Capacitor.Cli.Core/LocalIpc/FrameType.cs index 6e9f923b71..2a6ca4edf3 100644 --- a/src/Capacitor.Cli.Core/LocalIpc/FrameType.cs +++ b/src/Capacitor.Cli.Core/LocalIpc/FrameType.cs @@ -20,6 +20,6 @@ public enum FrameType : byte { Error = 67, AgentList = 68, // UTF-8 table payload: one `id\tstatus\tcwd` line per agent RestartAck = 69, // acknowledgement for Restart (Text = short status) - StopAck = 70, // acknowledgement for Stop (Text = one `id\tstatus` line per agent; status is "stopped" or "failed") + StopAck = 70, // acknowledgement for Stop (Text = one `id\tstatus` line per agent; status is "stopped", "skipped", or "failed") AttachedReadOnly = 71, // Attached for a protected agent: id + reason + snapshot, no input accepted } diff --git a/src/Capacitor.Cli.Core/Resources/help-agent.txt b/src/Capacitor.Cli.Core/Resources/help-agent.txt index 10f14888b5..fbe324310a 100644 --- a/src/Capacitor.Cli.Core/Resources/help-agent.txt +++ b/src/Capacitor.Cli.Core/Resources/help-agent.txt @@ -22,14 +22,12 @@ Options for start: -- Everything after this is passed to the vendor CLI verbatim Options for stop: - --all Stop every agent, including --private ones + --all Stop every agent, including --private ones. Review and + review-flow agents are skipped unless --force is given. + --force Include review and review-flow agents. Stopping a flow + participant mid-round leaves the flow without it. --yes, -y Skip the confirmation prompt for --all - --all stops every agent this daemon hosts, not just the ones you started - from this CLI — including agents launched from the web UI and review-flow - participants (e.g. reviewers mid-round). See - https://github.com/kurrent-io/kcap-cli/issues/379. - Options for ls, attach, stop: --daemon Target a specific daemon (defaults to your profile's) @@ -37,6 +35,12 @@ Agent ids: Any unique prefix works — `kcap agent attach ab12` is enough as long as it matches exactly one agent. An ambiguous prefix lists the candidates. +Review and flow agents: + Agents the daemon runs as part of a review flow (KIND `review-flow`) or a PR + review (`review`) are not yours to drive directly — they are addressed through + the flow tools. `attach` gives you a read-only view of one: you see its output, + your input is not delivered. `stop` refuses unless you pass --force. + Notes: The daemon owns the agent, not your terminal, so you can detach and the agent keeps running. Detach with the prefix key Ctrl-Q then d, and reattach later diff --git a/src/Capacitor.Cli.Core/Resources/help-usage.txt b/src/Capacitor.Cli.Core/Resources/help-usage.txt index 7363cf98e7..34d5b0cee4 100644 --- a/src/Capacitor.Cli.Core/Resources/help-usage.txt +++ b/src/Capacitor.Cli.Core/Resources/help-usage.txt @@ -30,7 +30,7 @@ Agents: agent start [-d] [--worktree] [--private] [-- ] Start a daemon-hosted agent agent ls List this daemon's agents (id, status, repo) agent attach Attach your terminal to a running agent (id prefix ok) - agent stop | --all [-y] Stop one agent, or all of them + agent stop | --all [-y] [--force] Stop one agent, or all of them Daemon: daemon start [-d] [--name N] Start the daemon (foreground, or -d for background; refuses on duplicate name) diff --git a/test/Capacitor.Cli.Tests.Integration/AgentVerbDispatchTests.cs b/test/Capacitor.Cli.Tests.Integration/AgentVerbDispatchTests.cs index 1bb73b9934..3fb4021f14 100644 --- a/test/Capacitor.Cli.Tests.Integration/AgentVerbDispatchTests.cs +++ b/test/Capacitor.Cli.Tests.Integration/AgentVerbDispatchTests.cs @@ -88,6 +88,15 @@ public async Task Start_without_a_server_reports_the_missing_server_itself() { await Assert.That(stderr).Contains("kcap agent start: no server configured"); } + [Test] + public async Task Stop_accepts_the_force_flag_without_treating_it_as_an_id() { + // --force must parse as a flag, not as a positional agent id, or the usage line fires. + var (_, stderr, _) = await RunCli("agent stop --all --force -y --daemon kcap-dispatch-test-absent"); + + await Assert.That(stderr).DoesNotContain("cannot combine an agent id with --all"); + await Assert.That(stderr).DoesNotContain("usage: kcap agent stop"); + } + static async Task<(string Stdout, string Stderr, int ExitCode)> RunCli( string argLine, bool clearServerUrl = false) { var binary = GetCliBinaryPath(); From b9a5fcfb195d4ce51631dcf92412081ba78cea04 Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Wed, 29 Jul 2026 19:41:28 +0200 Subject: [PATCH 12/15] docs: fix version-skew inaccuracies in the agent-protection caveat The claim that daemon-side enforcement "holds regardless of client version" was wrong: an old kcap sends the legacy Stop request (no --force concept), and the daemon treats that as --force, so a stale client can silently force-stop a review/review-flow agent against an up-to-date daemon. The old-daemon sentence also conflated stop with ls/attach: those degrade silently, but stop always sends the newer request format an old daemon can't decode, so it hard-fails and tells the user to restart the daemon instead of running unprotected. Also filled in two gaps in help-agent.txt's parallel text: attach's read-only view also ignores terminal resize, and --force lifts the single-id refusal, not just the --all skip. --- README.md | 2 +- src/Capacitor.Cli.Core/Resources/help-agent.txt | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index ee64765bdb..5f5552ae81 100644 --- a/README.md +++ b/README.md @@ -961,7 +961,7 @@ Agent ids are long, so `attach` and `stop` accept **any unique prefix** — an a - `kcap agent attach` on one is **read-only** — you see its output, your keystrokes are not delivered, and your terminal size is not applied to it. - `kcap agent stop` on one is **refused** unless you pass `--force`, and `stop --all` skips them and says how many it skipped. -Enforcement is in the daemon, so this holds regardless of client version. It does not apply when talking to a daemon older than this feature — that daemon reports no kind, every agent reads as `agent`, and the protections do not engage until it restarts onto the new binary. +Enforcement lives in the daemon, so a current CLI can't bypass it by skipping the check or lying about `--force`. That guarantee has two version-skew exceptions: an old `kcap` sends the legacy `Stop` request, which has no `--force` concept — the daemon treats that as `--force`, so an old client can silently force-stop a review or review-flow agent. And against an old daemon, `ls`/`attach` degrade silently (no kind reported, every agent reads as `agent`), but `stop` doesn't degrade — it sends a newer request format the old daemon can't decode, so the connection closes and the CLI tells you to restart the daemon instead of stopping anything. `agent start` auto-starts the daemon if one isn't already running, and needs a configured server for the daemon to record to. `ls`, `attach`, and `stop` only talk to the local socket, so they work without one. A locally-started agent appears in **your own** web UI (owner-only until you share it from the web UI); use `--private` to opt out of registration entirely. Unix only for now. diff --git a/src/Capacitor.Cli.Core/Resources/help-agent.txt b/src/Capacitor.Cli.Core/Resources/help-agent.txt index fbe324310a..031bf3b42e 100644 --- a/src/Capacitor.Cli.Core/Resources/help-agent.txt +++ b/src/Capacitor.Cli.Core/Resources/help-agent.txt @@ -24,8 +24,10 @@ Options for start: Options for stop: --all Stop every agent, including --private ones. Review and review-flow agents are skipped unless --force is given. - --force Include review and review-flow agents. Stopping a flow - participant mid-round leaves the flow without it. + --force Stop a single review or review-flow agent that would + otherwise be refused, or include them under --all. + Stopping a flow participant mid-round leaves the flow + without it. --yes, -y Skip the confirmation prompt for --all Options for ls, attach, stop: @@ -39,7 +41,8 @@ Review and flow agents: Agents the daemon runs as part of a review flow (KIND `review-flow`) or a PR review (`review`) are not yours to drive directly — they are addressed through the flow tools. `attach` gives you a read-only view of one: you see its output, - your input is not delivered. `stop` refuses unless you pass --force. + your input is not delivered, and your terminal size is not applied to it. + `stop` refuses unless you pass --force. Notes: The daemon owns the agent, not your terminal, so you can detach and the agent From 5ebba61c6920a0a363509ccdfc26c27411067a27 Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Wed, 29 Jul 2026 20:14:48 +0200 Subject: [PATCH 13/15] fix: address whole-branch review findings on flow-participant agent commands - stop --all --force now groups protected agents under their own labelled heading, matching the spec (the plan's snippet had it backwards; fixed the plan too so re-execution won't replay the defect) - the PID-record stop path (a prior-incarnation survivor) now honours Kind-based protection instead of reaping unconditionally; the decision reads the record via a new FindPidRecord accessor, keeping TryStopByPidRecordAsync itself policy-free for its server-origin caller - KindText/IsProtectedKind/ProtectionReason fail safe on an unrecognised LaunchKind instead of defaulting to unprotected - FrameCodec.StopV2 guards against a zero-length payload - ParseAgentRow and the ls fetch path restore the 3-column floor a stray tab in a repo path could otherwise defeat - doc/help/test fixes: help-agent.txt synopsis gets [--force], stale ls/AgentList column docs updated, and the tautological --force dispatch test is supplemented with one that can actually fail Closes #379, AI-1557 --- README.md | 2 +- ...7-flow-participant-aware-agent-commands.md | 13 +++-- src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs | 1 + src/Capacitor.Cli.Core/LocalIpc/FrameType.cs | 2 +- .../Resources/help-agent.txt | 8 +-- .../Services/AgentOrchestrator.LocalIpc.cs | 53 +++++++++++++++---- .../Services/AgentOrchestrator.cs | 16 ++++-- src/Capacitor.Cli/Commands/AgentCommand.cs | 24 ++++++--- .../AgentVerbDispatchTests.cs | 13 +++++ .../AgentOrchestratorLocalAttachTests.cs | 44 +++++++++++++++ 10 files changed, 145 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 5f5552ae81..f5cd5c468f 100644 --- a/README.md +++ b/README.md @@ -948,7 +948,7 @@ kcap agent start claude -d # start without attaching; prints ```bash kcap agent # no subcommand — same as `kcap agent ls` -kcap agent ls # list daemon-hosted agents (id, status, repo) +kcap agent ls # list daemon-hosted agents (id, status, kind, repo) kcap agent attach ab12 # re-attach your terminal (any unique id prefix works) kcap agent stop ab12 # graceful /exit, then terminate kcap agent stop --all -y # stop every agent this daemon hosts, no prompt diff --git a/docs/superpowers/plans/2026-07-29-ai1557-flow-participant-aware-agent-commands.md b/docs/superpowers/plans/2026-07-29-ai1557-flow-participant-aware-agent-commands.md index 9ef90762fe..4c973322c3 100644 --- a/docs/superpowers/plans/2026-07-29-ai1557-flow-participant-aware-agent-commands.md +++ b/docs/superpowers/plans/2026-07-29-ai1557-flow-participant-aware-agent-commands.md @@ -827,10 +827,15 @@ Replace the `--all` listing block (the `Console.WriteLine($"Found {agents.Count} } Console.WriteLine($"Found {targets.Length} agents:"); - foreach (var a in agents.Where(a => targets.Contains(a.Id))) Console.WriteLine($" • {a.Id} {a.Repo}"); - - if (!force && protectedIds.Length > 0) { - Console.WriteLine($"Skipping {protectedIds.Length} review agent(s) — pass --force to include them:"); + foreach (var a in agents.Where(a => targets.Contains(a.Id) && !protectedIds.Contains(a.Id))) + Console.WriteLine($" • {a.Id} {a.Repo}"); + + // The spec requires `--all --force` to show the blast radius too: both branches label + // protected rows with their Kind before the [y/N] prompt, not just the skip path. + if (protectedIds.Length > 0) { + Console.WriteLine(force + ? $"Including {protectedIds.Length} review agent(s):" + : $"Skipping {protectedIds.Length} review agent(s) — pass --force to include them:"); foreach (var a in agents.Where(a => protectedIds.Contains(a.Id))) Console.WriteLine($" • {a.Id} {a.Kind} {a.Repo}"); } diff --git a/src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs b/src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs index 6abb15ddeb..490dbf61d1 100644 --- a/src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs +++ b/src/Capacitor.Cli.Core/LocalIpc/FrameCodec.cs @@ -135,6 +135,7 @@ public static LocalFrame StopV2(bool force, string agentId) { return new(FrameType.StopV2) { Bytes = ms.ToArray(), Text = agentId }; } public static (bool force, string agentId) StopV2(LocalFrame f) { + Require(f.Bytes, 0, 1); var o = 1; return (f.Bytes[0] == 1, ReadLp(f.Bytes, ref o)); } diff --git a/src/Capacitor.Cli.Core/LocalIpc/FrameType.cs b/src/Capacitor.Cli.Core/LocalIpc/FrameType.cs index 2a6ca4edf3..578e53765c 100644 --- a/src/Capacitor.Cli.Core/LocalIpc/FrameType.cs +++ b/src/Capacitor.Cli.Core/LocalIpc/FrameType.cs @@ -18,7 +18,7 @@ public enum FrameType : byte { Stdout = 65, Exited = 66, Error = 67, - AgentList = 68, // UTF-8 table payload: one `id\tstatus\tcwd` line per agent + AgentList = 68, // UTF-8 table payload: one `id\tstatus\trepo\tkind\tflowRunId\tflowRole` line per agent RestartAck = 69, // acknowledgement for Restart (Text = short status) StopAck = 70, // acknowledgement for Stop (Text = one `id\tstatus` line per agent; status is "stopped", "skipped", or "failed") AttachedReadOnly = 71, // Attached for a protected agent: id + reason + snapshot, no input accepted diff --git a/src/Capacitor.Cli.Core/Resources/help-agent.txt b/src/Capacitor.Cli.Core/Resources/help-agent.txt index 031bf3b42e..bdaffd2a03 100644 --- a/src/Capacitor.Cli.Core/Resources/help-agent.txt +++ b/src/Capacitor.Cli.Core/Resources/help-agent.txt @@ -5,10 +5,12 @@ Usage: kcap agent Subcommands: start [flags] [-- ] Start an agent the daemon hosts for you - ls List this daemon's agents (id, status, repo) + ls List this daemon's agents (id, status, kind, repo) attach Attach your terminal to a running agent - stop Stop an agent (graceful /exit, then terminate) - stop --all [-y] Stop every agent this daemon hosts + stop [--force] + Stop an agent (graceful /exit, then terminate) + stop --all [-y] [--force] + Stop every agent this daemon hosts Running `kcap agent` with no subcommand lists agents. diff --git a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs index 1fc0b0c11b..1ed82a9113 100644 --- a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs +++ b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs @@ -14,11 +14,16 @@ public Task HandleLocalListAsync(Stream stream, CancellationToken ct) { } /// Wire spelling of . Kept separate from the enum name so the table - /// reads as a CLI column rather than a .NET identifier. + /// reads as a CLI column rather than a .NET identifier. A future kind that falls through the + /// switch reports its own enum name rather than masquerading as "agent" — the CLI's + /// `IsProtectedKind` then fails safe (protected) on anything it doesn't recognise, instead of + /// the daemon advertising an unprotected kind that its own `Kind != Default` checks would + /// still protect. static string KindText(LaunchKind kind) => kind switch { + LaunchKind.Default => "agent", LaunchKind.Review => "review", LaunchKind.ReviewFlow => "review-flow", - _ => "agent", + _ => kind.ToString(), }; /// @@ -77,8 +82,22 @@ await FrameCodec.WriteAsync(stream, LocalFrame.Error( } // Not live here — it may be a survivor of a previous daemon incarnation, which the PID - // record can still reap. This is why the client sends full ids verbatim. Kind is - // unknown for those, so protection cannot apply. + // record can still reap. This is why the client sends full ids verbatim. The record + // carries the same Kind/FlowRunId/FlowRole the live agent would have, so protection still + // applies — a review-flow survivor from a prior incarnation is refused exactly like a + // live one. TryStopByPidRecordAsync itself stays policy-free (it's shared with the + // server-origin HandleStopAgent path); the decision is made here, before it ever runs. + if (!force && FindPidRecord(agentId) is { Kind: not nameof(LaunchKind.Default) } record) { + var consequence = record.Kind == nameof(LaunchKind.ReviewFlow) + ? "Stopping it mid-round leaves the flow without a participant." + : "Stopping it discards the review before it can report back."; + + await FrameCodec.WriteAsync(stream, LocalFrame.Error( + $"{agentId} is a {ProtectionReason(record)}. {consequence} Pass --force to stop it anyway."), ct); + + return; + } + var reaped = await TryStopByPidRecordAsync(agentId); await FrameCodec.WriteAsync( @@ -185,13 +204,25 @@ public Task HandleLocalAttachAsync(string agentId, Stream stream, CancellationTo return AttachClientLoopAsync(agent, stream, ct, readOnly: agent.Kind != LaunchKind.Default); } - /// Human-readable "why is this read-only", carried on the AttachedReadOnly frame. - static string ProtectionReason(AgentInstance agent) { - var kind = agent.Kind == LaunchKind.ReviewFlow ? "review-flow" : "review"; - var role = string.IsNullOrEmpty(agent.FlowRole) ? "" : $", role {agent.FlowRole}"; - var flow = string.IsNullOrEmpty(agent.FlowRunId) ? "" : $" (flow {agent.FlowRunId}{role})"; - - return $"{kind} agent{flow}"; + /// Human-readable "why is this read-only/refused", carried on the AttachedReadOnly frame and + /// the not-live StopV2 refusal below (which reads the same kind from a persisted PID record + /// instead of a live AgentInstance). + static string ProtectionReason(AgentInstance agent) => ProtectionReason(agent.Kind.ToString(), agent.FlowRunId, agent.FlowRole); + + static string ProtectionReason(AgentPidRecord record) => ProtectionReason(record.Kind, record.FlowRunId, record.FlowRole); + + /// A kind this build doesn't recognise reports its own name rather than being mislabelled as + /// "review" — mirrors KindText's fail-safe shape. + static string ProtectionReason(string kind, string? flowRunId, string? flowRole) { + var label = kind switch { + nameof(LaunchKind.ReviewFlow) => "review-flow", + nameof(LaunchKind.Review) => "review", + _ => kind, + }; + var role = string.IsNullOrEmpty(flowRole) ? "" : $", role {flowRole}"; + var flow = string.IsNullOrEmpty(flowRunId) ? "" : $" (flow {flowRunId}{role})"; + + return $"{label} agent{flow}"; } /// diff --git a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs index a3a831bf78..2f032640df 100644 --- a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs +++ b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs @@ -719,11 +719,19 @@ internal void SeedPendingMarkerCandidateForTest(string agentId, string oldEpoch) /// on Unix, carries the expected KCAP_AGENT_ID env — ambiguity spares), reap it by identity /// and delete the record on confirmed death. This makes the server's registry-independent S2 stop /// effective even against a NEW daemon incarnation that never knew the agent in memory. - async Task TryStopByPidRecordAsync(string agentId) { - if (_pidRecords is null) return false; + /// Looks up a persisted PID record by agent id, or null if none exists. Read-only and + /// policy-free — shared by the reap below and by the local-stop protection check in + /// AgentOrchestrator.LocalIpc.cs, which decides whether to reap at all before this ever runs. + internal AgentPidRecord? FindPidRecord(string agentId) { + if (_pidRecords is null) return null; var record = _pidRecords.ReadAll().FirstOrDefault(r => r.AgentId == agentId); - if (record.AgentId != agentId) return false; // no record + + return record.AgentId == agentId ? record : null; + } + + async Task TryStopByPidRecordAsync(string agentId) { + if (FindPidRecord(agentId) is not { } record) return false; var confirmedGone = await ProcessReaper.ReapByRecordAsync(record, _logger, _shutdownCts.Token); if (confirmedGone) { @@ -733,7 +741,7 @@ async Task TryStopByPidRecordAsync(string agentId) { // boot's OrphanReaper record pass re-derives it and Upsert (idempotent on the source-stable // (AgentId, OldEpoch) key) collapses onto the committed entry, then completes the delete. _resolvedLedger?.Upsert(agentId, record.DaemonEpoch, record.FlowRunId, record.FlowRole); - _pidRecords.Delete(agentId); // delete ONLY on confirmed death (spec §6.4(2)) + _pidRecords?.Delete(agentId); // delete ONLY on confirmed death (spec §6.4(2)) } return confirmedGone; diff --git a/src/Capacitor.Cli/Commands/AgentCommand.cs b/src/Capacitor.Cli/Commands/AgentCommand.cs index cb5921ff56..a268f73ca1 100644 --- a/src/Capacitor.Cli/Commands/AgentCommand.cs +++ b/src/Capacitor.Cli/Commands/AgentCommand.cs @@ -180,10 +180,15 @@ static async Task StopAsync(string[] args) { } Console.WriteLine($"Found {targets.Length} agents:"); - foreach (var a in agents.Where(a => targets.Contains(a.Id))) Console.WriteLine($" • {a.Id} {a.Repo}"); - - if (!force && protectedIds.Length > 0) { - Console.WriteLine($"Skipping {protectedIds.Length} review agent(s) — pass --force to include them:"); + foreach (var a in agents.Where(a => targets.Contains(a.Id) && !protectedIds.Contains(a.Id))) + Console.WriteLine($" • {a.Id} {a.Repo}"); + + // Both branches label protected rows with their Kind before the [y/N] prompt, so the + // blast radius of --force is as visible as the default skip list is. + if (protectedIds.Length > 0) { + Console.WriteLine(force + ? $"Including {protectedIds.Length} review agent(s):" + : $"Skipping {protectedIds.Length} review agent(s) — pass --force to include them:"); foreach (var a in agents.Where(a => protectedIds.Contains(a.Id))) Console.WriteLine($" • {a.Id} {a.Kind} {a.Repo}"); } @@ -352,8 +357,11 @@ static async Task ListAsync(string[] args) { if (resp.Text.Length == 0) return []; + // A row needs at least id/status/repo — fewer columns is unparseable, not a short row + // to default-fill (a repo path containing a stray tab could otherwise shift the kind + // column and mislabel an agent). return [.. resp.Text.Split('\n') - .Where(l => l.Length > 0) + .Where(l => l.Length > 0 && l.Split('\t').Length >= 3) .Select(ParseAgentRow)]; } catch (Exception ex) when (ex is SocketException or IOException) { await Console.Error.WriteLineAsync($"kcap: cannot reach daemon: {ex.Message}"); @@ -446,8 +454,10 @@ internal static (string? Name, string? Error) DaemonNameFrom(string[] args) { } /// Kinds the CLI refuses to mutate by accident: a reviewer mid-round is not the user's to - /// type at or stop. Mirrors LaunchKind — anything that is not a plain agent is protected. - internal static bool IsProtectedKind(string kind) => kind is "review" or "review-flow"; + /// type at or stop. Mirrors LaunchKind — anything that is not a plain agent is protected, + /// including a kind this build doesn't recognise: an unrecognised kind fails safe rather + /// than reading as stoppable. + internal static bool IsProtectedKind(string kind) => kind is not "agent"; /// Splits an agent list into what `stop --all` will stop and what it will skip. internal static (string[] Stoppable, string[] Protected) PartitionByProtection(IReadOnlyList agents) => ( diff --git a/test/Capacitor.Cli.Tests.Integration/AgentVerbDispatchTests.cs b/test/Capacitor.Cli.Tests.Integration/AgentVerbDispatchTests.cs index 3fb4021f14..dd31551f94 100644 --- a/test/Capacitor.Cli.Tests.Integration/AgentVerbDispatchTests.cs +++ b/test/Capacitor.Cli.Tests.Integration/AgentVerbDispatchTests.cs @@ -97,6 +97,19 @@ public async Task Stop_accepts_the_force_flag_without_treating_it_as_an_id() { await Assert.That(stderr).DoesNotContain("usage: kcap agent stop"); } + [Test] + public async Task Stop_force_alone_without_all_or_an_id_still_reports_usage() { + // Neither --all nor a positional id is present, so the usage line must fire. If --force + // were ever mistreated as the positional agent id, hasId would flip true here and this + // would instead try to resolve "--force" as a target — the previous test above can't + // catch that regression because its args[0] is "--all", which already forces hasId false + // regardless of where --force sits. + var (_, stderr, exitCode) = await RunCli("agent stop --force --daemon kcap-dispatch-test-absent"); + + await Assert.That(stderr).Contains("usage: kcap agent stop"); + await Assert.That(exitCode).IsEqualTo(1); + } + static async Task<(string Stdout, string Stderr, int ExitCode)> RunCli( string argLine, bool clearServerUrl = false) { var binary = GetCliBinaryPath(); diff --git a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs index cfd313ef97..ec78529d53 100644 --- a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs @@ -7,6 +7,7 @@ using Capacitor.Cli.Daemon; using Capacitor.Cli.Daemon.Pty; using Capacitor.Cli.Daemon.Services; +using Capacitor.Cli.Tests.Unit.Daemon; using Microsoft.Extensions.Logging.Abstractions; namespace Capacitor.Cli.Tests.Unit; @@ -869,6 +870,49 @@ public async Task Stopping_a_non_flow_review_agent_without_force_is_refused_with await Assert.That(orch.GetAgentForTest("rev-1")!.Status).IsNotEqualTo("Completed"); } + [Test] + public async Task Stopping_a_prior_incarnation_flow_survivor_without_force_is_refused_before_reaping() { + // Not in _agents — this daemon incarnation never saw it — but its persisted PID record + // says it was a review-flow participant. The refusal must fire off the RECORD's Kind + // before TryStopByPidRecordAsync (and its live-process reap) ever runs. + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.WritePidRecordForTest(new AgentPidRecord( + "ghost-flow", 999_999, "", PidIdentityKind.IdentityUnavailable, "ReviewFlow", "codex", + "flow-7f3a", "reviewer", orch.DaemonIdForTest, orch.DaemonEpochForTest, DateTimeOffset.UtcNow)); + + var reply = await StopV2AndReadReply(orch, force: false, "ghost-flow"); + + await Assert.That(reply!.Type).IsEqualTo(FrameType.Error); + await Assert.That(reply.Text).Contains("review-flow"); + await Assert.That(reply.Text).Contains("--force"); + // Refused before any reap attempt — the record is untouched. + await Assert.That(orch.PidRecordsForTest().Any(r => r.AgentId == "ghost-flow")).IsTrue(); + } + + [Test] + public async Task Stopping_a_prior_incarnation_flow_survivor_with_force_bypasses_the_kind_gate() { + // --force must reach TryStopByPidRecordAsync itself (kept policy-free) rather than being + // turned back by the new gate above it. + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + + using var dummy = DummyProcess.StartSleep(30); + var pid = dummy.Pid; + var identity = ProcessIdentity.Capture(pid)!; + dummy.Kill(); dummy.WaitForExit(TimeSpan.FromSeconds(5)); // confirmed dead before the reap runs + + orch.WritePidRecordForTest(new AgentPidRecord( + "ghost-flow-2", pid, identity, PidIdentityKind.Present, "ReviewFlow", "codex", + "flow-7f3a", "reviewer", orch.DaemonIdForTest, orch.DaemonEpochForTest, DateTimeOffset.UtcNow)); + + var reply = await StopV2AndReadReply(orch, force: true, "ghost-flow-2"); + + await Assert.That(reply!.Type).IsEqualTo(FrameType.StopAck); + await Assert.That(reply.Text).IsEqualTo("ghost-flow-2\tstopped"); + await Assert.That(orch.PidRecordsForTest().Any(r => r.AgentId == "ghost-flow-2")).IsFalse(); + } + [Test] public async Task Stop_all_without_force_skips_protected_agents_and_says_so() { var server = new TripwireServerConnection(); From 81fcf3e898ca6bcd7a256fc61eb1a47df280e1f2 Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Thu, 30 Jul 2026 13:28:11 +0200 Subject: [PATCH 14/15] docs: correct the FindPidRecord summary and the ls row shape in top-level help MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The FindPidRecord extraction left the original XML summary attached to the new method, so it claimed to "reap it by identity and delete the record on confirmed death" — which it does not; it only reads. Moved back onto TryStopByPidRecordAsync, which does do that and was left undocumented. help-usage.txt still described `agent ls` as (id, status, repo); it grew a KIND column. This was the fourth instance of that stale text, the other three having been corrected already. Co-Authored-By: Claude Opus 5 --- src/Capacitor.Cli.Core/Resources/help-usage.txt | 2 +- src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Capacitor.Cli.Core/Resources/help-usage.txt b/src/Capacitor.Cli.Core/Resources/help-usage.txt index 34d5b0cee4..23d74084ca 100644 --- a/src/Capacitor.Cli.Core/Resources/help-usage.txt +++ b/src/Capacitor.Cli.Core/Resources/help-usage.txt @@ -28,7 +28,7 @@ Configuration: Agents: agent start [-d] [--worktree] [--private] [-- ] Start a daemon-hosted agent - agent ls List this daemon's agents (id, status, repo) + agent ls List this daemon's agents (id, status, kind, repo) agent attach Attach your terminal to a running agent (id prefix ok) agent stop | --all [-y] [--force] Stop one agent, or all of them diff --git a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs index 2f032640df..039edaa9da 100644 --- a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs +++ b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs @@ -714,11 +714,6 @@ internal ResolvedStartupCandidate SeedResolvedCandidateForTest(string agentId, s internal void SeedPendingMarkerCandidateForTest(string agentId, string oldEpoch) => _markerCandidates!.Write(new MarkerCandidate(agentId, _daemonId, oldEpoch, 999_999)); - /// Phase B (D4 §6.4(3) StopAgent fallback): the caller had no in-memory agent for - /// this id — consult the PID record and, if a live process still matches its EXACT identity (and, - /// on Unix, carries the expected KCAP_AGENT_ID env — ambiguity spares), reap it by identity - /// and delete the record on confirmed death. This makes the server's registry-independent S2 stop - /// effective even against a NEW daemon incarnation that never knew the agent in memory. /// Looks up a persisted PID record by agent id, or null if none exists. Read-only and /// policy-free — shared by the reap below and by the local-stop protection check in /// AgentOrchestrator.LocalIpc.cs, which decides whether to reap at all before this ever runs. @@ -730,6 +725,11 @@ internal void SeedPendingMarkerCandidateForTest(string agentId, string oldEpoch) return record.AgentId == agentId ? record : null; } + /// Phase B (D4 §6.4(3) StopAgent fallback): the caller had no in-memory agent for + /// this id — consult the PID record and, if a live process still matches its EXACT identity (and, + /// on Unix, carries the expected KCAP_AGENT_ID env — ambiguity spares), reap it by identity + /// and delete the record on confirmed death. This makes the server's registry-independent S2 stop + /// effective even against a NEW daemon incarnation that never knew the agent in memory. async Task TryStopByPidRecordAsync(string agentId) { if (FindPidRecord(agentId) is not { } record) return false; From 0501bf00ba4dded10cf69fe4f75e45d7d151262e Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Thu, 30 Jul 2026 14:15:15 +0200 Subject: [PATCH 15/15] Neutralise table delimiters in the agent list and refuse malformed rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AgentList` is a tab/newline-delimited table, and RepoPath, FlowRunId and FlowRole are free-form — a repo path may legally contain a tab or newline. Emitted raw, one shifts the reader's columns or splits the row. That is not merely cosmetic. The CLI keys `stop --all`'s confirmation off the kind column, so a shifted row makes a plain agent read as protected: the prompt says "Skipping 1 review agent", the user confirms N, and the daemon — which computes eligibility itself and is authoritative — stops N+1. The confirmation understates the blast radius, which is the property the prompt exists to convey. Closed at both layers. The daemon replaces tab/CR/LF with a space in the three free-form cells. The CLI accepts only exactly 3 columns (older daemon) or exactly 6 (current) and refuses the whole table otherwise, rather than acting on a guess. Found by Qodo on #408. Two earlier reviews rated this class Minor on the grounds that the daemon stays authoritative for the stop — true, but it is the prompt that the user consents to. Co-Authored-By: Claude Opus 5 --- .../Services/AgentOrchestrator.LocalIpc.cs | 11 +++++++- src/Capacitor.Cli/Commands/AgentCommand.cs | 20 +++++++++----- .../AgentOrchestratorLocalAttachTests.cs | 26 +++++++++++++++++++ 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs index 1ed82a9113..3686dacfed 100644 --- a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs +++ b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.LocalIpc.cs @@ -8,11 +8,20 @@ internal partial class AgentOrchestrator { /// Reply to a kcap agent ls request with a tab-separated agent table. public Task HandleLocalListAsync(Stream stream, CancellationToken ct) { var lines = _agents.Values.Select(a => - $"{a.Id}\t{a.Status}\t{a.RepoPath}\t{KindText(a.Kind)}\t{a.FlowRunId ?? ""}\t{a.FlowRole ?? ""}"); + $"{a.Id}\t{a.Status}\t{Cell(a.RepoPath)}\t{KindText(a.Kind)}\t{Cell(a.FlowRunId)}\t{Cell(a.FlowRole)}"); return FrameCodec.WriteAsync(stream, new LocalFrame(FrameType.AgentList) { Text = string.Join('\n', lines) }, ct); } + /// + /// Neutralises the table's own delimiters in a free-form field. A repo path may legally + /// contain a tab or newline, which would shift the reader's columns or split the row — and the + /// CLI keys `stop --all`'s confirmation off the kind column, so a shifted row understates the + /// blast radius the user is agreeing to. + /// + static string Cell(string? value) => + value is null ? "" : value.Replace('\t', ' ').Replace('\n', ' ').Replace('\r', ' '); + /// Wire spelling of . Kept separate from the enum name so the table /// reads as a CLI column rather than a .NET identifier. A future kind that falls through the /// switch reports its own enum name rather than masquerading as "agent" — the CLI's diff --git a/src/Capacitor.Cli/Commands/AgentCommand.cs b/src/Capacitor.Cli/Commands/AgentCommand.cs index a268f73ca1..1270dc6177 100644 --- a/src/Capacitor.Cli/Commands/AgentCommand.cs +++ b/src/Capacitor.Cli/Commands/AgentCommand.cs @@ -357,12 +357,20 @@ static async Task ListAsync(string[] args) { if (resp.Text.Length == 0) return []; - // A row needs at least id/status/repo — fewer columns is unparseable, not a short row - // to default-fill (a repo path containing a stray tab could otherwise shift the kind - // column and mislabel an agent). - return [.. resp.Text.Split('\n') - .Where(l => l.Length > 0 && l.Split('\t').Length >= 3) - .Select(ParseAgentRow)]; + // Exactly 3 columns is an older daemon; exactly 6 is a current one. Any other width + // means the row was corrupted in transit — most plausibly a delimiter inside a + // free-form field — and a shifted kind column would misreport what `stop --all` is + // about to do. Refuse the whole table rather than consent to a guess. + string[] rows = [.. resp.Text.Split('\n').Where(l => l.Length > 0)]; + + if (rows.Any(l => l.Split('\t').Length is not (3 or 6))) { + await Console.Error.WriteLineAsync( + "kcap: daemon sent a malformed agent table (unexpected column count); refusing to act on it"); + + return null; + } + + return [.. rows.Select(ParseAgentRow)]; } catch (Exception ex) when (ex is SocketException or IOException) { await Console.Error.WriteLineAsync($"kcap: cannot reach daemon: {ex.Message}"); diff --git a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs index ec78529d53..2d1002b2cf 100644 --- a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorLocalAttachTests.cs @@ -1003,4 +1003,30 @@ public async IAsyncEnumerable ReadOutputAsync([EnumeratorCancellation] C public void Resize(ushort _, ushort __) { } public void SendInterrupt() { } } + [Test] + public async Task Local_list_neutralises_delimiters_inside_a_free_form_field() { + // Repo paths and flow roles are free-form and may legally hold a tab or newline. Emitted + // raw they would shift the reader's columns or split the row, and the CLI keys + // `stop --all`'s confirmation off the kind column — so a corrupted row would understate + // the blast radius the user is agreeing to. + var server = new TripwireServerConnection(); + await using var orch = BuildOrchestrator(server, new SpyPtyProcessFactory(), new Dictionary()); + orch.SeedAgentForTest("tabby-1", kind: LaunchKind.ReviewFlow, + flowRunId: "flow\t7f3a", flowRole: "rev\niewer"); + + using var client = new DuplexTestStream(new MemoryStream(), new MemoryStream()); + await orch.HandleLocalListAsync(client, default); + client.WrittenStream.Position = 0; + var reply = await FrameCodec.ReadAsync(client.WrittenStream, default); + + var rows = reply!.Text.Split('\n'); + await Assert.That(rows.Length).IsEqualTo(1); + + var cols = rows[0].Split('\t'); + await Assert.That(cols.Length).IsEqualTo(6); + await Assert.That(cols[3]).IsEqualTo("review-flow"); + await Assert.That(cols[4]).IsEqualTo("flow 7f3a"); + await Assert.That(cols[5]).IsEqualTo("rev iewer"); + } + }