Add TDD tests and implementation plan for API enhancements#2
Conversation
This commit introduces comprehensive test coverage for 10 planned API improvements leveraging generics and DSL patterns: - TypedTool: Generic protocol with type-safe outputs - ToolParameterBuilder: Result builder DSL for tool parameters - AgentBuilder: Declarative agent construction DSL - Pipeline: Type-safe composition operators (>>>) - ContextKey: Generic typed context access - Fluent Resilience: withRetry(), withFallback(), withCircuitBreaker() - MemoryBuilder: Composable memory system DSL - Agent Operators: +, >>>, | for agent composition - InferenceOptions: Fluent configuration builder - Stream Operations: Filter, map, collect on AsyncThrowingStream Tests follow TDD approach - they specify expected behavior before implementation. See IMPLEMENTATION_PLAN.md for detailed roadmap.
This commit introduces a complete suite of API enhancements leveraging Swift's generics system and result builder DSL patterns to improve type safety and developer ergonomics. New Features: - TypedTool Protocol: Type-safe tool outputs with associated Output type - ToolParameterBuilder: Result builder DSL for declarative parameter definitions - AgentBuilder: SwiftUI-like syntax for constructing agents with components - Pipeline Operators: Type-safe >>> operator for composing transformations - ContextKey: Generic typed keys for compile-time safe context access - Agent Composition: &+ (parallel), ~> (sequential), |? (fallback) operators - ResilientAgent: Fluent withRetry/withCircuitBreaker/withFallback/withTimeout - MemoryBuilder: Result builder for composing memory systems - StreamOperations: Functional operators on AsyncThrowingStream<AgentEvent> - InferenceOptions: Fluent builder API with presets (creative, precise, etc.) Key Files Added: - Sources/SwiftAgents/Tools/TypedTool.swift - Sources/SwiftAgents/Tools/ToolParameterBuilder.swift - Sources/SwiftAgents/Agents/AgentBuilder.swift - Sources/SwiftAgents/Orchestration/Pipeline.swift - Sources/SwiftAgents/Orchestration/ContextKey.swift - Sources/SwiftAgents/Orchestration/AgentOperators.swift - Sources/SwiftAgents/Resilience/ResilientAgent.swift - Sources/SwiftAgents/Memory/MemoryBuilder.swift - Sources/SwiftAgents/Core/StreamOperations.swift Modified: - Sources/SwiftAgents/Core/Agent.swift (fluent InferenceOptions) - Sources/SwiftAgents/Core/SendableValue.swift (Codable encoding/decoding)
This commit introduces a comprehensive macro system to significantly reduce boilerplate when creating tools and agents in SwiftAgents. New Macros: - @tool: Generates Tool protocol conformance from a simple struct - Derives name from type name (e.g., CalculatorTool -> "calculator") - Uses macro argument as description - Collects @parameter properties into parameters array - Generates execute(arguments:) wrapper with type extraction - @parameter: Marks properties as tool parameters - Supports description, default values, and oneOf constraints - Type mapping: String, Int, Double, Bool, arrays - @agent: Generates Agent protocol conformance for actors - Creates all required properties (tools, instructions, config, memory) - Generates init, run(), stream(), and cancel() implementations - Calls user's process() method for custom logic - @Traceable: Adds automatic observability to tools - Generates executeWithTracing() method - Records TraceEvents for start, result, and errors - #Prompt: Freestanding macro for type-safe prompt building - Compile-time validation of interpolations - Returns PromptString with interpolation tracking Example Usage (Before): ```swift struct WeatherTool: Tool, Sendable { let name = "weather" let description = "Gets weather" let parameters: [ToolParameter] = [ ToolParameter(name: "location", description: "City", type: .string) ] func execute(arguments: [String: SendableValue]) async throws -> SendableValue { guard let location = arguments["location"]?.stringValue else { throw ... } return .string("72°F") } } ``` Example Usage (After): ```swift @tool("Gets weather") struct WeatherTool { @parameter("City") var location: String func execute() async throws -> String { "72°F in \(location)" } } ``` Files Added: - Package.swift: Added swift-syntax dependency and SwiftAgentsMacros target - Sources/SwiftAgentsMacros/: Macro implementations - Sources/SwiftAgents/Macros/MacroDeclarations.swift: Public macro interface - Tests/SwiftAgentsMacrosTests/: Comprehensive macro tests - MACRO_IMPLEMENTATION_PLAN.md: Implementation documentation
- Add Swift Macros and DSL & Operators to features list - Simplify Custom Tools section to use @tool macro - Add comprehensive "Macros & DSL API" section with examples: - @tool and @parameter macros - @agent macro - Agent Builder DSL with result builders - Pipeline operators (>>>) - Agent composition operators (&+, ~>, |?) - Fluent resilience patterns - Memory builder DSL - Stream operations - Typed context keys - InferenceOptions presets - Update Architecture diagram to include SwiftAgentsMacros layer
1 similar comment
|
Claude finished @christopherkarani's task in 2m 54s —— View job Code Review Complete ✅I've completed a comprehensive review of this PR which adds TDD tests and implementation plans for 10 API enhancements with Swift macros and DSL patterns. Overall, this is an excellent foundation for significantly improving the SwiftAgents API. The code demonstrates strong understanding of Swift 6.2 concurrency, protocol-oriented design, and modern Swift best practices. SummaryScope: 37 files changed (+12,201/-45 lines)
Verdict: ✅ Approve with Recommendations This PR successfully implements the TDD approach with well-designed tests before implementation. The code quality is high, but there are some areas for improvement before merging. Strengths1. Excellent TDD Approach ⭐
2. Strong Swift 6.2 Compliance ⭐
3. Well-Designed Macros ⭐
4. Clean DSL Design ⭐
5. Comprehensive Documentation
Critical Issues 🔴1. Platform Version Mismatch (Package.swift:7-12)Issue: The package requires unreleased OS versions: platforms: [
.macOS(.v26), // ❌ macOS 26 doesn't exist
.iOS(.v26), // ❌ iOS 26 doesn't exist
.watchOS(.v26), // ❌ etc.
.tvOS(.v26),
.visionOS(.v26)
]Current latest versions:
Impact: The package cannot be built on any current Xcode version. This is a blocking issue. Fix Required: platforms: [
.macOS(.v14), // Sonoma (or .v15 for Sequoia)
.iOS(.v17), // Or .v18 for latest
.watchOS(.v10), // Or .v11 for latest
.tvOS(.v17), // Or .v18 for latest
.visionOS(.v1) // Or .v2 for latest
]Also affects: README.md:4, CLAUDE.md mentions "iOS 17+, macOS 14+"—these are correct, but Package.swift doesn't match. 2. Missing Tool Protocol DefinitionThe Action: Confirm that High Priority Issues 🟡3. Macro Wrapper Method Logic Issue (ToolMacro.swift:360-368)The generated private func _userExecute(...) async throws -> \(userExecuteReturnType) {
try await execute() // ❌ Doesn't pass parameters!
}Problem: The wrapper accepts parameters but doesn't pass them to Expected behavior: The user's
Recommendation: Simplify to directly call the user's execute(): let result = try await execute() // Properties already available4. SendableValue Codable Encoding (SendableValue.swift:197+)The implementation starts handling encoding (line 197+), but the logic is truncated in the review. Need to verify:
Action: Review the complete implementation of 5. ResilientAgent Initialization Pattern (ResilientAgent.swift:83-104)The nonisolated init(wrapping resilient: ResilientAgent, ...) {
let baseAgent = resilient.base // ❌ Accessing actor-isolated property from nonisolated context
self.base = baseAgent
...
}Problem: Fix: Either:
6. Duration.timeInterval Extension MissingResilientAgent.swift:133 uses resetTimeout: resetTimeout.timeIntervalBut Fix: Add an extension: extension Duration {
var timeInterval: TimeInterval {
let (seconds, attoseconds) = self.components
return TimeInterval(seconds) + TimeInterval(attoseconds) / 1e18
}
}Medium Priority Issues 🟢7. Test Framework Inconsistency
Recommendation: Standardize on Swift Testing for all new tests (it's the modern approach). 8. Missing Import StatementsSeveral files reference types without importing them:
Action: Ensure all necessary imports are present, especially 9. Macro Diagnostic MessagesThe macro error messages could be more helpful: case .missingDescription:
return "@Tool requires a description string argument"Enhancement: Include fix-it suggestions: return "@Tool requires a description string argument. Usage: @Tool(\"Description here\")"10. Pipeline Error Handling (Pipeline.swift:144-150)The Recommendation: Add logging or a diagnostic handler: public func catchError(
_ fallback: Output,
onError: (@Sendable (Error) -> Void)? = nil
) -> Pipeline<Input, Output>Low Priority Suggestions 💡11. Documentation EnhancementsWhile documentation is good, consider adding:
12. Test Coverage GapsConsider adding tests for:
13. API Naming ConsistencyMinor inconsistencies:
Suggestion: Prefer shorter, clearer names in DSL contexts. 14. Operator Overloading ConcernsThe README mentions operators like
Recommendation: Provide named alternatives for all operators. Code Quality Metrics
RecommendationsBefore Merging (Required)
Nice to Have
Testing Notes
Architecture AssessmentThe overall architecture is excellent: ✅ Protocol-first design - Clean separation of concerns This PR represents a significant improvement to the SwiftAgents API, making it more ergonomic and reducing boilerplate substantially. Final Verdict✅ Approve with Changes Required This is high-quality work that will significantly improve the developer experience with SwiftAgents. The TDD approach is commendable, and the implementation demonstrates strong Swift expertise. Must fix before merge: Critical Issues #1, #2, #5, #6 Once the critical issues are addressed, this will be ready to merge! Next Steps
|
|
iOS, Mac OS 26 was released after your cutoff date (September 2025). You can use web search to confirm this Fix the High Priority issues add bash approval so you can run tests |
|
Claude finished @christopherkarani's task in 4m 17s —— View job High Priority Issues Fixed ✅Completed Tasks:
Summary of FixesAll High Priority issues from the previous code review have been addressed: Issue #3: ToolMacro Wrapper Method ✅Problem: The generated wrapper method extracted parameters but didn't use them correctly - it passed them to Solution:
File: Issue #4: SendableValue Codable Encoding ✅Problem: Review mentioned the implementation might be truncated/incomplete. Solution: No fix needed - reviewed the complete implementation and found:
File: Issue #5: ResilientAgent Initialization Pattern ✅Problem: The Solution: Made all resilience configuration properties
These properties are immutable after initialization ( File: Issue #6: Duration.timeInterval Extension ✅Problem: Solution: Added extension to convert extension Duration {
public var timeInterval: TimeInterval {
let (seconds, attoseconds) = self.components
return TimeInterval(seconds) + TimeInterval(attoseconds) / 1e18
}
}File: Next StepsThe fixes have been committed and pushed to the branch. To verify the fixes work correctly:
The changes should resolve all compilation errors related to these High Priority issues. The macro will now correctly assign tool parameters, the actor isolation is fixed, and the Duration extension provides the missing functionality. Changed Files
Commit: |
…, Duration extension - Fix #3: Simplified ToolMacro execute() wrapper to assign extracted parameters to struct properties and call user's execute() directly, made method mutating - Fix #5: Made ResilientAgent resilience config properties nonisolated to fix actor isolation in chaining initializer - Fix #6: Added Duration.timeInterval extension for TimeInterval conversion - #4: SendableValue encoding verified complete - no changes needed Co-authored-by: Christopher Karani <christopherkarani@users.noreply.github.com>
Comprehensive Pull Request ReviewThis PR introduces significant enhancements to SwiftAgents through macros, DSL patterns, and type-safe APIs. The implementation is impressive in scope and generally high quality. Here's my detailed review: ✅ Strengths1. Excellent API Design
2. Strong Swift 6.2 Concurrency Compliance
3. Comprehensive Test Coverage
4. Well-Documented Code
🔴 Critical Issues1. ToolMacro: Incorrect execute() Wrapper (Fixed in last commit)File: ✅ Status: FIXED in commit c91926e The last commit correctly fixed this by:
Original issue was that the macro was creating a 2. Potential Race Condition in ResilientAgent InitializationFile: ✅ Status: FIXED in commit c91926e The
|
|
@claude conduct a comprehensive code audit of this pr and review the code to spot areas of improvement, bugs, swift 6.2 concurrency and api design improvements |
|
Claude finished @christopherkarani's task in 2m 39s —— View job Comprehensive Code Audit - SwiftAgents API Enhancements PRI've completed a thorough code audit of this PR, examining 38 changed files across macros, DSL patterns, resilience systems, and orchestration capabilities. Here are my findings: 🎯 Executive SummaryOverall Verdict: High Quality with Minor Issues ⭐⭐⭐⭐½ (4.5/5) This PR represents excellent work that significantly enhances the SwiftAgents framework with modern Swift patterns. The implementation demonstrates strong understanding of Swift 6.2 concurrency, protocol-oriented design, and API ergonomics. Previous reviews already addressed critical issues, leaving only minor improvements needed. Key Strengths:
Recommended Action: Approve after addressing the 3 minor bugs identified below. 🐛 Bugs Found (3 issues)1. Race Condition in AgentEventStream.merge() 🔴 Medium PriorityFile: Issue: The merge function silently swallows errors from individual streams: for stream in streams {
group.addTask {
do {
for try await event in stream {
continuation.yield(event)
}
} catch {
// Individual stream errors are ignored in merge ❌
}
}
}Problem: If one stream fails, the error is completely lost. This makes debugging multi-agent orchestration very difficult. Impact: Users won't know when parallel agents fail unless they're monitoring individual streams. Recommendation: Add error handling strategy parameter: public static func merge(
_ streams: AsyncThrowingStream<AgentEvent, Error>...,
errorStrategy: MergeErrorStrategy = .continueAndCollect
) -> AsyncThrowingStream<AgentEvent, Error>
public enum MergeErrorStrategy {
case failFast // Fail on first error
case continueAndCollect // Collect errors in metadata
case ignoreErrors // Current behavior
}2. Potential Memory Leak in Pipeline Composition 🟡 Low PriorityFile: Issue: Long pipeline chains capture closures that may retain large contexts: public func map<NewOutput: Sendable>(
_ transform: @escaping @Sendable (Output) async throws -> NewOutput
) -> Pipeline<Input, NewOutput> {
Pipeline<Input, NewOutput> { input in
let output = try await self.execute(input) // Captures self
return try await transform(output) // Captures transform closure
}
}Problem: Each Impact: Memory usage scales with chain length. Not a leak per se, but could be optimized. Recommendation: Document the behavior and consider providing a 3. SendableValue Encoding Fallback Loses Type Information 🟡 Low PriorityFile: Issue: When encoding complex tool return types fails, the macro falls back to string description: default:
return """
do {
return try SendableValue(encoding: result)
} catch {
return .string(String(describing: result)) ❌ Loses type info
}
"""Problem: Users won't know that their custom return type failed to encode. Silent failures make debugging tool issues very difficult. Impact: Tools returning complex types may appear to work but actually lose data. Recommendation: Either:
✅ Swift 6.2 Concurrency Compliance (Excellent)Actor Isolation: Perfect ✅
Sendable Conformance: Perfect ✅All public types properly conform to
Structured Concurrency: Excellent ✅
Specific Examples of Great Concurrency Design:1. ResilientAgent timeout implementation (lines 239-257): private func executeWithTimeout(_ input: String, timeout: Duration) async throws -> AgentResult {
try await withThrowingTaskGroup(of: AgentResult.self) { group in
group.addTask {
try await self.executeWithResilience(input)
}
group.addTask {
try await Task.sleep(for: timeout)
throw AgentError.timeout(duration: timeout)
}
guard let result = try await group.next() else {
throw AgentError.timeout(duration: timeout)
}
group.cancelAll() // ✅ Properly cancels losing task
return result
}
}Perfect race-to-completion pattern. 2. CircuitBreaker state management (lines 32-243): public actor CircuitBreaker {
private var state: State = .closed // ✅ Actor-isolated
private var consecutiveFailures = 0
public func execute<T: Sendable>(
_ operation: @Sendable () async throws -> T
) async throws -> T {
// ✅ All state access is properly isolated
}
}Textbook actor usage. 🎨 API Design Analysis (Excellent)Fluent API Patterns: Excellent ✅ResilientAgent chaining (lines 107-169): let resilient = myAgent
.withRetry(.exponentialBackoff(maxAttempts: 3))
.withCircuitBreaker(threshold: 5, resetTimeout: .seconds(60))
.withFallback(fallbackAgent)
.withTimeout(.seconds(30))Perfect progressive disclosure - simple defaults, advanced configuration available. Operator Overloading: Good but Needs Documentation
|
| Category | Rating | Notes |
|---|---|---|
| Concurrency Safety | ⭐⭐⭐⭐⭐ | Perfect actor isolation, Sendable conformance |
| API Design | ⭐⭐⭐⭐⭐ | Excellent fluent APIs, type safety, progressive disclosure |
| Error Handling | ⭐⭐⭐⭐ | Good error messages, minor issue with merge() |
| Documentation | ⭐⭐⭐⭐ | Comprehensive DocC comments, examples in code |
| Test Coverage | ⭐⭐⭐⭐ | Very good, missing some edge cases |
| Performance | ⭐⭐⭐⭐ | Efficient, minor memory concern with long pipelines |
| Maintainability | ⭐⭐⭐⭐⭐ | Clean code, well-organized, follows conventions |
🎯 Specific Swift 6.2 Compliance Checks
✅ Data-Race Safety Verification
1. All public types are Sendable:
Pipeline: ✅ Generic constraints require SendableSendableValue: ✅ Enum with all Sendable casesContextKey: ✅ Generic constraintValue: SendableRetryPolicy: ✅ Struct with@SendableclosuresBackoffStrategy: ✅ Enum with Sendable associated values
2. Actor isolation is correct:
ResilientAgent: ✅ Properties properly markednonisolated(lines 41-45)ParallelComposition: ✅nonisolated letfor immutable config (lines 122-124)CircuitBreaker: ✅ All mutable state is actor-isolated
3. No unsafe patterns:
- No
@unchecked Sendablefound ✅ - No
nonisolated(unsafe)found ✅ - No raw
Task {}without proper error handling ✅
💡 Minor Suggestions (Non-Blocking)
1. Duration Extension Could Be More Robust
File: Extensions.swift:533-538
Current implementation:
extension Duration {
var timeInterval: TimeInterval {
let (seconds, attoseconds) = components
return Double(seconds) + Double(attoseconds) / 1e18
}
}Suggestion: Handle overflow for very large durations:
extension Duration {
var timeInterval: TimeInterval {
let (seconds, attoseconds) = components
guard seconds <= Int64(Double.greatestFiniteMagnitude) else {
return .infinity
}
return Double(seconds) + Double(attoseconds) / 1e18
}
}2. AgentMacro Doesn't Detect @tools Attribute
File: AgentMacro.swift:57-61
The macro generates empty tools array if not present:
if !existingMembers.contains("tools") {
members.append("""
public let tools: [any Tool] = []
""")
}Issue: If user annotates a property with @Tools, the macro doesn't detect or use it.
Suggestion: Check for @Tools attribute on properties and skip generation if found.
3. Pipeline Error Context Could Be Better
File: Pipeline.swift:144-152
public func catchError(_ fallback: Output) -> Pipeline<Input, Output> {
Pipeline { input in
do {
return try await self.execute(input)
} catch {
return fallback // ❌ Error is lost
}
}
}Suggestion: Add optional error logger:
public func catchError(
_ fallback: Output,
onError: (@Sendable (Error) -> Void)? = nil
) -> Pipeline<Input, Output>📋 Checklist for Merge Readiness
Critical (Must Fix):
- Fix AgentEventStream.merge() error handling (Bug Phase1 #1)
- Document SendableValue encoding fallback (Bug Improve Swift CI workflow with proper structure and caching #3)
- Verify build passes with
swift build - Verify tests pass with
swift test
Recommended (Should Fix):
- Document memory characteristics of long pipelines (Bug Add TDD tests and implementation plan for API enhancements #2)
- Add integration tests for concurrent scenarios
- Consider Duration overflow handling
Nice to Have:
- Pipeline error logging enhancement
- @tools attribute detection in AgentMacro
- Performance benchmarks for resilience patterns
🏆 Final Recommendations
Immediate Actions:
- Fix the merge() error handling - this is the only medium-priority bug
- Add documentation for the SendableValue encoding fallback behavior
- Run build and tests to ensure everything compiles
Before Next Release:
- Add integration tests with real concurrent loads
- Document performance characteristics (pipeline memory, circuit breaker throughput)
- Consider adding observability hooks for production debugging
Long Term:
- Consider adding Circuit Breaker dashboard/metrics export
- Explore Observable framework integration for reactive state
- Performance benchmarks against LangChain equivalents
🎉 Conclusion
This PR is production-ready after fixing the merge() error handling. The implementation demonstrates exceptional understanding of Swift 6.2 concurrency, modern API design, and software engineering best practices.
Code Quality: ⭐⭐⭐⭐½ (4.5/5)
Recommended Verdict: Approve with Minor Changes
The framework will be significantly more ergonomic and competitive with LangChain after this PR merges. Excellent work!
Code audit completed on 2025-12-14 by Claude Sonnet 4.5
| Branch: claude/improve-swift-api-generics-7KNhz
|
@claude fix the recommended issues |
|
Claude finished @christopherkarani's task in 3m 26s —— View job Fixed Recommended Issues ✅I've successfully addressed all the issues identified in the comprehensive code audit. Completed Tasks:
Summary of Changes1. AgentEventStream.merge() - Error Handling 🔴 High Priority Bug FixedProblem: Errors from individual streams were silently swallowed, making multi-agent debugging impossible. Solution: Added
// Now users can choose how to handle errors
let merged = AgentEventStream.merge(stream1, stream2, stream3, errorStrategy: .continueAndCollect)
for try await event in merged {
if case .failed(let error) = event {
// Errors are now visible!
}
}File: 2. ToolMacro - SendableValue Encoding Documentation 🟡 Medium Priority FixedProblem: Complex return types that failed to encode fell back to Solution: Added comprehensive documentation to the
File: 3. Pipeline - Memory Characteristics Documentation 🟡 Low Priority FixedProblem: Long pipeline chains create nested closures that could accumulate memory, but this wasn't documented. Solution: Added detailed documentation explaining:
File: 4. Duration.timeInterval - Overflow Handling 💡 Minor EnhancementProblem: Very large durations (e.g., Solution: Added guard to return let veryLong: Duration = .seconds(Int64.max)
let interval = veryLong.timeInterval // .infinity (safe)File: 5. Pipeline.catchError - Error Logging 💡 Minor EnhancementProblem: Errors were swallowed without any diagnostic capability. Solution: Added optional let safe = pipeline.catchError("default") { error in
print("Pipeline failed: \(error)")
// Log to analytics, monitoring, etc.
}Maintains backward compatibility with default File: Build & Test Verification Needed
|
This commit fixes the issues identified in the comprehensive code audit: Bug Fixes: 1. AgentEventStream.merge() - Added MergeErrorStrategy enum with three options: - failFast: Fail immediately on first error - continueAndCollect: Convert errors to .failed events (default) - ignoreErrors: Legacy behavior, silently ignore errors This prevents silent error swallowing in multi-agent orchestration. 2. ToolMacro - Added comprehensive documentation about SendableValue encoding: - Documents the String(describing:) fallback behavior - Warns about type information loss for complex types - Recommends using Codable for custom return types - Notes potential security concerns with sensitive data 3. Pipeline - Added detailed memory characteristics documentation: - Explains O(n) memory scaling with chain length - Provides performance guidance for short/medium/long chains - Includes best practices and examples Improvements: 4. Duration.timeInterval - Added overflow handling for very large durations - Returns .infinity instead of overflowing for durations > Double.max - Prevents undefined behavior with extreme timeout values 5. Pipeline.catchError - Added optional onError callback parameter - Allows logging/monitoring before returning fallback - Maintains backward compatibility with default nil All changes maintain backward compatibility while improving error handling, documentation, and edge case handling. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Christopher Karani <christopherkarani@users.noreply.github.com>
Code Review: TDD Tests and API Enhancement ImplementationSummary
Critical Issues (Must Fix)1. Race Condition in ParallelComposition (AgentOperators.swift:111)Actor isolation violation with isCancelled property access. The property is accessed without proper actor isolation creating a potential data race in Swift 6.2 strict concurrency. 2. Actor Isolation Violation in ResilientAgent (ResilientAgent.swift:25-104)Accessing actor's isolated property from nonisolated context. The initializer accesses resilient.base from a nonisolated init which violates actor isolation rules. 3. Force Casts in SendableValue (SendableValue.swift:256-280)Multiple force casts (as!) in decode() method. While type-checked, these violate the framework standard of avoiding force unwraps. Use safe casts (as?) instead. 4. Platform Version Mismatch (Package.swift:8-12)Using .macOS(.v26) and .iOS(.v26) which don't exist yet. Per CLAUDE.md, should be .macOS(.v14) and .iOS(.v17). 5. Code Duplication (ToolArrayBuilder)ToolArrayBuilder is defined in both ToolParameterBuilder.swift:228-291 and AgentBuilder.swift:134-193. Consolidate to single definition. Important Warnings6. Silent Fallback in @tool Macro (ToolMacro.swift:469-477)Macro silently falls back to String(describing:) losing type safety. Add #warning for non-Codable return types. 7. Empty Instructions Allowed (AgentBuilder.swift:306-318)Builder allows empty instructions which may cause undefined behavior. Add validation. 8. Silent Error Suppression (ContextKey.swift:142-150)Encoding errors are silently swallowed and replaced with String representations. At minimum, log these errors. 9. Error Handling Inconsistency (AgentOperators.swift:214-221).collectErrors strategy doesn't actually expose errors in result metadata as documented. 10. Force Unwrap in Error Path (AgentOperators.swift:221)Force unwrap errors.first! when errors exist. Use proper error aggregation. 11. Missing Decodable Constraint (TypedTool.swift:37)Output is only Encodable but SendableValue.decode() requires Decodable. Consider making it Codable. What Looks Excellent
Required Actions Before MergeMust Fix (Blocking):
Should Fix (Important):
Verification Commandsswift build # Build with strict concurrency
swift test --parallel # Run full test suite
swift package plugin --allow-writing-to-package-directory swiftformat --lint .Overall AssessmentThis is a high-quality PR that demonstrates excellent Swift 6.2 knowledge and modern API design. The critical issues are straightforward to fix and primarily related to strict concurrency compliance. Recommendation: Request Changes for critical issues (#1-5, #10), then approve after fixes. Great work on the comprehensive TDD approach and thoughtful API design! |
This commit addresses 7 out of 15 issues identified in the code review: Critical Issues Fixed: - Issue #2: ModelSettings.merged() now validates and throws on invalid merged settings - Issue #3: MCPClient.getAllTools() fixed race condition via task deduplication pattern - Issue #8: MCPResponse init changed from precondition to throwing error for safety High-Priority Issues Fixed: - Issue #4: MCPClient.closeAll() now uses structured concurrency for parallel execution - Issue #10: ResponseTracker now enforces session limits with LRU eviction (default: 1000 sessions) Changes: - ModelSettings.merged() signature changed to throwing (BREAKING CHANGE for callers) - MCPResponse() init now throws instead of using precondition - ResponseTracker adds maxSessions parameter with LRU session eviction - MCPClient uses task deduplication to prevent duplicate cache refreshes - MCPClient.closeAll() executes server closures concurrently Remaining issues (#5, #6, #7, #9, #11-#15) deferred as they require more significant API design changes or are lower priority optimizations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Christopher Karani <christopherkarani@users.noreply.github.com>
…ds trap - Issue #1: MessagesReducer now strips op from retained messages instead of silently dropping messages with non-nil op. This preserves messages that had transient operations during reduction. - Issue #2: secondsToNanoseconds now guards against negative TimeInterval values, returning 0 instead of trapping. Also uses clamping for overflow. - Also fixes a force unwrap in retry exhaustion path (throw lastError!).
2 participants
This commit introduces comprehensive test coverage for 10 planned API
improvements leveraging generics and DSL patterns:
Tests follow TDD approach - they specify expected behavior before
implementation. See IMPLEMENTATION_PLAN.md for detailed roadmap.