Phases A + B: CodingAgent protocol and per-session agent selection#197
Open
dhilgaertner wants to merge 2 commits intomainfrom
Open
Phases A + B: CodingAgent protocol and per-session agent selection#197dhilgaertner wants to merge 2 commits intomainfrom
dhilgaertner wants to merge 2 commits intomainfrom
Conversation
Lays down the behavior-preserving agent abstraction from Phase A: new CodingAgent/AgentKind/AgentRegistry/StateSignalSource/AgentStateTransition/ HookConfigWriter protocols in CrowCore, with ClaudeCodeAgent, ClaudeHookConfigWriter, and ClaudeHookSignalSource implementing them in CrowClaude. The hook-event RPC handler now applies AgentStateTransition values from the signal source instead of inlining the state machine, and HookConfigGenerator moved out of the main app. ClaudeState → AgentActivityState, .claudeLaunched → .agentLaunched, onLaunchClaude → onLaunchAgent. No user-visible change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase B of the agent abstraction. Session and AppConfig now carry an AgentKind — persisted, backward-compatible with existing JSON. The new CodingAgent.displayName / iconSystemName drive a picker in the new-session dialog and a "Default Agent" picker in Settings; both are visible but disabled until a second agent is registered (Phase C flips the bit by calling AgentRegistry.shared.register). The sidebar row gets a leading agent icon and the detail header gains a read-only "Agent: <name>" label (hidden for Manager, which stays pinned to Claude Code). The new-session RPC and crow CLI accept an --agent / agent_kind param. Codable tests cover the legacy-JSON-decodes-to-.claudeCode path for both Session and AppConfig so PRs preserving these models stay covered. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dgershman
approved these changes
Apr 22, 2026
Collaborator
dgershman
left a comment
There was a problem hiding this comment.
Code & Security Review
Critical Issues
None found.
Security Review
Strengths:
AgentHookEventstrips raw JSON payloads at theCrowCoreboundary, preventing JSON-value type leakage into the protocol layer.AgentKindis aRawRepresentablestruct (not an enum), so downstream packages can register agents without modifyingCrowCore— good extensibility without security surface expansion.- Backward-compatible
Codabledecoders (decodeIfPresentwith.claudeCodefallback) prevent crash-on-load from legacy data files. - Existing input validation on session names, path-traversal guards on worktree/terminal paths, and UUID validation are all preserved through the refactor.
AgentRegistryusesNSLockfor thread safety — appropriate for a low-contention singleton.
Concerns:
- None. The
ClaudeHookConfigWriterstill writes to.claude/settings.local.jsonwithin validated worktree paths. Hook commands are constructed from trustedcrowPath+ session UUID — no user-controlled injection vector.
Code Quality
Well done:
- The state machine extraction from AppDelegate into
ClaudeHookSignalSourceis clean. The 110-line inline switch is now a pure function returningAgentStateTransition, making it testable in isolation. 15 tests cover every branch. AgentStateTransitionuses a three-variant enum (leave/clear/set) for both notification and tool activity updates — expressive and prevents accidental state clobbering.- Manager session is correctly hardcoded to
.claudeCodeat construction time (SessionService.swift:248), never reading the config default. - UI pickers are disabled when only one agent is registered (
availableAgents.count < 2), preventing dead UI elements. - The
hook-eventhandler comment atAppDelegate.swift:887notes the Phase A limitation (always routes through default agent) — good breadcrumb for Phase C.
Minor observations (non-blocking):
AppDelegate.swift:887: The hook-event handler always usesAgentRegistry.shared.defaultAgent?.stateSignalSourcerather than looking up the session'sagentKind. The comment explains this is Phase A behavior, but when Phase C lands, this line will need to resolve per-session. Consider a// TODO(phase-c)marker.SessionService.swift:625andSessionService.swift:833:recoverOrphanandcreateReviewSessionboth setagentKind: appState.defaultAgentKind. This is correct behavior today but worth noting — recovered orphans will inherit the current default, not whatever agent they were originally created with (that info is lost).saveSettingsatAppDelegate.swift:434persists the new config but doesn't mirrordefaultAgentKindback toappState.defaultAgentKindthe way it does forremoteControlEnabledandhideSessionDetails. The Settings picker writes directly toconfig.defaultAgentKind, butappState.defaultAgentKindwill be stale until restart. Since theSettingsViewonChange callssave()which stores the config, the persisted value is correct — but any code readingappState.defaultAgentKind(like thenew-sessionRPC handler) will see the old value until app restart.
Summary Table
| Priority | Issue |
|---|---|
| 🟡 Yellow | saveSettings doesn't sync defaultAgentKind back to appState (stale until restart) |
| 🟢 Green | Add // TODO(phase-c) to hook-event handler's default-agent lookup |
| 🟢 Green | Orphan recovery inherits current default agent, not original — acceptable but worth documenting |
Recommendation: Approve. This is a well-structured, behavior-preserving refactor with thorough test coverage (15 signal-source tests + 7 Codable tests). The one yellow item (stale appState.defaultAgentKind after settings change) is low-impact since only one agent exists today, but should be fixed before Phase C when users can actually pick a different default.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #166 and #167. Parent spec: #150. Phase A is behavior-preserving; Phase B adds data-model/UI plumbing without exposing any new user-visible agents yet. Phase C (#168) flips a bit by registering a second agent in
AgentRegistry— no new call sites needed.Phase A — CodingAgent protocol (behavior-preserving) — closes #166
CodingAgent,AgentKind,AgentRegistry,StateSignalSource,AgentStateTransition,HookConfigWriterprotocols inCrowCore, plus anAgentHookEventpayload type soCrowCorestays free ofJSONValue.ClaudeCodeAgent,ClaudeHookConfigWriter(moved fromSources/Crow/App/HookConfigGenerator.swift), andClaudeHookSignalSourceinPackages/CrowClaude/; registersClaudeCodeAgentinAgentRegistry.sharedat app launch.AppDelegate.hook-event: the handler now flattens payload →AgentHookEvent, asks the agent's signal source for anAgentStateTransition, and applies it. 110-line switch becomes ~20 lines of apply-transition code.ClaudeState→AgentActivityState,TerminalReadiness.claudeLaunched→.agentLaunched,AppState.onLaunchClaude→onLaunchAgent,SessionHookState.claudeState→activityState.Sessionmodel change in this phase.Phase B — per-session agent selection plumbing — closes #167
Session.agentKind: AgentKind(persisted, defaults to.claudeCode, backward-compatible decoder).AppConfig.defaultAgentKind: AgentKind(top-level field, backward-compatible decoder, mirrored toAppState).CodingAgentgainsdisplayNameandiconSystemNameso UI can enumerate registered agents without baking names intoCrowCore.CreateSessionView: "Agent" picker seeded fromAppState.defaultAgentKind, disabled when only one agent is registered.SettingsView: "Default Agent" picker in the Defaults section, same single-option-disabled treatment.SessionRow: leading agent icon with tooltip.SessionDetailView: read-only "Agent:<displayName>" row (hidden for Manager, which stays pinned to Claude Code).crow new-sessionCLI grows--agent <kind>; RPC acceptsagent_kind..claudeCodeat construction — never reads the config default.Test plan
swift testat repo root passes (20 tests).swift testin every sub-package passes — 301 tests total (CrowCore 114, CrowClaude 23, CrowCLI 34, CrowGit 13, CrowIPC 32, CrowPersistence 23, CrowProvider 25, CrowUI 17, Crow/Tests 20).ClaudeHookSignalSourceTestscover every branch of the old AppDelegate switch (PreToolUse/AskUserQuestion, PreToolUse/other, PostToolUse, permission_prompt, idle_prompt, PermissionRequest-vs-question precedence, Stop, SessionStart/resume, SessionStart/fresh, SessionEnd, Task/Subagent preserving.waiting, blanket notification clear, unknown/PreCompact).agentKind/defaultAgentKinddecodes to.claudeCode), round-trip of custom raw values, and on-disk shape stability.<worktree>/.claude/settings.local.jsonis still written with all 17 hook entries; deleting a session removes them.crow new-session --name test --agent claude-codesucceeds and the returned JSON includesagent_kind.🤖 Generated with Claude Code