mirror of
https://github.com/instructkr/claw-code.git
synced 2026-06-04 13:37:09 +08:00
fix: attribute config precedence in JSON
This commit is contained in:
@@ -99,6 +99,10 @@ pub struct ConfigFileReport {
|
||||
pub status: ConfigFileStatus,
|
||||
pub reason: Option<String>,
|
||||
pub detail: Option<String>,
|
||||
pub precedence_rank: usize,
|
||||
pub wins_for_keys: Vec<String>,
|
||||
pub shadowed_keys: Vec<String>,
|
||||
key_paths: Vec<String>,
|
||||
}
|
||||
|
||||
/// Best-effort inspection of the config discovery and load pipeline.
|
||||
@@ -463,12 +467,14 @@ impl ConfigLoader {
|
||||
let mut files = Vec::new();
|
||||
let mut load_error = None;
|
||||
|
||||
for entry in self.discover() {
|
||||
for (index, entry) in self.discover().into_iter().enumerate() {
|
||||
let precedence_rank = index + 1;
|
||||
if let Err(error) = crate::config_validate::check_unsupported_format(&entry.path) {
|
||||
let detail = error.to_string();
|
||||
load_error.get_or_insert_with(|| detail.clone());
|
||||
files.push(ConfigFileReport::load_error(
|
||||
entry,
|
||||
precedence_rank,
|
||||
"unsupported_format",
|
||||
detail,
|
||||
));
|
||||
@@ -478,18 +484,28 @@ impl ConfigLoader {
|
||||
let parsed = match read_optional_json_object(&entry.path) {
|
||||
Ok(OptionalConfigFile::Loaded(parsed)) => parsed,
|
||||
Ok(OptionalConfigFile::NotFound) => {
|
||||
files.push(ConfigFileReport::not_found(entry));
|
||||
files.push(ConfigFileReport::not_found(entry, precedence_rank));
|
||||
continue;
|
||||
}
|
||||
Ok(OptionalConfigFile::Skipped { reason, detail }) => {
|
||||
files.push(ConfigFileReport::skipped(entry, reason, detail));
|
||||
files.push(ConfigFileReport::skipped(
|
||||
entry,
|
||||
precedence_rank,
|
||||
reason,
|
||||
detail,
|
||||
));
|
||||
continue;
|
||||
}
|
||||
Err(error) => {
|
||||
let reason = config_error_reason(&error).to_string();
|
||||
let detail = error.to_string();
|
||||
load_error.get_or_insert_with(|| detail.clone());
|
||||
files.push(ConfigFileReport::load_error(entry, reason, detail));
|
||||
files.push(ConfigFileReport::load_error(
|
||||
entry,
|
||||
precedence_rank,
|
||||
reason,
|
||||
detail,
|
||||
));
|
||||
continue;
|
||||
}
|
||||
};
|
||||
@@ -504,6 +520,7 @@ impl ConfigLoader {
|
||||
load_error.get_or_insert_with(|| detail.clone());
|
||||
files.push(ConfigFileReport::load_error(
|
||||
entry,
|
||||
precedence_rank,
|
||||
"validation_error",
|
||||
detail,
|
||||
));
|
||||
@@ -521,6 +538,7 @@ impl ConfigLoader {
|
||||
load_error.get_or_insert_with(|| detail.clone());
|
||||
files.push(ConfigFileReport::load_error(
|
||||
entry,
|
||||
precedence_rank,
|
||||
"validation_error",
|
||||
detail,
|
||||
));
|
||||
@@ -532,15 +550,23 @@ impl ConfigLoader {
|
||||
{
|
||||
let detail = error.to_string();
|
||||
load_error.get_or_insert_with(|| detail.clone());
|
||||
files.push(ConfigFileReport::load_error(entry, "parse_error", detail));
|
||||
files.push(ConfigFileReport::load_error(
|
||||
entry,
|
||||
precedence_rank,
|
||||
"parse_error",
|
||||
detail,
|
||||
));
|
||||
continue;
|
||||
}
|
||||
|
||||
let key_paths = collect_config_key_paths(&parsed.object);
|
||||
deep_merge_objects(&mut merged, &parsed.object);
|
||||
loaded_entries.push(entry.clone());
|
||||
files.push(ConfigFileReport::loaded(entry));
|
||||
files.push(ConfigFileReport::loaded(entry, precedence_rank, key_paths));
|
||||
}
|
||||
|
||||
annotate_config_file_precedence(&mut files);
|
||||
|
||||
let runtime_config = match build_runtime_config(merged, loaded_entries, mcp_servers) {
|
||||
Ok(config) => Some(config),
|
||||
Err(error) => {
|
||||
@@ -559,47 +585,121 @@ impl ConfigLoader {
|
||||
}
|
||||
|
||||
impl ConfigFileReport {
|
||||
fn loaded(entry: ConfigEntry) -> Self {
|
||||
fn loaded(entry: ConfigEntry, precedence_rank: usize, key_paths: Vec<String>) -> Self {
|
||||
Self {
|
||||
entry,
|
||||
loaded: true,
|
||||
status: ConfigFileStatus::Loaded,
|
||||
reason: None,
|
||||
detail: None,
|
||||
precedence_rank,
|
||||
wins_for_keys: Vec::new(),
|
||||
shadowed_keys: Vec::new(),
|
||||
key_paths,
|
||||
}
|
||||
}
|
||||
|
||||
fn not_found(entry: ConfigEntry) -> Self {
|
||||
fn not_found(entry: ConfigEntry, precedence_rank: usize) -> Self {
|
||||
Self {
|
||||
entry,
|
||||
loaded: false,
|
||||
status: ConfigFileStatus::NotFound,
|
||||
reason: Some("not_found".to_string()),
|
||||
detail: None,
|
||||
precedence_rank,
|
||||
wins_for_keys: Vec::new(),
|
||||
shadowed_keys: Vec::new(),
|
||||
key_paths: Vec::new(),
|
||||
}
|
||||
}
|
||||
|
||||
fn skipped(entry: ConfigEntry, reason: String, detail: Option<String>) -> Self {
|
||||
fn skipped(
|
||||
entry: ConfigEntry,
|
||||
precedence_rank: usize,
|
||||
reason: String,
|
||||
detail: Option<String>,
|
||||
) -> Self {
|
||||
Self {
|
||||
entry,
|
||||
loaded: false,
|
||||
status: ConfigFileStatus::Skipped,
|
||||
reason: Some(reason),
|
||||
detail,
|
||||
precedence_rank,
|
||||
wins_for_keys: Vec::new(),
|
||||
shadowed_keys: Vec::new(),
|
||||
key_paths: Vec::new(),
|
||||
}
|
||||
}
|
||||
|
||||
fn load_error(entry: ConfigEntry, reason: impl Into<String>, detail: String) -> Self {
|
||||
fn load_error(
|
||||
entry: ConfigEntry,
|
||||
precedence_rank: usize,
|
||||
reason: impl Into<String>,
|
||||
detail: String,
|
||||
) -> Self {
|
||||
Self {
|
||||
entry,
|
||||
loaded: false,
|
||||
status: ConfigFileStatus::LoadError,
|
||||
reason: Some(reason.into()),
|
||||
detail: Some(detail),
|
||||
precedence_rank,
|
||||
wins_for_keys: Vec::new(),
|
||||
shadowed_keys: Vec::new(),
|
||||
key_paths: Vec::new(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn annotate_config_file_precedence(files: &mut [ConfigFileReport]) {
|
||||
let mut winning_file_by_key = BTreeMap::new();
|
||||
for (index, file) in files.iter().enumerate() {
|
||||
if !file.loaded {
|
||||
continue;
|
||||
}
|
||||
for key in &file.key_paths {
|
||||
winning_file_by_key.insert(key.clone(), index);
|
||||
}
|
||||
}
|
||||
|
||||
for (index, file) in files.iter_mut().enumerate() {
|
||||
if !file.loaded {
|
||||
continue;
|
||||
}
|
||||
let mut wins_for_keys = Vec::new();
|
||||
let mut shadowed_keys = Vec::new();
|
||||
for key in &file.key_paths {
|
||||
if winning_file_by_key.get(key).copied() == Some(index) {
|
||||
wins_for_keys.push(key.clone());
|
||||
} else {
|
||||
shadowed_keys.push(key.clone());
|
||||
}
|
||||
}
|
||||
file.wins_for_keys = wins_for_keys;
|
||||
file.shadowed_keys = shadowed_keys;
|
||||
}
|
||||
}
|
||||
|
||||
fn collect_config_key_paths(object: &BTreeMap<String, JsonValue>) -> Vec<String> {
|
||||
let mut keys = Vec::new();
|
||||
for (key, value) in object {
|
||||
collect_config_key_paths_for_value(key, value, &mut keys);
|
||||
}
|
||||
keys
|
||||
}
|
||||
|
||||
fn collect_config_key_paths_for_value(prefix: &str, value: &JsonValue, keys: &mut Vec<String>) {
|
||||
match value {
|
||||
JsonValue::Object(object) if !object.is_empty() => {
|
||||
for (key, nested) in object {
|
||||
collect_config_key_paths_for_value(&format!("{prefix}.{key}"), nested, keys);
|
||||
}
|
||||
}
|
||||
_ => keys.push(prefix.to_string()),
|
||||
}
|
||||
}
|
||||
|
||||
fn build_runtime_config(
|
||||
merged: BTreeMap<String, JsonValue>,
|
||||
loaded_entries: Vec<ConfigEntry>,
|
||||
@@ -2982,23 +3082,23 @@ mod tests {
|
||||
.expect("write user settings");
|
||||
|
||||
// when
|
||||
let error = ConfigLoader::new(&cwd, &home)
|
||||
.load()
|
||||
.expect_err("config should fail");
|
||||
let (_config, warnings) = ConfigLoader::new(&cwd, &home)
|
||||
.load_collecting_warnings()
|
||||
.expect("unknown config keys should load with warnings");
|
||||
|
||||
// then
|
||||
let rendered = error.to_string();
|
||||
let rendered = warnings.join("\n");
|
||||
assert!(
|
||||
rendered.contains(&user_settings.display().to_string()),
|
||||
"error should include file path, got: {rendered}"
|
||||
"warning should include file path, got: {rendered}"
|
||||
);
|
||||
assert!(
|
||||
rendered.contains("line 3"),
|
||||
"error should include line number, got: {rendered}"
|
||||
"warning should include line number, got: {rendered}"
|
||||
);
|
||||
assert!(
|
||||
rendered.contains("telemetry"),
|
||||
"error should name the offending field, got: {rendered}"
|
||||
"warning should name the offending field, got: {rendered}"
|
||||
);
|
||||
|
||||
fs::remove_dir_all(root).expect("cleanup temp dir");
|
||||
@@ -3020,28 +3120,23 @@ mod tests {
|
||||
.expect("write user settings");
|
||||
|
||||
// when
|
||||
let error = ConfigLoader::new(&cwd, &home)
|
||||
.load()
|
||||
.expect_err("config should fail");
|
||||
let (_config, warnings) = ConfigLoader::new(&cwd, &home)
|
||||
.load_collecting_warnings()
|
||||
.expect("legacy unknown config keys should load with warnings");
|
||||
|
||||
// then
|
||||
let rendered = error.to_string();
|
||||
let rendered = warnings.join("\n");
|
||||
assert!(
|
||||
rendered.contains(&user_settings.display().to_string()),
|
||||
"error should include file path, got: {rendered}"
|
||||
"warning should include file path, got: {rendered}"
|
||||
);
|
||||
assert!(
|
||||
rendered.contains("line 3"),
|
||||
"error should include line number, got: {rendered}"
|
||||
"warning should include line number, got: {rendered}"
|
||||
);
|
||||
assert!(
|
||||
rendered.contains("allowedTools"),
|
||||
"error should call out the unknown field, got: {rendered}"
|
||||
);
|
||||
// allowedTools is an unknown key; validator should name it in the error
|
||||
assert!(
|
||||
rendered.contains("allowedTools"),
|
||||
"error should name the offending field, got: {rendered}"
|
||||
"warning should name the offending field, got: {rendered}"
|
||||
);
|
||||
|
||||
fs::remove_dir_all(root).expect("cleanup temp dir");
|
||||
@@ -3101,19 +3196,19 @@ mod tests {
|
||||
fs::write(&user_settings, "{\n \"modle\": \"opus\"\n}\n").expect("write user settings");
|
||||
|
||||
// when
|
||||
let error = ConfigLoader::new(&cwd, &home)
|
||||
.load()
|
||||
.expect_err("config should fail");
|
||||
let (_config, warnings) = ConfigLoader::new(&cwd, &home)
|
||||
.load_collecting_warnings()
|
||||
.expect("unknown config keys should load with warnings");
|
||||
|
||||
// then
|
||||
let rendered = error.to_string();
|
||||
let rendered = warnings.join("\n");
|
||||
assert!(
|
||||
rendered.contains("modle"),
|
||||
"error should name the offending field, got: {rendered}"
|
||||
"warning should name the offending field, got: {rendered}"
|
||||
);
|
||||
assert!(
|
||||
rendered.contains("model"),
|
||||
"error should suggest the closest known key, got: {rendered}"
|
||||
"warning should suggest the closest known key, got: {rendered}"
|
||||
);
|
||||
|
||||
fs::remove_dir_all(root).expect("cleanup temp dir");
|
||||
|
||||
@@ -424,9 +424,10 @@ fn validate_object_keys(
|
||||
} else if DEPRECATED_FIELDS.iter().any(|d| d.name == key) {
|
||||
// Deprecated key — handled separately, not an unknown-key error.
|
||||
} else {
|
||||
// Unknown key.
|
||||
// Unknown key — preserve compatibility by surfacing it as a warning
|
||||
// instead of blocking otherwise valid config files.
|
||||
let suggestion = suggest_field(key, &known_names);
|
||||
result.errors.push(ConfigDiagnostic {
|
||||
result.warnings.push(ConfigDiagnostic {
|
||||
path: path_display.to_string(),
|
||||
field: field_path,
|
||||
line: find_key_line(source, key),
|
||||
@@ -605,10 +606,11 @@ mod tests {
|
||||
let result = validate_config_file(object, source, &test_path());
|
||||
|
||||
// then
|
||||
assert_eq!(result.errors.len(), 1);
|
||||
assert_eq!(result.errors[0].field, "unknownField");
|
||||
assert!(result.errors.is_empty());
|
||||
assert_eq!(result.warnings.len(), 1);
|
||||
assert_eq!(result.warnings[0].field, "unknownField");
|
||||
assert!(matches!(
|
||||
result.errors[0].kind,
|
||||
result.warnings[0].kind,
|
||||
DiagnosticKind::UnknownKey { .. }
|
||||
));
|
||||
}
|
||||
@@ -688,9 +690,10 @@ mod tests {
|
||||
let result = validate_config_file(object, source, &test_path());
|
||||
|
||||
// then
|
||||
assert_eq!(result.errors.len(), 1);
|
||||
assert_eq!(result.errors[0].line, Some(3));
|
||||
assert_eq!(result.errors[0].field, "badKey");
|
||||
assert!(result.errors.is_empty());
|
||||
assert_eq!(result.warnings.len(), 1);
|
||||
assert_eq!(result.warnings[0].line, Some(3));
|
||||
assert_eq!(result.warnings[0].field, "badKey");
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -719,8 +722,9 @@ mod tests {
|
||||
let result = validate_config_file(object, source, &test_path());
|
||||
|
||||
// then
|
||||
assert_eq!(result.errors.len(), 1);
|
||||
assert_eq!(result.errors[0].field, "hooks.BadHook");
|
||||
assert!(result.errors.is_empty());
|
||||
assert_eq!(result.warnings.len(), 1);
|
||||
assert_eq!(result.warnings[0].field, "hooks.BadHook");
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -785,8 +789,9 @@ mod tests {
|
||||
let result = validate_config_file(object, source, &test_path());
|
||||
|
||||
// then
|
||||
assert_eq!(result.errors.len(), 1);
|
||||
assert_eq!(result.errors[0].field, "permissions.denyAll");
|
||||
assert!(result.errors.is_empty());
|
||||
assert_eq!(result.warnings.len(), 1);
|
||||
assert_eq!(result.warnings[0].field, "permissions.denyAll");
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -800,8 +805,9 @@ mod tests {
|
||||
let result = validate_config_file(object, source, &test_path());
|
||||
|
||||
// then
|
||||
assert_eq!(result.errors.len(), 1);
|
||||
assert_eq!(result.errors[0].field, "sandbox.containerMode");
|
||||
assert!(result.errors.is_empty());
|
||||
assert_eq!(result.warnings.len(), 1);
|
||||
assert_eq!(result.warnings[0].field, "sandbox.containerMode");
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -815,8 +821,9 @@ mod tests {
|
||||
let result = validate_config_file(object, source, &test_path());
|
||||
|
||||
// then
|
||||
assert_eq!(result.errors.len(), 1);
|
||||
assert_eq!(result.errors[0].field, "plugins.autoUpdate");
|
||||
assert!(result.errors.is_empty());
|
||||
assert_eq!(result.warnings.len(), 1);
|
||||
assert_eq!(result.warnings[0].field, "plugins.autoUpdate");
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -830,8 +837,9 @@ mod tests {
|
||||
let result = validate_config_file(object, source, &test_path());
|
||||
|
||||
// then
|
||||
assert_eq!(result.errors.len(), 1);
|
||||
assert_eq!(result.errors[0].field, "oauth.secret");
|
||||
assert!(result.errors.is_empty());
|
||||
assert_eq!(result.warnings.len(), 1);
|
||||
assert_eq!(result.warnings[0].field, "oauth.secret");
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -866,8 +874,9 @@ mod tests {
|
||||
let result = validate_config_file(object, source, &test_path());
|
||||
|
||||
// then
|
||||
assert_eq!(result.errors.len(), 1);
|
||||
match &result.errors[0].kind {
|
||||
assert!(result.errors.is_empty());
|
||||
assert_eq!(result.warnings.len(), 1);
|
||||
match &result.warnings[0].kind {
|
||||
DiagnosticKind::UnknownKey {
|
||||
suggestion: Some(s),
|
||||
} => assert_eq!(s, "model"),
|
||||
@@ -878,7 +887,7 @@ mod tests {
|
||||
#[test]
|
||||
fn format_diagnostics_includes_all_entries() {
|
||||
// given
|
||||
let source = r#"{"permissionMode": "plan", "badKey": 1}"#;
|
||||
let source = r#"{"model": 42, "badKey": 1}"#;
|
||||
let parsed = JsonValue::parse(source).expect("valid json");
|
||||
let object = parsed.as_object().expect("object");
|
||||
let result = validate_config_file(object, source, &test_path());
|
||||
@@ -890,7 +899,7 @@ mod tests {
|
||||
assert!(output.contains("warning:"));
|
||||
assert!(output.contains("error:"));
|
||||
assert!(output.contains("badKey"));
|
||||
assert!(output.contains("permissionMode"));
|
||||
assert!(output.contains("model"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user