Skip to content

refactor: extract shared kild-protocol crate for IPC message types#375

Merged
Wirasm merged 2 commits into
mainfrom
kild/328-kild-protocol-crate
Feb 11, 2026
Merged

refactor: extract shared kild-protocol crate for IPC message types#375
Wirasm merged 2 commits into
mainfrom
kild/328-kild-protocol-crate

Conversation

@Wirasm

@Wirasm Wirasm commented Feb 11, 2026

Copy link
Copy Markdown
Owner

Summary

  • Extract ClientMessage, DaemonMessage, and SessionInfo wire types from kild-daemon into a new lightweight kild-protocol crate (serde + serde_json only)
  • Replace manual json!() construction and .get() chains in kild-core and kild-tmux-shim with typed enum construction and pattern matching
  • Protocol changes are now compile-checked across all three consumers — no more silent drift

Changes

Crate What changed
kild-protocol (new) ClientMessage, DaemonMessage, SessionInfo + 18 serde roundtrip tests
kild-daemon Re-exports types from kild-protocol, removed moved type definitions
kild-core send_request() accepts &ClientMessage, returns DaemonMessage
kild-tmux-shim Same typed rewrite for all IPC functions

Test plan

  • cargo fmt --check — clean
  • cargo clippy --all -- -D warnings — 0 warnings
  • cargo test --all — 1655+ tests pass, 0 failures
  • cargo build --all — full workspace builds
  • Wire format unchanged (existing roundtrip tests prove this)

Closes #328

Extract ClientMessage, DaemonMessage, and SessionInfo wire types from
kild-daemon into a new kild-protocol crate with only serde/serde_json
dependencies. All three consumers (kild-daemon, kild-core, kild-tmux-shim)
now import typed enums from kild-protocol instead of hand-crafting JSON,
eliminating protocol drift across three independent implementations.

- kild-protocol: new crate with typed enums and serde roundtrip tests
- kild-daemon: re-exports types from kild-protocol (zero behavior change)
- kild-core: replace json!() + .get() chains with typed construction/matching
- kild-tmux-shim: same typed rewrite for all IPC functions
@Wirasm

Wirasm commented Feb 11, 2026

Copy link
Copy Markdown
Owner Author

PR Review Summary

Critical Issues (2 found)

Agent Issue Location
silent-failure-hunter read_scrollback: base64 decode failure silently returns empty data via .unwrap_or_default() crates/kild-core/src/daemon/client.rs:421-423
silent-failure-hunter read_scrollback: unexpected response type silently returns Ok(Some(Vec::new())) instead of error crates/kild-core/src/daemon/client.rs:426

Important Issues (2 found)

Agent Issue Location
silent-failure-hunter get_session_info: unexpected response type silently returns Ok(None) (looks like "not found") crates/kild-core/src/daemon/client.rs:388
silent-failure-hunter get_session_status: unexpected response logged at debug level but returns Ok(None) with misleading "completed" event name crates/kild-core/src/daemon/client.rs:333-339

Suggestions (2 found)

Agent Suggestion Location
type-design-analyzer SessionInfo.status should be a typed enum (e.g. SessionStatus::Running { pty_pid } / SessionStatus::Stopped { exit_code }) to make illegal states unrepresentable crates/kild-protocol/src/types.rs:13
type-design-analyzer DaemonMessage::Error.code should be a typed enum instead of String to eliminate brittle string matching in client code crates/kild-protocol/src/messages.rs:153-158

Strengths

  • Clean extraction of wire types into zero-dependency protocol crate (serde only)
  • Correct dependency graph — no circular deps, kild-daemon re-exports from kild-protocol
  • Comprehensive serde roundtrip tests (18 tests) covering all message variants + edge cases
  • Typed enum construction eliminates entire classes of runtime bugs (typos, missing fields, type coercion)
  • CLAUDE.md updates are accurate and complete
  • Existing 14 integration tests provide behavioral regression safety

Documentation Updates

  • CLAUDE.md — Already updated with kild-protocol crate description, build commands, and architecture notes. All changes verified accurate.

Agent Results

Agent Verdict
code-reviewer PASS — no high-confidence issues
docs-impact-agent No updates needed — documentation accurate
silent-failure-hunter 2 CRITICAL + 2 HIGH — silent failures in client.rs
pr-test-analyzer GOOD COVERAGE — no additional tests required
type-design-analyzer B+ (7/10) — adequate, SessionInfo status typing recommended

Verdict

NEEDS FIXES — 2 critical silent failures in read_scrollback must be addressed before merge. The base64 .unwrap_or_default() and catch-all Ok(Some(Vec::new())) both hide real errors. Note: the shim's read_scrollback already handles base64 decode correctly — match that pattern.

Recommended Actions

  1. Fix read_scrollback base64 decode: replace .unwrap_or_default() with .map_err() (match shim pattern)
  2. Fix read_scrollback catch-all: return ProtocolError for unexpected response types instead of empty vec
  3. Fix get_session_info and get_session_status catch-all arms to return errors (or at minimum warn! with _failed event name)
  4. Consider typed SessionStatus enum for SessionInfo in a follow-up

Replace stringly-typed fields with compile-checked enums:

- SessionInfo.status: String → SessionStatus enum (Creating, Running, Stopped)
- DaemonMessage::Error.code: String → ErrorCode enum with #[serde(other)] fallback
- DaemonClientError::DaemonError now carries ErrorCode for typed error matching

Fix silent failures in daemon client (kild-core):
- read_scrollback: base64 decode errors now surface instead of unwrap_or_default()
- read_scrollback: unexpected response types return ProtocolError instead of empty vec
- get_session_info: unexpected responses return ProtocolError instead of Ok(None)
- get_session_status: unexpected responses warn + return ProtocolError instead of
  silently returning Ok(None) with misleading "completed" event name

Wire format unchanged — serde rename_all ensures backward compatibility.
@Wirasm Wirasm merged commit 0f85e77 into main Feb 11, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: extract shared kild-protocol crate for IPC message types

1 participant