feat: allow raw json

[dougEfresh]

No description provided.

[claude]

Claude finished @dougEfresh's task in 2m 39s —— View job

---

Code Review Complete

Summary

This PR adds support for raw JSON tool settings through a new tool-setting KDL node, allowing users to configure tools not yet directly supported by the kg schema. The implementation is generally solid with good test coverage, but there are several issues ranging from error handling concerns to missing documentation.

---

Review Checklist Results

Code Quality ❌

• Code follows style guide (nightly fmt)
• No commented-out code
• Meaningful variable names
• DRY principle followed
• Issues Found: See below

Testing ⚠️

• Unit tests for new functions (test in src/kdl/mod.rs:99-165)
• Missing: Edge case tests for malformed JSON
• Missing: Tests for non-object JSON values (arrays, primitives)
• Missing: Integration test for full agent generation with tool-setting
• Note: Cannot verify test coverage % without running suite

Documentation ❌

• Missing: README not updated to mention new tool-setting feature
• Missing: No CHANGELOG.md entry
• Inline comments present for complex logic
• Doc comments on public types

Security ✅

• No hardcoded credentials
• Input validation implemented (JSON parsing + object check)
• Proper error handling with wrap_err_with
• No sensitive data in logs

---

Critical Issues

1. Insufficient Error Context (src/agent/mod.rs:137)

Location: src/agent/mod.rs:137

The error handling violates CLAUDE.md guidelines for error context. When extra_tool_settings() fails, the user doesn't know which agent caused the problem.

Current:

let extra_tool_settings = value.extra_tool_settings()?;

Should be:

let extra_tool_settings = value.extra_tool_settings()
    .wrap_err_with(|| format!("Failed to process tool settings for agent '{}'", value.name))?;

Per CLAUDE.md lines 113-116: "Add context at each layer where it's meaningful" and "Provide user-actionable information when possible."

---

2. Breaking Change to Public API (src/agent/mod.rs:64)

Location: src/agent/mod.rs:64

The type of tools_settings changed from HashMap<ToolTarget, serde_json::Value> to HashMap<String, serde_json::Value>. This is a breaking change that affects:

• Any code deserializing existing agent JSON files
• Any code programmatically accessing agent.tools_settings

Issue: The PR title says "feat:" but this includes a breaking change that should be documented. The old format used enum keys like ToolTarget::Aws, the new format uses string keys like "aws".

Recommendation: Either:

1. Add migration logic to handle both formats during deserialization
2. Clearly document this as a breaking change in commit message and require a major version bump
3. Consider if backward compatibility is needed

---

3. Inconsistent Error Handling Pattern (src/agent/mod.rs:106-130)

Location: src/agent/mod.rs:106-130

Uses .unwrap() for native tool serialization but proper error handling for extra tool settings. This is inconsistent and could panic at runtime.

Current:

tools_settings.insert(
    ToolTarget::Aws.to_string(),
    serde_json::to_value(&tool).unwrap(),  // ⚠️ Can panic
);

Should be:

tools_settings.insert(
    ToolTarget::Aws.to_string(),
    serde_json::to_value(&tool)
        .wrap_err("Failed to serialize AWS tool configuration")?,
);

This applies to all four native tool insertions (lines 106, 114, 122, 130).

---

Medium Priority Issues

4. Validation Gap: Duplicate tool-setting Names

Location: src/kdl/agent.rs:229-230

No validation prevents duplicate tool-setting entries with the same name. If a user writes:

tool-setting "foo" { json "{}" }
tool-setting "foo" { json "{}" }

The last one silently wins due to HashMap behavior. This could lead to confusing behavior.

Recommendation: Add validation in extra_tool_settings() to detect and error on duplicates, or at minimum document this behavior.

---

5. Missing Merge Test Coverage

Location: src/kdl/merge.rs:21

The merge logic extends tool_settings but there's no test verifying this works correctly. The test file src/kdl/merge.rs has extensive tests for merging other fields but doesn't test tool_settings merge behavior.

Recommendation: Add test case verifying:

• Parent has tool-setting "a", child has tool-setting "b" → merged has both
• Both have tool-setting with same name → child wins (document this!)

---

6. Type Safety Lost

Location: src/agent/mod.rs:64

By switching from HashMap<ToolTarget, serde_json::Value> to HashMap<String, serde_json::Value>, we lose compile-time guarantees that tool names are valid. This makes typos more likely:

// Old: Won't compile with typo
agent.tools_settings.get(&ToolTarget::Aws)

// New: Silently returns None on typo
agent.tools_settings.get("aws")  // vs "Aws" vs "AWS"?

Recommendation: Consider defining string constants for native tool names:

pub mod tool_names {
    pub const AWS: &str = "aws";
    pub const READ: &str = "read";
    pub const WRITE: &str = "write";
    pub const SHELL: &str = "shell";
}

---

Low Priority / Nice-to-Have

7. Unclear JSON Requirement

Location: src/kdl/agent.rs:84-90

The error message says "must be a JSON object" but doesn't explain why. Per CLAUDE.md line 82: "Error messages should be actionable: what failed, why, and how to fix"

Current:

return Err(eyre!(
    "tool-setting '{}' must be a JSON object, got: {}",
    self.name, v
));

Better:

return Err(eyre!(
    "tool-setting '{}' must be a JSON object (not array or primitive), got: {}",
    self.name, v
))
.suggestion("Tool settings must be objects like: { \"key\": \"value\" }")

---

8. Test Could Be More Robust

Location: src/kdl/mod.rs:160-165

Test only checks happy path. Consider adding tests for:

• Invalid JSON syntax
• JSON array instead of object
• JSON primitive instead of object
• Empty JSON object
• Very large JSON (performance)

---

9. Documentation Parity

The comment on line 70 references https://kiro.dev/docs/cli/custom-agents/configuration-reference/#toolssettings-field, but:

1. We can't verify this URL is correct or exists
2. No inline example in the doc comment itself
3. The public API docs should be self-contained

Recommendation: Add example to doc comment:

/// # Example
/// ```kdl
/// tool-setting "@git/status" {
///     json "{ \"git_user\": \"$GIT_USER\" }"
/// }
/// ```

---

Positive Observations

✅ Good use of wrap_err_with in ToolSetting::to_value() (line 82) - includes the tool name in error context

✅ Proper test coverage - Test in src/kdl/mod.rs verifies parsing, access, and JSON structure validation

✅ Clean separation - New functionality is additive and doesn't break existing native tool code paths

✅ Forward compatibility - This design allows users to configure future tools without kg updates

---

Required Actions Before Merge

1. Fix critical error handling issues (Initial CLI design and draft #1, feat: mdbook #3)
2. Document breaking change (feat: enhance output #2) or add backward compatibility
3. Add test for merge behavior (feat: use KDL as configuration  #5)
4. Update README with tool-setting feature documentation
5. Add CHANGELOG entry describing the feature and breaking change

Optional Improvements

• Address validation gap (feat: remove lib #4)
• Add type safety constants (feat: init command #6)
• Improve error messages (feat: allow raw json #7)
• Expand test coverage (feat: use Facet rs #8)
• Enhance doc comments (#9)

---

Implementation Suggestions

For Issue #1 (Error Context):

// In src/agent/mod.rs around line 137
let extra_tool_settings = value.extra_tool_settings()
    .wrap_err_with(|| format!("Failed to process tool settings for agent '{}'", value.name))?;

For Issue #3 (Unwrap in serialization):

// In src/agent/mod.rs, replace all 4 .unwrap() calls
let tool: AwsTool = native_tools.into();
if tool != AwsTool::default() {
    tools_settings.insert(
        ToolTarget::Aws.to_string(),
        serde_json::to_value(&tool)
            .wrap_err("Failed to serialize AWS tool configuration")?,
    );
}

Apply same pattern to Read, Write, and Shell tools.

For Issue #5 (Test merge):

// Add to src/kdl/merge.rs tests
#[test_log::test]
fn test_agent_merge_tool_settings() -> crate::Result<()> {
    let kdl = r#"
        agent "child" {
            tool-setting "child-tool" {
                json "{ \"key\": \"child\" }"
            }
        }
        agent "parent" {
            tool-setting "parent-tool" {
                json "{ \"key\": \"parent\" }"
            }
        }
    "#;
    
    let config: GeneratorConfig = parse("test.kdl", kdl)?;
    let child = config.get("child").unwrap().clone();
    let parent = config.get("parent").unwrap().clone();
    let merged = child.merge(parent);
    
    let settings = merged.extra_tool_settings()?;
    assert_eq!(settings.len(), 2);
    assert!(settings.contains_key("child-tool"));
    assert!(settings.contains_key("parent-tool"));
    Ok(())
}

---

Overall Assessment: The feature implementation is sound and follows good Rust patterns, but needs polish on error handling, documentation, and test coverage before merging. The breaking change to tools_settings HashMap key type needs special attention.

[codecov]

Codecov Report

❌ Patch coverage is 94.21965% with 10 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/kdl/agent.rs	61.53%	10 Missing ⚠️

Flag	Coverage Δ
default	77.66% <94.21%> (+9.18%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/agent/custom_tool.rs	100.00% <100.00%> (+100.00%)	⬆️
src/agent/hook.rs	92.10% <100.00%> (+92.10%)	⬆️
src/agent/mcp_config.rs	100.00% <100.00%> (ø)
src/agent/mod.rs	88.42% <100.00%> (+88.42%)	⬆️
src/agent/tools.rs	96.77% <100.00%> (+96.77%)	⬆️
src/agent/wrapper_types.rs	100.00% <100.00%> (+50.00%)	⬆️
src/commands.rs	92.41% <100.00%> (ø)
src/generator/merge.rs	92.39% <100.00%> (+0.16%)	⬆️
src/generator/mod.rs	46.91% <ø> (+22.22%)	⬆️
src/kdl/agent_file.rs	91.66% <100.00%> (+0.23%)	⬆️
... and 3 more

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

Claude finished @dougEfresh's task in 2m 49s —— View job

---

Code Review Complete

Summary

This PR adds support for raw JSON tool settings through a new tool-setting KDL node, allowing users to configure tools not yet directly supported by the kg schema. While the implementation shows good architectural thinking and test coverage for the happy path, there are critical issues with error handling that violate the project's CLAUDE.md guidelines, plus several gaps in documentation and testing.

---

Review Checklist Results

Code Quality ❌

• Code follows style guide (nightly fmt)
• No commented-out code
• Meaningful variable names
• DRY principle followed
• Critical Issues Found: See below

Testing ⚠️

• Unit tests for new parsing logic (src/kdl/mod.rs:99-165)
• Missing: Edge case tests for malformed JSON
• Missing: Tests for non-object JSON values (arrays, primitives, strings)
• Missing: Tests for duplicate tool-setting entries
• Missing: Integration test for merge behavior with tool-settings
• Missing: Test that verifies extra settings override native tools
• Coverage: Codecov reports 33.33% patch coverage with 24 lines missing coverage (19 in src/agent/mod.rs, 5 in src/kdl/agent.rs)

Documentation ❌

• Missing: README not updated to mention new tool-setting feature
• Missing: No CHANGELOG.md entry (currently only has "## [Unreleased]" header)
• Inline comments present for complex logic
• Doc comments on public types (src/kdl/agent.rs:65-70)
• Issue: Doc comment references external URL but provides no inline example

Security ✅

• No hardcoded credentials
• Input validation implemented (JSON parsing + object type check)
• Proper error handling with wrap_err_with in ToolSetting::to_value()
• No sensitive data in logs
• No obvious injection vulnerabilities

---

Critical Issues (Must Fix Before Merge)

1. Insufficient Error Context in TryFrom (src/agent/mod.rs:137)

Severity: High
Location: src/agent/mod.rs:137

Violates CLAUDE.md error handling guidelines (lines 113-116). When extra_tool_settings() fails, users don't know which agent caused the problem.

Current Code:

let extra_tool_settings = value.extra_tool_settings()?;

Required Fix:

let extra_tool_settings = value.extra_tool_settings()
    .wrap_err_with(|| format!("Failed to process tool settings for agent '{}'", value.name))?;

Why This Matters: Per CLAUDE.md: "Add context at each layer where it's meaningful" and "Provide user-actionable information when possible." Without agent name context, users with multiple agents won't know where to look for the error.

---

2. Panic Risk from .unwrap() on Serialization (src/agent/mod.rs:107, 114, 122, 130)

Severity: High
Location: Four locations in src/agent/mod.rs

Uses .unwrap() for native tool serialization, which can panic at runtime. This is inconsistent with proper error handling used elsewhere and violates the principle of graceful error handling.

Current Pattern (repeated 4 times):

tools_settings.insert(
    ToolTarget::Aws.to_string(),
    serde_json::to_value(&tool).unwrap(),  // ⚠️ Can panic
);

Required Fix:

tools_settings.insert(
    ToolTarget::Aws.to_string(),
    serde_json::to_value(&tool)
        .wrap_err("Failed to serialize AWS tool configuration")?,
);

Apply this pattern to all four native tool insertions (lines 107, 114, 122, 130).

Why This Matters:

• Inconsistent error handling pattern (extra tools use proper error handling, native tools don't)
• Can cause unexpected panics during agent generation
• Violates CLAUDE.md principle of actionable error messages

---

3. Breaking Change Not Documented (src/agent/mod.rs:64)

Severity: Medium-High
Location: src/agent/mod.rs:64

The type of tools_settings changed from HashMap<ToolTarget, serde_json::Value> to HashMap<String, serde_json::Value>. This is a breaking change affecting:

• Any code deserializing existing agent JSON files
• Any code programmatically accessing agent.tools_settings
• External consumers of this library

Issues:

1. PR title says "feat:" but includes breaking API change
2. Old format used enum keys (ToolTarget::Aws), new format uses string keys ("aws")
3. No migration path documented
4. CHANGELOG.md not updated

Required Actions:

1. Add entry to CHANGELOG.md under ## [Unreleased] with:

   ### Changed (BREAKING)
   - Changed `Agent.tools_settings` from `HashMap<ToolTarget, serde_json::Value>` to `HashMap<String, serde_json::Value>` to support custom tool settings

2. Update README to document the new tool-setting KDL node
3. Consider if backward compatibility is needed, or document that this requires a major version bump

---

High Priority Issues

4. No Validation for Duplicate tool-setting Names

Severity: Medium
Location: src/kdl/agent.rs:229-230

No validation prevents duplicate tool-setting entries with the same name:

tool-setting "foo" { json "{}" }
tool-setting "foo" { json "{}" }  // Silently wins

Due to HashMap behavior, last entry wins with no warning. This could confuse users.

Recommendation: Add validation in extra_tool_settings() to detect and error on duplicates:

pub fn extra_tool_settings(&self) -> crate::Result<HashMap<String, serde_json::Value>> {
    let mut seen = HashSet::new();
    let result: HashMap<_, _> = self.tool_settings.iter().map(|t| {
        if !seen.insert(&t.name) {
            return Err(color_eyre::eyre::eyre!(
                "Duplicate tool-setting '{}' found. Each tool-setting name must be unique.", 
                t.name
            ));
        }
        t.to_value()
    }).collect::<Result<Vec<_>>>()?
      .into_iter()
      .collect();
    Ok(result)
}

---

5. Missing Test Coverage for Merge Behavior

Severity: Medium
Location: src/kdl/merge.rs:21

The merge logic extends tool_settings (line 21) but there's no test verifying this works correctly. The test file has extensive tests for merging other fields but skips tool_settings.

Required Test (add to src/kdl/merge.rs tests):

#[test_log::test]
fn test_agent_merge_tool_settings() -> crate::Result<()> {
    let kdl = r#"
        agent "child" {
            tool-setting "child-tool" {
                json "{ \"key\": \"child\" }"
            }
        }
        agent "parent" {
            tool-setting "parent-tool" {
                json "{ \"key\": \"parent\" }"
            }
            tool-setting "shared" {
                json "{ \"value\": \"parent\" }"
            }
        }
    "#;
    
    let config: GeneratorConfig = parse("test.kdl", kdl)?;
    let child = config.get("child").unwrap().clone();
    let parent = config.get("parent").unwrap().clone();
    let merged = child.merge(parent);
    
    let settings = merged.extra_tool_settings()?;
    assert_eq!(settings.len(), 2);
    assert!(settings.contains_key("child-tool"));
    assert!(settings.contains_key("parent-tool"));
    
    // Test that duplicate from both extends (child wins)
    // Add child with "shared" and verify child value wins
    Ok(())
}

This test should verify:

• Parent has tool-setting "a", child has tool-setting "b" → merged has both
• Both have tool-setting with same name → document which wins (likely child based on Vec::extend)

---

6. Type Safety Loss

Severity: Medium
Location: src/agent/mod.rs:64

By switching from HashMap<ToolTarget, serde_json::Value> to HashMap<String, serde_json::Value>, compile-time guarantees for valid tool names are lost. Typos become runtime issues:

// Old: Won't compile with typo
agent.tools_settings.get(&ToolTarget::Aws)

// New: Silently returns None on typo
agent.tools_settings.get("aws")  // vs "Aws"? "AWS"?

Recommendation: Define string constants for native tool names:

pub mod tool_names {
    pub const AWS: &str = "aws";
    pub const READ: &str = "read";
    pub const WRITE: &str = "write";
    pub const SHELL: &str = "shell";
}

Then use: tools_settings.insert(tool_names::AWS, ...)

---

Medium Priority Issues

7. Error Message Could Be More Helpful (src/kdl/agent.rs:84-90)

Severity: Low
Location: src/kdl/agent.rs:84-90

Per CLAUDE.md line 82: "Error messages should be actionable: what failed, why, and how to fix"

Current:

return Err(eyre!(
    "tool-setting '{}' must be a JSON object, got: {}",
    self.name, v
));

Better:

return Err(eyre!(
    "tool-setting '{}' must be a JSON object (not array or primitive), got: {}",
    self.name, v
))
.suggestion("Tool settings must be objects like: {{ \"key\": \"value\" }}")
.suggestion("Arrays and primitives are not supported as tool settings")

---

8. Test Coverage Gaps

Severity: Low
Location: src/kdl/mod.rs:99-168

Test only covers happy path. Add tests for error cases:

#[test_log::test]
fn test_tool_setting_invalid_json() -> crate::Result<()> {
    let kdl = r#"
        agent "test" {
            tool-setting "bad" {
                json "{ invalid json }"
            }
        }
    "#;
    let result: Result<GeneratorConfig> = parse("test.kdl", kdl);
    assert!(result.is_err());
    assert!(result.unwrap_err().to_string().contains("Failed to parse JSON"));
    Ok(())
}

#[test_log::test]
fn test_tool_setting_array_rejected() -> crate::Result<()> {
    let kdl = r#"
        agent "test" {
            tool-setting "array" {
                json "[1, 2, 3]"
            }
        }
    "#;
    let config: GeneratorConfig = parse("test.kdl", kdl)?;
    let agent = &config.agents[0];
    let result = agent.extra_tool_settings();
    assert!(result.is_err());
    assert!(result.unwrap_err().to_string().contains("must be a JSON object"));
    Ok(())
}

#[test_log::test]
fn test_tool_setting_primitive_rejected() -> crate::Result<()> {
    let kdl = r#"
        agent "test" {
            tool-setting "primitive" {
                json "\"just a string\""
            }
        }
    "#;
    let config: GeneratorConfig = parse("test.kdl", kdl)?;
    let agent = &config.agents[0];
    let result = agent.extra_tool_settings();
    assert!(result.is_err());
    Ok(())
}

---

9. Documentation Improvements

Missing from README: No mention of the new feature. Add section:

### Custom Tool Settings

For tools not yet supported by kg's schema, use the `tool-setting` node:

\`\`\`kdl
agent "my-agent" {
    tool-setting "@git/status" {
        json "{ \"git_user\": \"$GIT_USER\" }"
    }
}
\`\`\`

The JSON value must be a valid JSON object (arrays and primitives are not supported).

Doc Comment Enhancement (src/kdl/agent.rs:65-70):

/// Raw JSON tool settings for forward compatibility.
///
/// Allows users to configure tool settings not yet supported by kg's schema.
/// The JSON must be a valid object (not array or primitive).
///
/// # Example
/// ```kdl
/// tool-setting "@git/status" {
///     json "{ \"git_user\": \"$GIT_USER\" }"
/// }
/// ```
///
/// See https://kiro.dev/docs/cli/custom-agents/configuration-reference/#toolssettings-field

---

Positive Observations ✅

• Good architectural design: Allows forward compatibility without schema updates
• Proper separation of concerns: New functionality is additive, doesn't break native tool code paths
• Test coverage for happy path: Test in src/kdl/mod.rs verifies parsing and validation
• Good use of wrap_err_with in ToolSetting::to_value() - includes tool name in error context
• CI enhancement: Added tests for init and validate --global commands

---

Required Actions Before Merge

Must Fix (Blocking)

1. ✅ Fix error context in src/agent/mod.rs:137 (Issue Initial CLI design and draft #1)
2. ✅ Replace .unwrap() with proper error handling in src/agent/mod.rs:107, 114, 122, 130 (Issue feat: enhance output #2)
3. ✅ Add CHANGELOG.md entry documenting breaking change (Issue feat: mdbook #3)
4. ✅ Add README section documenting new tool-setting feature (Issue #9)

Should Fix (Strongly Recommended)

5. Add validation for duplicate tool-setting names (Issue feat: remove lib #4)
6. Add test for merge behavior with tool-settings (Issue feat: use KDL as configuration  #5)
7. Add error case tests (invalid JSON, arrays, primitives) (Issue feat: use Facet rs #8)

Nice to Have (Optional)

8. Add type safety constants for native tool names (Issue feat: init command #6)
9. Improve error messages with suggestions (Issue feat: allow raw json #7)
10. Enhance doc comment with inline example (Issue #9)

---

Code Examples for Fixes

For Issue #1 (Error Context):

// In src/agent/mod.rs around line 137
let extra_tool_settings = value.extra_tool_settings()
    .wrap_err_with(|| format!("Failed to process tool settings for agent '{}'", value.name))?;

For Issue #2 (Replace unwrap):

// In src/agent/mod.rs, change the signature
impl TryFrom<&KdlAgent> for Agent {
    type Error = color_eyre::Report;

    fn try_from(value: &KdlAgent) -> std::result::Result<Self, Self::Error> {
        // ... existing code ...
        
        let tool: AwsTool = native_tools.into();
        if tool != AwsTool::default() {
            tools_settings.insert(
                ToolTarget::Aws.to_string(),
                serde_json::to_value(&tool)
                    .wrap_err("Failed to serialize AWS tool configuration")?,
            );
        }
        
        // Repeat for ReadTool, WriteTool, ExecuteShellTool
    }
}

For Issue #4 (Duplicate Detection):

pub fn extra_tool_settings(&self) -> crate::Result<HashMap<String, serde_json::Value>> {
    let mut result = HashMap::new();
    for setting in &self.tool_settings {
        let (name, value) = setting.to_value()?;
        if result.contains_key(&name) {
            return Err(color_eyre::eyre::eyre!(
                "Duplicate tool-setting '{}' found. Each tool-setting name must be unique.", 
                name
            ));
        }
        result.insert(name, value);
    }
    Ok(result)
}

---

Test Execution Note

⚠️ I cannot run the test suite in this environment due to permissions and build requirements. Please run locally:

cargo +nightly fmt --all
cargo test
cargo clippy -- -D warnings

The Codecov report shows 24 lines missing coverage, primarily in the new code paths. Adding the recommended tests should improve coverage significantly.

---

Final Assessment

Overall: The feature implementation is architecturally sound and adds useful functionality. However, the critical error handling issues (#1, #2) must be fixed before merge as they violate project guidelines and can cause poor user experience or panics. The breaking change (#3) needs proper documentation. With these fixes and improved test coverage, this will be a solid addition to the codebase.

Recommendation: Request changes for critical issues, strongly recommend addressing high-priority issues for code quality and maintainability.

[dougEfresh]

@Jobtaker,
i won't support fully merging behavior for raw json

[claude]

Claude finished @dougEfresh's task in 2m 49s —— View job

---

Updated Code Review Complete

Thank you for the updates! I've reviewed the current state of the PR. I understand your decision not to support fully merging behavior for raw JSON - that's a reasonable design choice. Let me provide an updated review based on what's been addressed and what still needs attention.

---

Summary

The implementation has significantly improved since the last review. Critical error handling issues have been fixed, and duplicate validation has been added. However, documentation is still missing (CHANGELOG, README), and there are a few remaining testing gaps.

---

✅ Issues Successfully Addressed

1. Fixed: Panic Risk from .unwrap() (Previously Critical Issue #2)

Location: src/agent/mod.rs:107-137

Excellent fix! All four native tool serializations now use proper error handling with .map_err():

serde_json::to_value(&tool)
    .map_err(|e| eyre!("Failed to serialize {tool_name} tool configuration {e}"))?,

This is much better than the previous .unwrap() and provides actionable error messages.

2. Fixed: Duplicate tool-setting Validation (Previously Issue #4)

Location: src/kdl/agent.rs:233-238

Great addition! The code now properly detects and errors on duplicate tool-setting entries:

if result.contains_key(&name) {
    return Err(color_eyre::eyre::eyre!(
        "[{self}] - Duplicate tool-setting '{}' found. Each tool-setting name must be unique.",
        name
    ));
}

The error message includes the agent name context ([{self}]), which is helpful for debugging.

3. Good: Test Coverage for Happy Path

Location: src/kdl/mod.rs:105-168

The test validates:

• Parsing of tool-setting nodes
• JSON parsing and object validation
• Access via extra_tool_settings()

---

❌ Critical Issues Remaining

1. Missing CHANGELOG Entry (Breaking Change Documentation)

Severity: High
Location: CHANGELOG.md

The CHANGELOG.md still only contains:

## [Unreleased]

Required Action: Document the breaking change to tools_settings HashMap key type:

## [Unreleased]

### Added
- Support for raw JSON tool settings via `tool-setting` KDL node
- Validation to prevent duplicate tool-setting names

### Changed (BREAKING)
- Changed `Agent.tools_settings` from `HashMap<ToolTarget, serde_json::Value>` to `HashMap<String, serde_json::Value>` to support custom tool settings
- Native tool names now use lowercase string keys ("aws", "read", "write", "shell") instead of enum variants

Why This Matters: Per CLAUDE.md and semantic versioning best practices, breaking API changes must be documented. External consumers of this library need to know about the type change.

---

2. Missing README Documentation

Severity: Medium-High
Location: README.md

The README has no mention of the new tool-setting feature. Users won't discover this functionality.

Required Action: Add a section documenting the feature. Suggested addition after line 58:

### Custom Tool Settings

For tools not yet supported by kg's native schema, you can use raw JSON tool settings:

\`\`\`kdl
agent "my-agent" {
    tool-setting "@git/status" {
        json "{ \"git_user\": \"$GIT_USER\" }"
    }
}
\`\`\`

**Note**: The JSON value must be a valid JSON object. Arrays and primitive values are not supported.

Custom tool settings will override native tool settings if they share the same name.

---

⚠️ Medium Priority Issues

3. Missing Test: Merge Behavior with tool-settings

Severity: Medium
Location: src/kdl/merge.rs (tests section)

The merge logic extends tool_settings at line 21, but there's no test verifying this works correctly. Your comment "i won't support fully merging behavior for raw json" suggests you want simple extension behavior (child + parent), which is what's implemented. This is fine, but it should be tested.

Recommendation: Add a test to src/kdl/merge.rs tests (after line 205):

#[test_log::test]
fn test_agent_merge_tool_settings() -> crate::Result<()> {
    let kdl = r#"
        agent "child" {
            tool-setting "child-only" {
                json "{ \"key\": \"child\" }"
            }
            inherits "parent"
        }
        agent "parent" template=true {
            tool-setting "parent-only" {
                json "{ \"key\": \"parent\" }"
            }
        }
    "#;
    
    let config: GeneratorConfig = parse("test.kdl", kdl)?;
    let child = config.agents.iter().find(|a| a.name == "child").unwrap().clone();
    let parent = config.agents.iter().find(|a| a.name == "parent").unwrap().clone();
    let merged = child.merge(parent);
    
    let settings = merged.extra_tool_settings()?;
    
    // Both tool settings should be present (simple extension, not deep merge)
    assert_eq!(settings.len(), 2);
    assert!(settings.contains_key("child-only"));
    assert!(settings.contains_key("parent-only"));
    
    Ok(())
}

Also test the duplicate scenario (child and parent both define same tool-setting name):

#[test_log::test]
fn test_agent_merge_tool_settings_duplicate_names() -> crate::Result<()> {
    let kdl = r#"
        agent "child" {
            tool-setting "shared" {
                json "{ \"source\": \"child\" }"
            }
            inherits "parent"
        }
        agent "parent" template=true {
            tool-setting "shared" {
                json "{ \"source\": \"parent\" }"
            }
        }
    "#;
    
    let config: GeneratorConfig = parse("test.kdl", kdl)?;
    let child = config.agents.iter().find(|a| a.name == "child").unwrap().clone();
    let parent = config.agents.iter().find(|a| a.name == "parent").unwrap().clone();
    let merged = child.merge(parent);
    
    // This should error because of duplicate name
    let result = merged.extra_tool_settings();
    assert!(result.is_err());
    let err = result.unwrap_err().to_string();
    assert!(err.contains("Duplicate tool-setting 'shared'"));
    
    Ok(())
}

This documents the current behavior: merging creates a Vec extension, and duplicate detection happens in extra_tool_settings().

---

4. Missing Test: Error Cases

Severity: Low-Medium
Location: src/kdl/mod.rs (tests section)

Current test only covers the happy path. Add tests for error conditions:

#[test_log::test]
fn test_tool_setting_invalid_json() {
    let kdl = r#"
        agent "test" {
            tool-setting "bad" {
                json "{ invalid }"
            }
        }
    "#;
    let result: Result<GeneratorConfig, _> = parse("test.kdl", kdl);
    assert!(result.is_err());
}

#[test_log::test]
fn test_tool_setting_array_rejected() -> crate::Result<()> {
    let kdl = r#"
        agent "test" {
            tool-setting "array" {
                json "[1, 2, 3]"
            }
        }
    "#;
    let config: GeneratorConfig = parse("test.kdl", kdl)?;
    let agent = &config.agents[0];
    let result = agent.extra_tool_settings();
    assert!(result.is_err());
    let err = result.unwrap_err().to_string();
    assert!(err.contains("must be a JSON object"));
    Ok(())
}

#[test_log::test]
fn test_tool_setting_primitive_rejected() -> crate::Result<()> {
    let kdl = r#"
        agent "test" {
            tool-setting "string" {
                json "\"just a string\""
            }
        }
    "#;
    let config: GeneratorConfig = parse("test.kdl", kdl)?;
    let agent = &config.agents[0];
    let result = agent.extra_tool_settings();
    assert!(result.is_err());
    Ok(())
}

---

💡 Nice-to-Have Improvements

5. Enhanced Error Message with Suggestions

Severity: Low
Location: src/kdl/agent.rs:84-90

Per CLAUDE.md guidelines on actionable error messages, consider adding suggestions:

if !v.is_object() {
    return Err(color_eyre::eyre::eyre!(
        "tool-setting '{}' must be a JSON object (not array or primitive), got: {}",
        self.name,
        v
    ))
    .suggestion("Tool settings must be objects like: { \"key\": \"value\" }")
    .suggestion("Arrays like [1, 2, 3] and primitives like \"string\" are not supported");
}

6. Doc Comment Enhancement

Severity: Low
Location: src/kdl/agent.rs:65-70

The doc comment references an external URL but lacks an inline example. Consider adding:

/// Raw JSON tool settings for forward compatibility.
///
/// Allows users to configure tool settings not yet supported by kg's schema.
/// The JSON must be a valid object (not array or primitive).
///
/// # Example
/// ```kdl
/// tool-setting "@git/status" {
///     json "{ \"git_user\": \"$GIT_USER\" }"
/// }
/// ```
///
/// See https://kiro.dev/docs/cli/custom-agents/configuration-reference/#toolssettings-field

---

Review Checklist Results

Code Quality ✅

• Code follows style guide (nightly fmt)
• No commented-out code
• Meaningful variable names
• DRY principle followed
• Error handling uses proper patterns (fixed!)

Testing ⚠️

• Unit test for tool-setting parsing and validation
• Missing: Test for merge behavior with tool-settings
• Missing: Tests for error cases (invalid JSON, arrays, primitives)
• Missing: Test for duplicate detection across inheritance
• Coverage: Codecov reports 33.33% patch coverage (24/36 lines missing coverage in src/agent/mod.rs)

Documentation ❌

• Missing: README section for tool-setting feature
• Missing: CHANGELOG.md entry documenting breaking change
• Inline comments present
• Doc comments on public types

Security ✅

• No hardcoded credentials
• Input validation (JSON parsing + object type check + duplicate detection)
• Proper error handling with context
• No sensitive data in logs

---

Required Actions Before Merge

Must Fix (Blocking)

1. ✅ Fix .unwrap() panic risk (DONE)
2. ✅ Add duplicate validation (DONE)
3. ❌ Add CHANGELOG.md entry documenting the breaking change (see suggestion above)
4. ❌ Add README documentation for the tool-setting feature (see suggestion above)

Strongly Recommended

5. Add test for merge behavior with tool-settings (documents current extension behavior)
6. Add tests for error cases (invalid JSON, arrays, primitives)

Optional Improvements

7. Enhance error messages with suggestions (CLAUDE.md compliance)
8. Add inline example to doc comment

---

Positive Observations ✅

1. Excellent error handling improvements - The switch from .unwrap() to .map_err() with descriptive messages is exactly what was needed
2. Thoughtful validation - Duplicate detection with agent context in error message
3. Clean architecture - Extra tool settings override native tools (line 146), which is the right design
4. Good test coverage for happy path - The main parsing flow is well tested
5. Forward compatibility - This feature enables users to configure new tools without waiting for kg updates

---

Assessment & Recommendation

Overall: The implementation quality has significantly improved. Critical error handling issues are fixed, and the code now follows proper Rust error handling patterns. The main gaps are in documentation (CHANGELOG, README) which are essential for users to discover and understand this feature.

Merge Recommendation: Request changes - documentation must be added before merge. The breaking change needs to be documented in CHANGELOG, and users need to know about the feature via README. Testing improvements are strongly recommended but not strictly blocking.

---

Understanding Your Merge Decision

Regarding your comment "i won't support fully merging behavior for raw json" - I understand this means you want simple Vec extension (child tool-settings + parent tool-settings) rather than deep merging of JSON objects with the same name. This is implemented correctly at line 21 of src/kdl/merge.rs:

self.tool_settings.extend(other.tool_settings);

The duplicate detection at extra_tool_settings() time will catch conflicts, which is a reasonable design. A test would document this behavior for future maintainers.

---