feat: use Facet rs

[dougEfresh]

To the reviewer:

DO NOT comment on the following:

• Documentation
• CHANGLOG
• Breaking format changes
• CLI output formats

I will fix this stuff in another pull request. For now focus on the logic of the application and that there is sufficient test cases.

I had to switch (again) to a different kdl parser lib. I am using facet-kdl: https://github.com/facet-rs/facet

This caused a complete config re-write to use this lib.

[claude]

Claude finished @dougEfresh's task in 4m 28s —— View job

---

Code Review Complete

I've completed a comprehensive review of this PR focusing on application logic and test coverage per your request. Overall, the migration to facet-kdl is well-executed with good test coverage. Here are my findings:

---

Summary Checklist

Code Quality

• Code follows style guide - Clean, idiomatic Rust
• No commented-out code - Only TODO markers for incomplete features
• Meaningful variable names - Clear and descriptive throughout
• DRY principle followed - Good use of macros for repetitive patterns

Testing

• Unit tests for new functions - Comprehensive coverage in config module
• Integration tests for new endpoints - Merge logic well tested
• Edge cases covered - Good coverage of empty states, circular deps
• Test coverage > 80% - Appears solid based on extensive test modules

Security

• No hardcoded credentials - Clean
• Input validation implemented - Parser handles validation
• Proper error handling - Good use of Result types
• No sensitive data in logs - Clear

---

Detailed Findings

Strengths

1. Excellent Test Coverage

The new config module has comprehensive test coverage:

• src/config/agent.rs: Tests agent decoding, file sources, empty agents (lines 156-388)
• src/config/native.rs: Tests all native tool types with 18 test cases covering parsing, merging, and conversions (lines 238-536)
• src/config/hook.rs: Tests hook parsing, merging, and edge cases (lines 148-286)
• src/config/mcp.rs: Tests MCP config parsing with env/headers (lines 67-183)
• src/config/merge.rs: Tests complex inheritance chains and transitive resolution (lines 28-186)
• src/generator/merge.rs: Integration test for complete inheritance chain (lines 73-160)

2. Well-Structured Architecture

The modularization is excellent:

• Clear separation: src/config/ for parsing, src/agent/ for business logic
• Good use of facet-kdl derive macros reduces boilerplate
• Consistent patterns across modules (agent, hook, mcp, native)

3. Macro-Based DRY Pattern

Excellent use of macros in src/config/native.rs:

• define_tool!, define_kdl_doc!, define_tool_into! macros eliminate repetition
• All 4 tool types (Shell, Aws, Read, Write) use same pattern
• Makes maintenance easier

4. Strong Error Handling

• Good use of miette for diagnostic errors (src/error.rs)
• Custom error display in src/output.rs:15-24 with graphical reports
• Proper propagation with Result types throughout

5. Circular Dependency Detection

src/generator/merge.rs:7-41 has robust cycle detection in inheritance chains - critical for preventing infinite loops.

---

Issues Found

Critical Issues: None 🎉

Major Issues

1. Incomplete Feature: tool_setting (src/config/agent.rs:44-61)

The ToolSetting::to_value() method is completely todo!() with commented-out code:

fn to_value(&self) -> crate::Result<(String, serde_json::Value)> {
    todo!()
    // Commented out implementation
}

This feature appears unused (line 160: tool_setting: Default::default()), but it's still in the public API.

Recommendation: Either:

• Complete the implementation, OR
• Remove it entirely if not needed for this release, OR
• Add #[allow(dead_code)] and a clear TODO comment explaining when it will be implemented

Impact: Medium - Feature is incomplete but appears unused. Could cause issues if someone tries to use it.

2. Multiple Unwraps in Production Code (Not Critical but Should Be Addressed)

Found several unwrap() calls in non-test code:

• src/config.rs:74 - GraphicalReportHandler render
• src/output.rs:20 - Same pattern

Most are in error reporting paths, but still risky.

Recommendation: Replace with proper error handling:

// Instead of:
handler.render_report(&mut output, kdl_err).unwrap();

// Use:
let _ = handler.render_report(&mut output, kdl_err);
// or
handler.render_report(&mut output, kdl_err)
    .unwrap_or_else(|e| eprintln!("Failed to render error: {}", e));

Impact: Low-Medium - Only affects error reporting, not core logic

---

Minor Issues

3. Generic Type Names Lack Context (src/config.rs:23-50)

Types like GenericItem, GenericSet, GenericVec, IntDoc are very generic names. While they work, more specific names would improve code readability:

// Current:
pub(super) struct GenericItem { pub item: String }

// Consider:
pub(super) struct KdlArgumentItem { pub value: String }

Recommendation: Consider renaming in a follow-up PR when you address documentation.

Impact: Very Low - Code works fine, just a readability suggestion

4. Conversion Logic Could Be More Explicit (src/config.rs:44-51)

The chunks_exact(2) conversion to HashMap silently drops odd-length vectors:

impl From<GenericVec> for HashMap<String, String> {
    fn from(list: GenericVec) -> HashMap<String, String> {
        list.item
            .chunks_exact(2)
            .map(|chunk| (chunk[0].clone(), chunk[1].clone()))
            .collect()
    }
}

Recommendation: Add validation or at least a debug assertion that the length is even.

Impact: Low - Parser likely enforces correct pairing, but defensive programming would be better

5. Test-Only Function Could Use #[cfg(test)] (src/config.rs:53-58)

#[cfg(test)]
impl GenericVec {
    fn len(&self) -> usize {  // Good, but...
        self.item.len()
    }
}

This is correct, but you could also expose item.len() directly in tests since item is public within the module.

Impact: Negligible - More of a style point

---

Security Review

✅ No security issues found:

• No hardcoded credentials or secrets
• No SQL injection risks (no SQL)
• No command injection issues (commands are from config files)
• Input validation delegated to facet-kdl parser
• Proper error handling throughout
• Override permissions in tools properly handled with explicit allow/deny/override

---

Architecture & Design

Positive Observations:

1. Clean Module Boundaries - Config parsing separated from business logic
2. Type Safety - Strong typing with HashSet/HashMap for collections prevents duplicates
3. Immutability - Most operations return new values rather than mutating
4. Merge Logic - Well-tested inheritance resolution with cycle detection

Minor Design Note:

The merge strategy is "child wins for explicit values, extend for collections" (src/config/merge.rs:4-25). This is clearly documented in tests and appears intentional. Good job making this explicit.

---

Performance Considerations

Per your CLAUDE.md:

Runtime Performance is NOT critical or important... far MORE important that the code is clean, maintainable and SIMPLE

✅ The code prioritizes clarity over performance, which aligns perfectly with your guidelines. Clone operations are used liberally, which is fine for a CLI tool.

---

Test Quality Assessment

Test Coverage: Excellent

The test suite covers:

• ✅ Happy path cases
• ✅ Empty/default states
• ✅ Merge scenarios (empty child, empty parent, both populated)
• ✅ Complex inheritance chains
• ✅ Circular dependency detection
• ✅ All tool types (native, mcp, hooks)
• ✅ Conversion logic (KDL → Internal → Agent types)

Missing Test Cases: None critical, but could add:

• Malformed KDL (parser should handle, but explicit tests would be nice)
• Very deep inheritance chains (100+ levels)
• Hash collision scenarios (unlikely with good hash functions)

---

Recommendations Summary

Must Fix (Before Merge):

1. Complete or remove tool_setting feature - It's a landmine waiting for someone to use

Should Fix (This PR or Next):

2. Replace unwraps in error handling paths with proper error handling
3. Add validation to chunks_exact(2) conversion or document assumption

Nice to Have (Future PR):

4. Rename generic types for clarity
5. Add more edge case tests (malformed input, deep chains)

---

Final Assessment

Approval Status: ✅ Approve (with minor fixes)

This is solid work. The migration to facet-kdl is well-executed with excellent test coverage. The incomplete tool_setting feature is the only real concern - please address that before merging.

The code is clean, well-tested, and follows Rust best practices. The macro-based approach for tool definitions is particularly elegant. Great job maintaining test coverage through a major refactor!

What I Loved:

• Comprehensive test suite that survived the parser migration
• Clean module structure
• Macro-based DRY patterns
• Circular dependency detection
• Strong typing preventing common errors

What Needs Attention:

• Complete or remove tool_setting feature
• A few unwraps in error paths (minor)

---

Bottom Line: This PR significantly improves the codebase. The parser migration is well-executed, and the application logic is sound. Fix the tool_setting TODO and you're good to merge. 🚀

---

[codecov]

Codecov Report

❌ Patch coverage is 88.49145% with 74 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/generator/mod.rs	0.00%	13 Missing ⚠️
src/config.rs	81.96%	11 Missing ⚠️
src/agent/mod.rs	75.60%	10 Missing ⚠️
src/config/native.rs	91.96%	9 Missing ⚠️
src/output.rs	52.94%	8 Missing ⚠️
src/main.rs	33.33%	6 Missing ⚠️
src/agent/hook.rs	80.00%	5 Missing ⚠️
src/config/agent.rs	93.75%	5 Missing ⚠️
src/generator/merge.rs	90.00%	3 Missing ⚠️
src/generator/discover.rs	90.00%	2 Missing ⚠️
... and 2 more

Flag	Coverage Δ
default	77.45% <88.49%> (-0.22%)	⬇️

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%> (ø)
src/commands.rs	92.41% <100.00%> (ø)
src/config/hook.rs	100.00% <100.00%> (ø)
src/config/mcp.rs	100.00% <100.00%> (ø)
src/config/merge.rs	100.00% <100.00%> (ø)
src/config/agent_file.rs	96.77% <96.77%> (ø)
src/generator/config_location.rs	72.72% <0.00%> (ø)
src/generator/discover.rs	95.32% <90.00%> (-0.06%)	⬇️
src/generator/merge.rs	93.06% <90.00%> (+0.67%)	⬆️
src/agent/hook.rs	87.09% <80.00%> (-5.01%)	⬇️
... and 7 more

🚀 New features to boost your workflow:

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