Skip to content

refactor/typed output 2602#167

Merged
emesal merged 22 commits intodevfrom
refactor/typed-output-2602
Feb 19, 2026
Merged

refactor/typed output 2602#167
emesal merged 22 commits intodevfrom
refactor/typed-output-2602

Conversation

@emesal
Copy link
Owner

@emesal emesal commented Feb 19, 2026

  • refactor(sink): replace Diagnostic with typed ResponseEvent variants
  • refactor(send): emit typed ResponseEvent variants, remove verbose gating
  • refactor(cli/sink): handle typed ResponseEvent variants with local filtering
  • refactor(json/sink): emit all typed ResponseEvent variants as structured JSONL
  • refactor(core): remove verbose from PromptOptions
  • refactor(config): remove verbose, hide_tool_calls, show_thinking from core; add CommandEvent
  • feat(cli/config): add verbose, hide_tool_calls, show_thinking to CliConfig; wire CLI flags
  • refactor(json): remove verbose load-time handling, emit CommandEvent
  • refactor(output): remove diagnostic/diagnostic_always from OutputSink
  • refactor(load): replace LoadOptions.verbose with typed load events via OutputSink
  • docs: update configuration.md and cli-reference.md for presentation layer refactor
  • chore: fmt cleanup and add plan docs

… core; add CommandEvent

Presentation fields no longer live in chibi-core config. Core emits all
events unconditionally; clients filter. Adds CommandEvent enum and
emit_event to OutputSink trait.
…onfig; wire CLI flags

Presentation fields now live in CliConfig / CLI ResolvedConfig.
ChibiInput gains verbose_flag, hide_tool_calls_flag, show_thinking_flag.
CLI tests updated to assert on input flags rather than config_overrides.
Replaces diagnostic() calls with emit_event(CommandEvent::ContextLoaded).
All callers migrated to direct eprintln! or emit_event. The two trait
methods and their impls in OutputHandler and JsonOutputSink are gone.
…a OutputSink

Adds McpToolsLoaded, McpBridgeUnavailable, LoadSummary CommandEvent variants.
load_with_options now takes &dyn OutputSink and emits these unconditionally;
clients filter based on verbose mode. LoadOptions.verbose removed. Resolves #166.
…ayer refactor

verbose, hide_tool_calls, show_thinking moved from core config.toml to cli.toml.
JSON mode overrides list updated to reflect that presentation fields are CLI-only.
cargo fmt reformatting across modified files. Adds the plan and handoff
documents from this refactor session.
@emesal
Copy link
Owner Author

emesal commented Feb 19, 2026

Code review

Found 4 issues:

  1. Stale doc comment on execute_tool claims CHIBI_VERBOSE=1 is injected into plugin subprocesses — it no longer is. The line cmd.env(\"CHIBI_VERBOSE\", \"1\") was removed in this PR but the doc comment was not updated. Plugin authors reading this will be misled. (AGENTS.md says "Missing or incorrect documentation including code comments are critical bugs.")

/// Tools receive arguments via stdin (JSON), leaving stdout for results.
/// Tools also receive CHIBI_VERBOSE=1 env var when verbose mode is enabled.
pub fn execute_tool(tool: &Tool, arguments: &serde_json::Value) -> io::Result<String> {
let mut cmd = Command::new(&tool.path);

  1. show_thinking default silently inverted from true to false. ConfigDefaults::SHOW_THINKING was true (added in commit 9142ed9), but the new CliConfig::default() sets show_thinking: false. Users without an explicit config entry will silently stop seeing thinking/reasoning content — a breaking behaviour change. The design doc claims zero behaviour change for CLI users.

verbose: false,
hide_tool_calls: false,
show_thinking: false,
image: ImageConfig::default(),
markdown_style: default_markdown_style(),

  1. NoopSink is defined twice with identical bodies in chibi.rs. The same struct and full impl OutputSink block are copy-pasted into Chibi::load() and Chibi::from_home(). (AGENTS.md says "Iterate over structures to prevent code duplication. Less code is better code; one pattern is better than two.")

pub fn load() -> io::Result<Self> {
struct NoopSink;
impl OutputSink for NoopSink {
fn emit_result(&self, _: &str) {}
fn emit_event(&self, _: CommandEvent) {}
fn newline(&self) {}
fn emit_entry(&self, _: &crate::context::TranscriptEntry) -> io::Result<()> {
Ok(())
}
fn confirm(&self, _: &str) -> bool {
false
}
}
Self::load_with_options(LoadOptions::default(), &NoopSink)
}
/// Load chibi from a specific home directory.
///
/// Convenience method for `load_with_options(LoadOptions { home: Some(...), ..Default::default() }, sink)`.
/// Kept as a shortcut for library users who only need to override the home directory.
///
/// This overrides both `CHIBI_HOME` and the default `~/.chibi`.
pub fn from_home(home: &Path) -> io::Result<Self> {
struct NoopSink;
impl OutputSink for NoopSink {
fn emit_result(&self, _: &str) {}
fn emit_event(&self, _: CommandEvent) {}
fn newline(&self) {}
fn emit_entry(&self, _: &crate::context::TranscriptEntry) -> io::Result<()> {
Ok(())
}
fn confirm(&self, _: &str) -> bool {
false
}
}
Self::load_with_options(
LoadOptions {
home: Some(home.to_path_buf()),
..Default::default()
},
&NoopSink,
)

  1. OutputHandler::emit_event maps every CommandEvent to a (String, bool) tuple but verbose_only is hardcoded true in all 12 branches — the bool never varies, making the two-tuple an abstraction with zero differentiation. A single guard at the top (or per-arm handling) would be cleaner. (AGENTS.md says "one pattern is better than two.")

fn emit_event(&self, event: CommandEvent) {
let (text, verbose_only) = match &event {
CommandEvent::AutoDestroyed { count } => (
format!("[Auto-destroyed {} expired context(s)]", count),
true,
),
CommandEvent::CacheCleanup {
removed,
max_age_days,
} => (
format!(
"[Auto-cleanup: removed {} old cache entries (older than {} days)]",
removed,
max_age_days + 1
),
true,
),
CommandEvent::SystemPromptSet { context } => (
format!("[System prompt set for context '{}']", context),
true,
),
CommandEvent::UsernameSaved { username, context } => (
format!("[Username '{}' saved to context '{}']", username, context),
true,
),
CommandEvent::InboxEmpty { context } => {
(format!("[No messages in inbox for '{}']", context), true)
}
CommandEvent::InboxProcessing { count, context } => (
format!(
"[Processing {} message(s) from inbox for '{}']",
count, context
),
true,
),
CommandEvent::AllInboxesEmpty => ("[No messages in any inbox.]".to_string(), true),
CommandEvent::InboxesProcessed { count } => (
format!("[Processed inboxes for {} context(s).]", count),
true,
),
CommandEvent::ContextLoaded { tool_count } => {
(format!("[Loaded {} tool(s)]", tool_count), true)
}
CommandEvent::McpToolsLoaded { count } => {
(format!("[MCP: {} tools loaded]", count), true)
}
CommandEvent::McpBridgeUnavailable { reason } => {
(format!("[MCP: bridge unavailable: {}]", reason), true)
}
CommandEvent::LoadSummary {
builtin_count,
builtin_names,
plugin_count,
plugin_names,
} => {
let mut lines = format!(
"[Built-in ({}): {}]",
builtin_count,
builtin_names.join(", ")
);
if *plugin_count == 0 {
lines.push_str("\n[No plugins loaded]");
} else {
lines.push_str(&format!(
"\n[Plugins ({}): {}]",
plugin_count,
plugin_names.join(", ")
));
}
(lines, true)
}
};
if !verbose_only || self.verbose {
eprintln!("{}", text);

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@emesal emesal merged commit 9553277 into dev Feb 19, 2026
21 checks passed
@emesal emesal deleted the refactor/typed-output-2602 branch February 19, 2026 11:53
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.

1 participant

Comments