Skip to content

fix: profiles bugs — path resolution, fail-fast, dedup merge (#238) #239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ajianaz merged 1 commit into from
Jun 10, 2026
Merged

fix: profiles bugs — path resolution, fail-fast, dedup merge (#238) #239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/config/loader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ pub fn load_config(

// 1. Load global config (~/.cora/config.yaml)
if let Some(cora) = load_global_config()? {
cora.merge_into(&mut config);
cora.merge_into(&mut config)?;
}

// 2. Load project config (.cora.yaml)
Expand All @@ -257,10 +257,10 @@ pub fn load_config(
path.display()
))
})?;
cora.merge_into(&mut config);
cora.merge_into(&mut config)?;
debug!(path = %path.display(), "loaded explicit config");
} else if let Some((path, cora)) = find_cora_file(&std::env::current_dir()?)? {
cora.merge_into(&mut config);
cora.merge_into(&mut config)?;
debug!(path = %path.display(), "loaded discovered config");
} else {
debug!("no .cora.yaml found, using defaults");
Expand Down
52 changes: 28 additions & 24 deletions src/config/schema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ impl CoraFile {

/// Merge this file config into a `Config`, overwriting only fields that are present.
#[allow(clippy::assigning_clones)]
pub fn merge_into(&self, config: &mut Config) {
pub fn merge_into(&self, config: &mut Config) -> Result<(), CoraError> {
if let Some(v) = &self.model {
config.provider.model.clone_from(v);
}
Expand Down Expand Up @@ -473,11 +473,15 @@ impl CoraFile {
config.profile = Some(profile);
}
Err(e) => {
// Warn but don't fail — profile is optional enhancement
tracing::warn!("invalid profile in config, ignoring: {e}");
// Fail fast — invalid profile means review policy is broken.
// Better to error than silently run without the intended policy.
return Err(CoraError::ConfigParse(format!(
"invalid profile in config: {e}"
)));
}
}
}
Ok(())
}
}

Expand Down Expand Up @@ -542,7 +546,7 @@ mod tests {
fn merge_empty_cora_file_leaves_defaults() {
let mut cfg = Config::default();
let cora = CoraFile::default();
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.provider.provider, "openai");
assert_eq!(cfg.provider.model, "gpt-4o-mini");
assert_eq!(cfg.output.format, "pretty");
Expand All @@ -559,7 +563,7 @@ mod tests {
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.provider.provider, "anthropic");
assert_eq!(cfg.provider.model, "claude-3-haiku");
assert_eq!(cfg.provider.base_url, "https://api.anthropic.com/v1");
Expand All @@ -577,7 +581,7 @@ base_url: https://api.z.ai/api/coding/paas/v4
)
.unwrap();

cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();

assert_eq!(cfg.provider.provider, "openai");
assert_eq!(cfg.provider.model, "glm-5.1");
Expand All @@ -595,7 +599,7 @@ base_url: https://api.z.ai/api/coding/paas/v4
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.provider.provider, "ollama");
assert_eq!(cfg.provider.model, "llama3.1"); // resolved from ollama preset
assert_eq!(cfg.provider.base_url, "http://localhost:11434/v1"); // resolved from ollama preset
Expand All @@ -611,7 +615,7 @@ provider: zai
)
.unwrap();

cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.provider.provider, "zai");
assert_eq!(cfg.provider.model, "glm-5.1"); // resolved from zai preset
assert_eq!(cfg.provider.base_url, "https://api.z.ai/api/coding/paas/v4"); // resolved from zai preset
Expand All @@ -624,7 +628,7 @@ provider: zai
focus: Some(vec!["security".to_string(), "bugs".to_string()]),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.focus, vec!["security", "bugs"]);
}

Expand All @@ -635,7 +639,7 @@ provider: zai
rules: Some(vec!["no unwrap".to_string()]),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.rules, vec!["no unwrap"]);
}

Expand All @@ -649,7 +653,7 @@ provider: zai
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.ignore.files, vec!["vendor/**"]);
assert_eq!(cfg.ignore.rules, vec!["skip-rule-1"]);
}
Expand All @@ -666,7 +670,7 @@ provider: zai
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.hook.mode, "block");
assert_eq!(cfg.hook.min_severity, "critical");
assert_eq!(cfg.hook.max_diff_size, 1024);
Expand All @@ -683,7 +687,7 @@ provider: zai
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.output.format, "json");
assert!(!cfg.output.color);
}
Expand Down Expand Up @@ -854,7 +858,7 @@ review:
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.response_format, "json_object");
}

Expand All @@ -871,7 +875,7 @@ review:
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(
cfg.review_system_prompt_override.as_deref(),
Some("Custom prompt here.")
Expand All @@ -891,7 +895,7 @@ review:
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(
cfg.review_system_prompt_file.as_deref(),
Some("prompts/review.md")
Expand Down Expand Up @@ -924,7 +928,7 @@ scan:
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(
cfg.scan_system_prompt_override.as_deref(),
Some("Performance only.")
Expand Down Expand Up @@ -1023,7 +1027,7 @@ llm:
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.temperature, 0.7);
// Other LLM fields should remain at defaults
assert_eq!(cfg.max_tokens, 4096);
Expand All @@ -1043,7 +1047,7 @@ llm:
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.max_tokens, 2048);
}

Expand All @@ -1059,7 +1063,7 @@ llm:
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.timeout, 300);
}

Expand All @@ -1075,7 +1079,7 @@ llm:
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.temperature, 1.0);
assert_eq!(cfg.max_tokens, 16384);
assert_eq!(cfg.timeout, 240);
Expand Down Expand Up @@ -1194,7 +1198,7 @@ bundling:
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.bundling.max_chars_per_group, 30_000);
assert_eq!(cfg.bundling.max_files_per_group, 10);
assert_eq!(
Expand All @@ -1218,7 +1222,7 @@ bundling:
}),
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.bundling.max_chars_per_group, 40_000);
assert_eq!(cfg.bundling.max_files_per_group, 20); // default unchanged
assert_eq!(
Expand All @@ -1235,7 +1239,7 @@ bundling:
let cora = CoraFile {
..Default::default()
};
cora.merge_into(&mut cfg);
cora.merge_into(&mut cfg).unwrap();
assert_eq!(cfg.bundling.max_chars_per_group, 60_000);
assert_eq!(cfg.bundling.max_files_per_group, 20);
}
Expand Down
45 changes: 33 additions & 12 deletions src/engine/profiles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,23 +157,35 @@ pub fn load_all_builtins() -> Vec<Profile> {
/// Resolve a `ProfileRef` from `.cora.yaml` into a final `Profile`.
///
/// Resolution order:
/// 1. If `Name("builtin")` → load built-in
/// 2. If `Name("./path.yaml")` → load from file
/// 3. If `Inline { extends, ... }` → merge with base
/// 4. If `Inline` without extends → use as-is
/// - If `Name("builtin")` → load built-in
/// - If `Name("./path.yaml")` → load from file (resolve relative to project root)
/// - If `Inline { extends, ... }` → merge with base
/// - If `Inline` without extends → use as-is
pub fn resolve_profile(profile_ref: &ProfileRef) -> Result<Profile, String> {
resolve_profile_with_root(profile_ref, None)
}

/// Resolve profile with optional project root for relative file path resolution.
pub fn resolve_profile_with_root(
profile_ref: &ProfileRef,
project_root: Option<&std::path::Path>,
) -> Result<Profile, String> {
match profile_ref {
ProfileRef::Name(name) => {
// Try built-in first
if let Some(p) = load_builtin(name) {
debug!(profile = name, "loaded built-in profile");
return Ok(p);
}
// Try loading from file (resolve relative to CWD)
let path = std::path::Path::new(name);
// Resolve relative to project root (not CWD) for deterministic behavior
let path = if let Some(root) = project_root {
root.join(name)
} else {
std::path::PathBuf::from(name)
};
if path.is_file() {
let content = std::fs::read_to_string(path)
.map_err(|e| format!("cannot read profile file '{}': {e}", name))?;
let content = std::fs::read_to_string(&path)
.map_err(|e| format!("cannot read profile file '{}': {e}", path.display()))?;
let mut profile = parse_profile_yaml(&content)?;
// If profile has no name, derive from filename
if profile.name.is_empty() {
Expand All @@ -183,11 +195,11 @@ pub fn resolve_profile(profile_ref: &ProfileRef) -> Result<Profile, String> {
.unwrap_or("custom")
.to_string();
}
debug!(profile = name, "loaded profile from file");
debug!(profile = %path.display(), "loaded profile from file");
return Ok(profile);
}
Err(format!(
"unknown profile '{name}'. Available: {}",
"unknown profile '{name}'. Available built-ins: {}",
BUILTIN_PROFILES.join(", ")
))
}
Expand All @@ -206,9 +218,18 @@ pub fn resolve_profile(profile_ref: &ProfileRef) -> Result<Profile, String> {
profile.name = "custom".to_string();
}

// Merge focus areas (append, don't replace)
// Merge focus areas by id — replace existing, append new
if !inline.focus_areas.is_empty() {
profile.focus_areas.extend(inline.focus_areas.clone());
for area in &inline.focus_areas {
if let Some(existing) = profile.focus_areas.iter_mut().find(|a| a.id == area.id)
{
// Replace existing focus area with override
*existing = area.clone();
} else {
// Append new focus area
profile.focus_areas.push(area.clone());
}
}
}

// Override ignore areas
Expand Down
Loading