diff --git a/.beads/interactions.jsonl b/.beads/interactions.jsonl --- a/.beads/interactions.jsonl +++ b/.beads/interactions.jsonl @@ -90,3 +90,4 @@ {"id":"int-1f1c3a07","kind":"field_change","created_at":"2026-07-01T08:52:06.275727261Z","actor":"dawn","issue_id":"klbr-5h3","extra":{"field":"status","new_value":"closed","old_value":"in_progress","reason":"Re-exported core DTO types from klbr-ipc and removed daemon field-by-field mappers"}} {"id":"int-623e8776","kind":"field_change","created_at":"2026-07-01T08:54:40.646821049Z","actor":"dawn","issue_id":"klbr-2kc","extra":{"field":"status","new_value":"closed","old_value":"in_progress","reason":"Collapsed KDL optional-node parser helpers and centralized serde integer deserializers"}} {"id":"int-4cb60a18","kind":"field_change","created_at":"2026-07-01T09:02:05.887375856Z","actor":"dawn","issue_id":"klbr-3ha","extra":{"field":"status","new_value":"closed","old_value":"in_progress","reason":"Refactored shared SSE and media data-url parsing in models.rs; added focused coverage and verified klbr-core tests."}} +{"id":"int-d2a36a78","kind":"field_change","created_at":"2026-07-01T09:11:34.33603338Z","actor":"dawn","issue_id":"klbr-647","extra":{"field":"status","new_value":"closed","old_value":"in_progress","reason":"Migrated klbr-core Message.role to a Role enum with lowercase serde compatibility and updated core role checks; verified workspace tests."}} diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -51,7 +51,7 @@ {"_type":"issue","id":"klbr-2kc","title":"core: reduce KDL config parser boilerplate in parser.rs","description":"parser.rs contains ~20 copy-pasted optional_*_node helpers that share identical structure. Simplify with macros or generic helpers. Also resolve duplicate deserialize_usize.","status":"closed","priority":2,"issue_type":"task","assignee":"dawn","owner":"90008@klbr.net","created_at":"2026-06-30T21:39:55Z","created_by":"dawn","updated_at":"2026-07-01T08:54:41Z","started_at":"2026-07-01T08:52:25Z","closed_at":"2026-07-01T08:54:41Z","close_reason":"Collapsed KDL optional-node parser helpers and centralized serde integer deserializers","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"klbr-zue","title":"core: fix shell.rs unsafe byte-slice truncation","description":"shell.rs truncates command output using byte offsets on a lossy UTF-8 string (e.g. stdout[..20_000]), which can panic if slicing in the middle of a multi-byte character. Truncate by char count or indices instead.","status":"closed","priority":2,"issue_type":"task","assignee":"dawn","owner":"90008@klbr.net","created_at":"2026-06-30T21:39:48Z","created_by":"dawn","updated_at":"2026-06-30T22:03:16Z","started_at":"2026-06-30T22:02:13Z","closed_at":"2026-06-30T22:03:16Z","close_reason":"Replaced shell stdout/stderr byte slicing with char-boundary truncation helper and added UTF-8 regression tests. Verified cargo test -p klbr-core shell_truncation, full cargo test -p klbr-core (168 passed, 1 ignored), cargo fmt --check, git diff --check, and rg confirms the unsafe stdout/stderr slices are gone.","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"klbr-3ha","title":"core: refactor models.rs media stripping and SSE parser duplication","description":"strip_media_urls and replace_media_urls_with_placeholders share near-identical regex parsing logic. Also, complete() and stream() duplicate manual SSE chunk parsing. Clean these up.","status":"closed","priority":2,"issue_type":"task","assignee":"dawn","owner":"90008@klbr.net","created_at":"2026-06-30T21:39:42Z","created_by":"dawn","updated_at":"2026-07-01T09:02:06Z","started_at":"2026-07-01T08:55:00Z","closed_at":"2026-07-01T09:02:06Z","close_reason":"Refactored shared SSE and media data-url parsing in models.rs; added focused coverage and verified klbr-core tests.","dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"klbr-647","title":"core: migrate Message.role from String to Role enum","description":"Message.role is currently represented as a raw String, which leads to fragile string comparisons ('assistant', 'tool', etc.) all over the codebase. Migrate to a proper Role enum.","status":"open","priority":2,"issue_type":"task","owner":"90008@klbr.net","created_at":"2026-06-30T21:39:36Z","created_by":"dawn","updated_at":"2026-06-30T21:39:36Z","dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"klbr-647","title":"core: migrate Message.role from String to Role enum","description":"Message.role is currently represented as a raw String, which leads to fragile string comparisons ('assistant', 'tool', etc.) all over the codebase. Migrate to a proper Role enum.","status":"closed","priority":2,"issue_type":"task","assignee":"dawn","owner":"90008@klbr.net","created_at":"2026-06-30T21:39:36Z","created_by":"dawn","updated_at":"2026-07-01T09:11:34Z","started_at":"2026-07-01T09:03:58Z","closed_at":"2026-07-01T09:11:34Z","close_reason":"Migrated klbr-core Message.role to a Role enum with lowercase serde compatibility and updated core role checks; verified workspace tests.","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"klbr-so2","title":"core: decompose agent.rs run_turn() god method","description":"agent.rs run_turn() is 500+ lines, managing execution, tool loops, preemption, and discord integration. Break down tool execution loops and phase handlers into separate functions.","status":"open","priority":2,"issue_type":"task","owner":"90008@klbr.net","created_at":"2026-06-30T21:39:29Z","created_by":"dawn","updated_at":"2026-06-30T21:39:29Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"klbr-ow7","title":"core: refactor agent.rs run() loop failure-handling duplication","description":"agent.rs run() loop copy-pastes the exact same ~30-line backoff/failure-handling parsing block 4 times across different event stream branches. Extract to a helper function.","status":"open","priority":2,"issue_type":"task","owner":"90008@klbr.net","created_at":"2026-06-30T21:39:23Z","created_by":"dawn","updated_at":"2026-06-30T21:39:23Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"klbr-7e0","title":"core: clean up dead tool files in src/tools/","description":"There are 12 unused tool files in klbr-core/src/tools/ (remember.rs, recall.rs, tag_memory.rs, etc.) that are not declared as submodules or registered. They should be deleted.","status":"closed","priority":2,"issue_type":"task","assignee":"dawn","owner":"90008@klbr.net","created_at":"2026-06-30T21:39:16Z","created_by":"dawn","updated_at":"2026-06-30T22:08:24Z","started_at":"2026-06-30T22:05:01Z","closed_at":"2026-06-30T22:08:24Z","close_reason":"Removed unregistered old memory tool modules and updated memory docs","dependency_count":0,"dependent_count":0,"comment_count":0} diff --git a/klbr-core/src/agent.rs b/klbr-core/src/agent.rs --- a/klbr-core/src/agent.rs +++ b/klbr-core/src/agent.rs @@ -12,7 +12,7 @@ harness_block::{self, HarnessBlock, DEFAULT_FORMAT_STYLE}, interrupt::Interrupt, memory::MemoryStore, - models::{LlmClient, LlmEvent, Message}, + models::{LlmClient, LlmEvent, Message, Role}, pipeline::{BenchQuery, ContextBudget, MemoryPipeline}, router::{RouteDecision, Router}, tools::{self, ToolContext}, @@ -242,7 +242,9 @@ let stream_failed = ctx .turns .last() - .map(|m| m.role == "assistant" && m.content_str().starts_with("error:")) + .map(|m| { + m.role == Role::Assistant && m.content_str().starts_with("error:") + }) .unwrap_or(false); if stream_failed { failure_count += 1; @@ -285,7 +287,7 @@ let stream_failed = ctx .turns .last() - .map(|m| m.role == "assistant" && m.content_str().starts_with("error:")) + .map(|m| m.role == Role::Assistant && m.content_str().starts_with("error:")) .unwrap_or(false); if stream_failed { failure_count += 1; @@ -338,7 +340,9 @@ let stream_failed = ctx .turns .last() - .map(|m| m.role == "assistant" && m.content_str().starts_with("error:")) + .map(|m| { + m.role == Role::Assistant && m.content_str().starts_with("error:") + }) .unwrap_or(false); if stream_failed { failure_count += 1; @@ -383,7 +387,7 @@ let stream_failed = ctx .turns .last() - .map(|m| m.role == "assistant" && m.content_str().starts_with("error:")) + .map(|m| m.role == Role::Assistant && m.content_str().starts_with("error:")) .unwrap_or(false); if stream_failed { failure_count += 1; @@ -436,7 +440,9 @@ let stream_failed = ctx .turns .last() - .map(|m| m.role == "assistant" && m.content_str().starts_with("error:")) + .map(|m| { + m.role == Role::Assistant && m.content_str().starts_with("error:") + }) .unwrap_or(false); if stream_failed { failure_count += 1; @@ -1259,8 +1265,8 @@ fn clean_failed_turns(ctx: &mut Context) { while let Some(last) = ctx.turns.last() { let content = last.content_str(); - let is_error = last.role == "assistant" && content.starts_with("error:"); - let is_nudge = last.role == "user" + let is_error = last.role == Role::Assistant && content.starts_with("error:"); + let is_nudge = last.role == Role::User && (content.contains("time since last event:") || content.contains("time check:") || content.contains("you wrote plain") @@ -1272,7 +1278,7 @@ } } if let Some(last) = ctx.turns.last() { - if last.role == "user" { + if last.role == Role::User { tracing::info!("evicting problematic user turn: {:?}", last.content_str()); ctx.turns.pop(); } @@ -1283,10 +1289,10 @@ let Some(last) = ctx.turns.last() else { return false; }; - if last.role == "assistant" { + if last.role == Role::Assistant { return false; } - if last.role == "tool" && is_completed_wait_tool_result(ctx) { + if last.role == Role::Tool && is_completed_wait_tool_result(ctx) { return false; } true @@ -1296,7 +1302,7 @@ let Some(last) = ctx.turns.last() else { return false; }; - if last.role != "tool" || !is_wait_timeout_without_event(last.content_str()) { + if last.role != Role::Tool || !is_wait_timeout_without_event(last.content_str()) { return false; } let Some(tool_call_id) = last.tool_call_id.as_deref() else { @@ -1307,7 +1313,7 @@ .iter() .rev() .skip(1) - .find(|msg| msg.role == "assistant") + .find(|msg| msg.role == Role::Assistant) .and_then(|msg| msg.tool_calls.as_ref()) .is_some_and(|calls| { calls @@ -1581,7 +1587,7 @@ } if !recollection.is_empty() { - if ctx.turns.first().map(|m| m.role.as_str()) == Some("assistant") { + if ctx.turns.first().map(|m| &m.role) == Some(&Role::Assistant) { let first = &mut ctx.turns[0]; let new_content = if first.content_str().is_empty() { recollection.clone() @@ -1740,7 +1746,7 @@ } fn is_reflection_maintenance_message(message: &Message) -> bool { - matches!(message.role.as_str(), "reflection") + message.role == Role::Reflection || message.content.as_deref().is_some_and(|content| { let trimmed = content.trim_start(); trimmed.starts_with("") @@ -1965,7 +1971,7 @@ } } else { msgs.push(Message { - role: "assistant".into(), + role: Role::Assistant, content, reasoning_content: reasoning, tool_calls: None, @@ -2061,6 +2067,7 @@ compaction_source_messages, compaction_transcript, complete_compaction_recollection, needs_assistant_response, recall_evidence_packets, COMPACTION_ASSISTANT_PREFILL, }; + use crate::models::Role; use anyhow::Result; use chrono::{TimeZone, Utc}; use tempfile::NamedTempFile; @@ -2356,7 +2363,7 @@ "\n\nreflect over this", ), Message { - role: "reflection".to_string(), + role: Role::Reflection, content: Some("logged reflection".to_string()), reasoning_content: None, tool_calls: None, diff --git a/klbr-core/src/context.rs b/klbr-core/src/context.rs --- a/klbr-core/src/context.rs +++ b/klbr-core/src/context.rs @@ -5,7 +5,7 @@ use chrono::{SecondsFormat, TimeZone, Utc}; use crate::memory::MemoryStore; -use crate::models::{Message, ToolCall}; +use crate::models::{Message, Role, ToolCall}; use rusqlite::{params, OptionalExtension}; #[derive(Debug, Clone, PartialEq, Eq)] @@ -120,7 +120,7 @@ "compaction" => self.load_compaction_turn(entry), "user" | "assistant" | "system" => { self.turns.push(Message { - role: entry.role.clone(), + role: Role::from(entry.role.as_str()), content: Some(entry.content.clone()), reasoning_content: entry.reasoning.clone(), tool_calls: None, @@ -223,7 +223,7 @@ pub fn push_assistant(&mut self, content: &str, reasoning: Option<&str>) { self.turns.push(Message { - role: "assistant".into(), + role: Role::Assistant, content: Some(content.to_string()), reasoning_content: reasoning.map(|s| s.to_string()), tool_calls: None, @@ -282,8 +282,8 @@ // find a safe cut point that doesn't split a tool call sequence let mut safe_cut = cut_index; while safe_cut < self.turns.len() { - let role = self.turns[safe_cut].role.as_str(); - if role == "user" || role == "assistant" { + let role = &self.turns[safe_cut].role; + if matches!(role, Role::User | Role::Assistant) { break; } safe_cut += 1; @@ -1090,7 +1090,7 @@ fn scan_visible_refs(messages: &[Message]) -> Vec<(String, RetrievalLane)> { let mut found = Vec::new(); - let last_user_idx = messages.iter().rposition(|m| m.role == "user"); + let last_user_idx = messages.iter().rposition(|m| m.role == Role::User); for (idx, m) in messages.iter().enumerate() { let Some(content) = &m.content else { @@ -1508,7 +1508,7 @@ pub fn as_messages_with_refs(&self, memory: &MemoryStore) -> Vec { let mut messages: Vec = self.system.iter().chain(&self.turns).cloned().collect(); - let last_user_idx = messages.iter().rposition(|m| m.role == "user"); + let last_user_idx = messages.iter().rposition(|m| m.role == Role::User); let Some(idx) = last_user_idx else { return messages; }; diff --git a/klbr-core/src/models.rs b/klbr-core/src/models.rs --- a/klbr-core/src/models.rs +++ b/klbr-core/src/models.rs @@ -2,9 +2,10 @@ use futures::StreamExt; use reqwest::{Client, RequestBuilder}; use rusqlite::OptionalExtension; -use serde::{Deserialize, Serialize}; +use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; use serde_json::{json, Map, Value}; use std::collections::{BTreeMap, HashMap}; +use std::fmt; use std::path::Path; use std::sync::{Arc, Mutex}; use tokio::sync::mpsc; @@ -25,9 +26,94 @@ pub function: ToolCallFunction, } +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum Role { + System, + User, + Assistant, + Tool, + Reflection, + Compaction, + Other(String), +} + +impl Role { + pub fn as_str(&self) -> &str { + match self { + Self::System => "system", + Self::User => "user", + Self::Assistant => "assistant", + Self::Tool => "tool", + Self::Reflection => "reflection", + Self::Compaction => "compaction", + Self::Other(role) => role, + } + } +} + +impl From<&str> for Role { + fn from(role: &str) -> Self { + match role { + "system" => Self::System, + "user" => Self::User, + "assistant" => Self::Assistant, + "tool" => Self::Tool, + "reflection" => Self::Reflection, + "compaction" => Self::Compaction, + other => Self::Other(other.to_string()), + } + } +} + +impl From for Role { + fn from(role: String) -> Self { + Self::from(role.as_str()) + } +} + +impl PartialEq<&str> for Role { + fn eq(&self, other: &&str) -> bool { + self.as_str() == *other + } +} + +impl PartialEq for &str { + fn eq(&self, other: &Role) -> bool { + *self == other.as_str() + } +} + +impl fmt::Display for Role { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.as_str()) + } +} + +impl Serialize for Role { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(self.as_str()) + } +} + +impl<'de> Deserialize<'de> for Role { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let role = String::deserialize(deserializer)?; + if role.is_empty() { + return Err(de::Error::custom("message role cannot be empty")); + } + Ok(Self::from(role)) + } +} + #[derive(Clone, Debug, Serialize, Deserialize)] pub struct Message { - pub role: String, + pub role: Role, #[serde(skip_serializing_if = "Option::is_none")] pub content: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -41,7 +127,7 @@ impl Message { pub fn system(s: impl Into) -> Self { Self { - role: "system".into(), + role: Role::System, content: Some(s.into()), reasoning_content: None, tool_calls: None, @@ -51,7 +137,7 @@ pub fn user(s: impl Into) -> Self { Self { - role: "user".into(), + role: Role::User, content: Some(s.into()), reasoning_content: None, tool_calls: None, @@ -61,7 +147,7 @@ pub fn assistant(s: impl Into) -> Self { Self { - role: "assistant".into(), + role: Role::Assistant, content: Some(s.into()), reasoning_content: None, tool_calls: None, @@ -75,7 +161,7 @@ reasoning_content: Option, ) -> Self { Self { - role: "assistant".into(), + role: Role::Assistant, content, reasoning_content, tool_calls: Some(calls), @@ -85,7 +171,7 @@ pub fn tool_result(id: impl Into, content: impl Into) -> Self { Self { - role: "tool".into(), + role: Role::Tool, content: Some(content.into()), reasoning_content: None, tool_calls: None, @@ -98,7 +184,7 @@ } fn normalize_for_api(&mut self) -> bool { - if self.role != "assistant" { + if self.role != Role::Assistant { return true; } @@ -1666,6 +1752,26 @@ assert_eq!(usage.total_tokens, 5); } + #[test] + fn role_serde_preserves_wire_strings() { + let value = serde_json::to_value(Message::tool_result("call_1", "ok")).unwrap(); + assert_eq!(value["role"], "tool"); + + let reflection: Message = serde_json::from_value(json!({ + "role": "reflection", + "content": "logged reflection" + })) + .unwrap(); + assert_eq!(reflection.role, Role::Reflection); + + let custom: Message = serde_json::from_value(json!({ + "role": "provider-special", + "content": "kept" + })) + .unwrap(); + assert_eq!(custom.role.as_str(), "provider-special"); + } + async fn spawn_http_response(status: &str, content_type: &str, body: &str) -> String { let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let addr = listener.local_addr().unwrap(); @@ -1765,7 +1871,7 @@ #[test] fn prepare_messages_drops_thought_only_assistant_messages() { let prepared = prepare_messages_for_api(&[Message { - role: "assistant".into(), + role: Role::Assistant, content: None, reasoning_content: Some("thinking".into()), tool_calls: None, @@ -1778,7 +1884,7 @@ #[test] fn prepare_messages_drops_blank_assistant_messages() { let prepared = prepare_messages_for_api(&[Message { - role: "assistant".into(), + role: Role::Assistant, content: Some(" ".into()), reasoning_content: None, tool_calls: None, @@ -1791,7 +1897,7 @@ #[test] fn prepare_messages_drops_blank_content_with_reasoning() { let prepared = prepare_messages_for_api(&[Message { - role: "assistant".into(), + role: Role::Assistant, content: Some(" ".into()), reasoning_content: Some("kept".into()), tool_calls: None,