diff --git a/docs/implementation/PHASE-1-STEP-16f-NOTES.md b/docs/implementation/PHASE-1-STEP-16f-NOTES.md index d667711..b31fff5 100644 --- a/docs/implementation/PHASE-1-STEP-16f-NOTES.md +++ b/docs/implementation/PHASE-1-STEP-16f-NOTES.md @@ -191,9 +191,6 @@ pub struct Config { } impl Config { - pub async fn search_rag(...) -> Result { ... } - pub fn load_macro(name: &str) -> Result { ... } - pub async fn sync_models(url: &str, ...) -> Result<()> { ... } pub async fn load_with_interpolation(info_flag: bool) -> Result { ... } pub fn load_from_file(config_path: &Path) -> Result<(Self, String)> { ... } pub fn load_from_str(content: &str) -> Result { ... } @@ -201,10 +198,11 @@ impl Config { } ``` -Just shape + loaders + a few helpers that don't touch runtime -state. `search_rag`/`load_macro`/`sync_models` are associated -functions (no `&self`) that could be free functions someday, but -that's cosmetic and out of scope for 16f. +Just shape + four loaders. The three associated functions that +used to live here (`search_rag`, `load_macro`, `sync_models`) +were relocated in the 16f cleanup pass below — none of them +touched Config state, they were squatters from the god-object +era. ## Assumptions made @@ -301,3 +299,57 @@ per-request `RequestContext`. ## Files deleted (16f) - `src/config/bridge.rs` + +## 16f cleanup pass — Config straggler relocation + +After the main 16f deletions landed, three associated functions +remained on `impl Config` that took no `&self` and didn't touch +any Config field — they were holdovers from the god-object era, +attached to `Config` only because Config used to be the +namespace for everything. Relocated each to its rightful owner: + +| Method | New home | Why | +|--------|----------|-----| +| `Config::load_macro(name)` | `Macro::load(name)` in `src/config/macros.rs` | Sibling of `Macro::install_macros` already there. The function loads a macro from disk and parses it into a `Macro` — pure macro concern. | +| `Config::search_rag(app, rag, text, signal)` | `Rag::search_with_template(&self, app, text, signal)` in `src/rag/mod.rs` | Operates on a `Rag` instance and one field of `AppConfig`. Pulled `RAG_TEMPLATE` constant along with it. | +| `Config::sync_models(url, signal)` | Free function `config::sync_models(url, signal)` in `src/config/mod.rs` | Fetches a URL, parses YAML, writes to `paths::models_override_file()`. No Config state involved. Sibling pattern to `install_builtins`, `default_sessions_dir`, `list_sessions`. | + +### Caller updates + +- `src/config/macros.rs:23` — `Config::load_macro(name)` → `Macro::load(name)` +- `src/config/input.rs:214` — `Config::search_rag(&self.app_config, rag, &self.text, abort_signal)` → `rag.search_with_template(&self.app_config, &self.text, abort_signal)` +- `src/main.rs:149` — `Config::sync_models(&url, abort_signal.clone())` → `sync_models(&url, abort_signal.clone())` (added `sync_models` to the `crate::config::{...}` import list) + +### Constants relocated + +- `RAG_TEMPLATE` moved from `src/config/mod.rs` to `src/rag/mod.rs` alongside the new `search_with_template` method that uses it. + +### Final shape of `impl Config` + +```rust +impl Config { + pub async fn load_with_interpolation(info_flag: bool) -> Result { ... } + pub fn load_from_file(config_path: &Path) -> Result<(Self, String)> { ... } + pub fn load_from_str(content: &str) -> Result { ... } + pub fn load_dynamic(model_id: &str) -> Result { ... } +} +``` + +Four loaders, all returning `Self` or `(Self, String)`. Nothing +else. The `Config` type is now genuinely what its docstring +claims: a serde POJO with constructors. No squatters. + +### Verification (cleanup pass) + +- `cargo check` — clean +- `cargo clippy --all-targets` — clean +- `cargo test` — 122 passing, zero failures +- `Config::sync_models` / `Config::load_macro` / `Config::search_rag` — zero hits in `src/` + +### Files modified (cleanup pass) + +- `src/config/mod.rs` — deleted `Config::load_macro`, `Config::search_rag`, `Config::sync_models`, and `RAG_TEMPLATE` const; added free `sync_models` function +- `src/config/macros.rs` — added `Macro::load`, updated import (added `Context`, `read_to_string`; removed `Config`) +- `src/rag/mod.rs` — added `RAG_TEMPLATE` const and `Rag::search_with_template` method +- `src/config/input.rs` — updated caller to `rag.search_with_template` +- `src/main.rs` — added `sync_models` to import list, updated caller diff --git a/src/config/input.rs b/src/config/input.rs index 7d6e26c..b133bec 100644 --- a/src/config/input.rs +++ b/src/config/input.rs @@ -210,8 +210,9 @@ impl Input { return Ok(()); } if let Some(rag) = &self.rag { - let result = - Config::search_rag(&self.app_config, rag, &self.text, abort_signal).await?; + let result = rag + .search_with_template(&self.app_config, &self.text, abort_signal) + .await?; self.patched_text = Some(result); self.rag_name = Some(rag.name().to_string()); } diff --git a/src/config/macros.rs b/src/config/macros.rs index 69f631b..86dd34e 100644 --- a/src/config/macros.rs +++ b/src/config/macros.rs @@ -1,12 +1,12 @@ use crate::config::paths; -use crate::config::{Config, RequestContext, RoleLike, ensure_parent_exists}; +use crate::config::{RequestContext, RoleLike, ensure_parent_exists}; use crate::repl::{run_repl_command, split_args_text}; use crate::utils::{AbortSignal, multiline_text}; -use anyhow::{Result, anyhow}; +use anyhow::{Context, Result, anyhow}; use indexmap::IndexMap; use rust_embed::Embed; use serde::Deserialize; -use std::fs::File; +use std::fs::{File, read_to_string}; use std::io::Write; #[derive(Embed)] @@ -20,7 +20,7 @@ pub async fn macro_execute( args: Option<&str>, abort_signal: AbortSignal, ) -> Result<()> { - let macro_value = Config::load_macro(name)?; + let macro_value = Macro::load(name)?; let (mut new_args, text) = split_args_text(args.unwrap_or_default(), cfg!(windows)); if !text.is_empty() { new_args.push(text.to_string()); @@ -76,6 +76,14 @@ pub struct Macro { } impl Macro { + pub fn load(name: &str) -> Result { + let path = paths::macro_file(name); + let err = || format!("Failed to load macro '{name}' at '{}'", path.display()); + let content = read_to_string(&path).with_context(err)?; + let value: Macro = serde_yaml::from_str(&content).with_context(err)?; + Ok(value) + } + pub fn install_macros() -> Result<()> { info!( "Installing built-in macros in {}", diff --git a/src/config/mod.rs b/src/config/mod.rs index b2b6114..37466c3 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -89,30 +89,6 @@ const SUMMARIZATION_PROMPT: &str = "Summarize the discussion briefly in 200 words or less to use as a prompt for future context."; const SUMMARY_CONTEXT_PROMPT: &str = "This is a summary of the chat history as a recap: "; -const RAG_TEMPLATE: &str = r#"Answer the query based on the context while respecting the rules. (user query, some textual context and rules, all inside xml tags) - - -__CONTEXT__ - - - -__SOURCES__ - - - -- If you don't know, just say so. -- If you are not sure, ask for clarification. -- Answer in the same language as the user query. -- If the context appears unreadable or of poor quality, tell the user then answer as best as you can. -- If the answer is not in the context but you think you know the answer, explain that to the user then answer with your own knowledge. -- Answer directly and without using xml tags. -- When using information from the context, cite the relevant source from the section. - - - -__INPUT__ -"#; - const LEFT_PROMPT: &str = "{color.red}{model}){color.green}{?session {?agent {agent}>}{session}{?role /}}{!session {?agent {agent}>}}{role}{?rag @{rag}}{color.cyan}{?session )}{!session >}{color.reset} "; const RIGHT_PROMPT: &str = "{color.purple}{?session {?consume_tokens {consume_tokens}({consume_percent}%)}{!consume_tokens {consume_tokens}}}{color.reset}"; @@ -251,60 +227,29 @@ pub fn list_sessions() -> Vec { list_file_names(default_sessions_dir(), ".yaml") } +pub async fn sync_models(url: &str, abort_signal: AbortSignal) -> Result<()> { + let content = abortable_run_with_spinner(fetch(url), "Fetching models.yaml", abort_signal) + .await + .with_context(|| format!("Failed to fetch '{url}'"))?; + println!("✓ Fetched '{url}'"); + let list = serde_yaml::from_str::>(&content) + .with_context(|| "Failed to parse models.yaml")?; + let models_override = ModelsOverride { + version: env!("CARGO_PKG_VERSION").to_string(), + list, + }; + let models_override_data = + serde_yaml::to_string(&models_override).with_context(|| "Failed to serde {}")?; + + let model_override_path = paths::models_override_file(); + ensure_parent_exists(&model_override_path)?; + std::fs::write(&model_override_path, models_override_data) + .with_context(|| format!("Failed to write to '{}'", model_override_path.display()))?; + println!("✓ Updated '{}'", model_override_path.display()); + Ok(()) +} + impl Config { - pub async fn search_rag( - app: &AppConfig, - rag: &Rag, - text: &str, - abort_signal: AbortSignal, - ) -> Result { - let (reranker_model, top_k) = rag.get_config(); - let (embeddings, sources, ids) = rag - .search(text, top_k, reranker_model.as_deref(), abort_signal) - .await?; - let rag_template = app.rag_template.as_deref().unwrap_or(RAG_TEMPLATE); - let text = if embeddings.is_empty() { - text.to_string() - } else { - rag_template - .replace("__CONTEXT__", &embeddings) - .replace("__SOURCES__", &sources) - .replace("__INPUT__", text) - }; - rag.set_last_sources(&ids); - Ok(text) - } - - pub fn load_macro(name: &str) -> Result { - let path = paths::macro_file(name); - let err = || format!("Failed to load macro '{name}' at '{}'", path.display()); - let content = read_to_string(&path).with_context(err)?; - let value: Macro = serde_yaml::from_str(&content).with_context(err)?; - Ok(value) - } - - pub async fn sync_models(url: &str, abort_signal: AbortSignal) -> Result<()> { - let content = abortable_run_with_spinner(fetch(url), "Fetching models.yaml", abort_signal) - .await - .with_context(|| format!("Failed to fetch '{url}'"))?; - println!("✓ Fetched '{url}'"); - let list = serde_yaml::from_str::>(&content) - .with_context(|| "Failed to parse models.yaml")?; - let models_override = ModelsOverride { - version: env!("CARGO_PKG_VERSION").to_string(), - list, - }; - let models_override_data = - serde_yaml::to_string(&models_override).with_context(|| "Failed to serde {}")?; - - let model_override_path = paths::models_override_file(); - ensure_parent_exists(&model_override_path)?; - std::fs::write(&model_override_path, models_override_data) - .with_context(|| format!("Failed to write to '{}'", model_override_path.display()))?; - println!("✓ Updated '{}'", model_override_path.display()); - Ok(()) - } - pub async fn load_with_interpolation(info_flag: bool) -> Result { let config_path = paths::config_file(); let (mut config, content) = if !config_path.exists() { diff --git a/src/main.rs b/src/main.rs index a71023c..b453566 100644 --- a/src/main.rs +++ b/src/main.rs @@ -23,7 +23,7 @@ use crate::config::paths; use crate::config::{ Agent, AppConfig, AppState, CODE_ROLE, Config, EXPLAIN_SHELL_ROLE, Input, RequestContext, SHELL_ROLE, TEMP_SESSION_NAME, WorkingMode, ensure_parent_exists, install_builtins, - list_agents, load_env_file, macro_execute, + list_agents, load_env_file, macro_execute, sync_models, }; use crate::render::{prompt_theme, render_error}; use crate::repl::Repl; @@ -146,7 +146,7 @@ async fn run( ) -> Result<()> { if cli.sync_models { let url = ctx.app.config.sync_models_url(); - return Config::sync_models(&url, abort_signal.clone()).await; + return sync_models(&url, abort_signal.clone()).await; } if cli.list_models { diff --git a/src/rag/mod.rs b/src/rag/mod.rs index c09c9c6..1232452 100644 --- a/src/rag/mod.rs +++ b/src/rag/mod.rs @@ -20,6 +20,30 @@ use std::{ }; use tokio::time::sleep; +const RAG_TEMPLATE: &str = r#"Answer the query based on the context while respecting the rules. (user query, some textual context and rules, all inside xml tags) + + +__CONTEXT__ + + + +__SOURCES__ + + + +- If you don't know, just say so. +- If you are not sure, ask for clarification. +- Answer in the same language as the user query. +- If the context appears unreadable or of poor quality, tell the user then answer as best as you can. +- If the answer is not in the context but you think you know the answer, explain that to the user then answer with your own knowledge. +- Answer directly and without using xml tags. +- When using information from the context, cite the relevant source from the section. + + + +__INPUT__ +"#; + pub struct Rag { app_config: Arc, name: String, @@ -318,6 +342,29 @@ impl Rag { Ok((embeddings, sources, ids)) } + pub async fn search_with_template( + &self, + app: &AppConfig, + text: &str, + abort_signal: AbortSignal, + ) -> Result { + let (reranker_model, top_k) = self.get_config(); + let (embeddings, sources, ids) = self + .search(text, top_k, reranker_model.as_deref(), abort_signal) + .await?; + let rag_template = app.rag_template.as_deref().unwrap_or(RAG_TEMPLATE); + let text = if embeddings.is_empty() { + text.to_string() + } else { + rag_template + .replace("__CONTEXT__", &embeddings) + .replace("__SOURCES__", &sources) + .replace("__INPUT__", text) + }; + self.set_last_sources(&ids); + Ok(text) + } + fn resolve_source(&self, id: &DocumentId) -> String { let (file_index, _) = id.split(); self.data