Repository navigation
Make the kcap agent commands flow-participant aware - #408
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…delivery 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.
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.
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.
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.
…ommands - 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
…evel help 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 <noreply@anthropic.com>
PR Summary by QodoMake
AI Description
Diagram
High-Level Assessment
Files changed (18)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
`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 <noreply@anthropic.com>
|
Fixed in Two earlier review passes rated this class Minor because the daemon computes eligibility itself and is authoritative for the stop — so a mis-parsed row cannot cause a wrong stop. That reasoning holds for the decision but not for the confirmation prompt, which is what the user actually consents to. Traced concretely: with a tab in a repo path, Closed at both layers, per the "preferable + minimal mitigation" split in the suggestion:
I did not take the full structured/length-prefixed payload option. The daemon is the only producer, sanitisation makes every reachable case safe, and the strict column check fails closed on anything else — so the remaining benefit is bounded while the cost is a wire-format change to a frame in the same unreleased PR. New test: Verification: unit 4238 passed / 42 failed (the pre-existing |
Confirmed —
The verification results also look consistent with the fix: integration and AOT checks pass, with only the documented pre-existing unit-test failures remaining. |
1 participant
Closes #379
AI-1557
kcap agent ls|attach|stoptreated every daemon-hosted agent identically, so a review-flow participant — a reviewer mid-round — was indistinguishable from an agent you started.attachhanded you raw PTY stdin on it, andstop --allkilled it silently. This teaches the commands the difference.What changed
kcap agent lsgains aKINDcolumn:For
reviewandreview-flowagents:attachis read-only. Output streams so you can watch a reviewer work; your keystrokes are not delivered, and your terminal size is not applied to it — a read-only viewer never entersClientDims, so it cannot shrink the participants terminal through the min-clamp.stopis refused unless--force, with a message naming the flow.stop --allskips them and reports how many, rather than silently omitting them.--forceincludes them and lists them under their own heading in the confirmation, so the destructive variant shows more information than the safe one.Enforcement is daemon-side for both, so a current client cannot bypass it.
Wire changes
FrameTypeis append-only.StopV2 = 10carries a force flag;AttachedReadOnly = 71carries id + reason + snapshot.AgentListrows grow toid⇥status⇥repo⇥kind⇥flowRunId⇥flowRole;StopAckgains askippedstatus.AttachedReadOnlyis a separate frame rather than a flag onAttachedbecauseAttacheds payload ends with an unbounded snapshot — a trailing flag would be painted onto the users terminal instead of parsed. It also fails closed: an older CLI cannot decode frame 71, so it errors rather than pumping stdin at a participant.Version skew — please read
ls/attachdegrade silently to unprotected.stopdoes not degrade — it hard-fails, because the CLI sendsStopV2which that daemon cannot decode, and tells you to restart it.Stopframe, which has no force concept, so the daemon treats it as--force. An old client can therefore silently force-stop a protected agent. Frame 8 postdates v0.11.8, so no released client is affected — but it is a real hole and it is documented in the README rather than glossed.Known limitations
--forcestill leaves the flow unaware its participant was stopped. Attributing that needs server-side work.LaunchKind.Default) are deliberately not protected — they are your own work by another route.Verification
CodexHookCommandTests/uninstall-config.tomlbaseline confirmed at the merge base. Zero new.main(13 commits ahead) and tested there: plainmainfails 45 unit tests, the merged result fails the same 45 with 21 more tests; integration 162/162 solo. So the merge is semantically clean, not merely conflict-free — a check worth doing explicitly after Group the agent commands underkcap agent#383 auto-merged cleanly into a guard that shadowed it.Design and plan
docs/superpowers/specs/2026-07-29-ai1557-flow-participant-aware-agent-commands-design.mddocs/superpowers/plans/2026-07-29-ai1557-flow-participant-aware-agent-commands.md🤖 Generated with Claude Code