Validate MCP response metadata and tighten runtime identity

[christopherkarani]

Summary

• enforce JSON-RPC response invariants (version 2.0 and non-empty ids) plus preconditions
• centralize handoff lookup and allow runtimes to surface explicit identities
• harden tool call parsing, empty arguments handling, and add new duration + registry tests
  Testing
• Not run (not requested)

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[christopherkarani]

@claude review

[claude]

Code Review

PR: Validate MCP response metadata and tighten runtime identity
Files changed: 13 (+313 / -43)

---

Build / Test / Lint Results

⚠️ Environment Note: The build, tests, and linter all fail on this Linux CI runner due to pre-existing environment incompatibilities — not caused by this PR:

• swift build / swift test: Wax dependency unconditionally imports CryptoKit (Apple-only); swift-tiktoken uses URLSession.shared without Linux guards.
• swiftlint lint: libsourcekitdInProc.so is absent on this runner.

These failures exist on main as well. All code analysis below is based on a thorough manual review.

---

Overall Assessment

This is a solid hardening PR with clear intent. The three main changes — MCP response validation, centralized handoff lookup, and tightened tool-call parsing — are all improvements. The new tests cover the happy and sad paths for the new behavior. I have a few issues to flag, ranging from a behavioral inconsistency worth fixing to minor observations.

---

Issues

1. MCPRequest.init inconsistency with empty id — should fix

Sources/Swarm/MCP/MCPProtocol.swift lines 86-96

The memberwise initializer silently replaces an empty id with a generated UUID:

self.id = id.isEmpty ? UUID().uuidString : id   // silent fallback

But init(from:) throws on an empty id:

guard !decodedId.isEmpty else {
    throw DecodingError.dataCorrupted(...)       // hard error
}

The doc comment says "Precondition: id must be non-empty" — but then silently handles it. These two paths should be consistent. The silent-fallback behavior is also tested in MCPProtocolTests under the name requestSanitizesEmptyId, suggesting it is intentional, but it contradicts the stated precondition.

Recommendation: If an empty id is a programmer error, apply precondition(!id.isEmpty) (matching the method treatment). If silent generation is desired, remove the doc-comment precondition and align the decoder to also substitute a UUID rather than throwing.

---

2. MCPResponse decoder rejects result: null — worth discussing

Sources/Swarm/MCP/MCPProtocol.swift lines 266-277

The mutual-exclusivity guard throws when both result and error decode to nil. Per JSON-RPC 2.0 spec, result must be present on success but may explicitly be null. The current decoder treats a wire payload of {"jsonrpc":"2.0","id":"x","result":null} as invalid (both nil after decodeIfPresent). If this is intentional (i.e., the framework treats null result as an error), it should be documented; if it's an oversight, the decoder should check for the presence of the key rather than the decoded value.

---

3. SendableValue wire-format limitation — document or track

Tests/SwarmTests/MCP/MCPProtocolTests.swift lines 53-57

The test comment already notes this, but it's worth calling out in the review: SendableValue uses Swift's synthesized discriminated-union Codable, so a round-tripped response looks like {"string": {"_0": "value"}} rather than the raw "value" that an actual MCP server would send. This makes the current MCPRequest/MCPResponse types unsuitable for real MCP wire protocol without a custom Codable implementation for SendableValue. Consider tracking this as a known limitation (e.g., in a GitHub issue or a // TODO: comment near the type) so it isn't forgotten.

---

4. Missing unit tests for two new types — minor

• HandoffConfiguration+Lookup.swift (handoffConfiguration(for:)): No dedicated unit test. The method is exercised indirectly through SwarmRunner tests, but a direct test (match found, match not found, empty sequence) would make the contract explicit and protect against regression.
• AgentRuntimeIdentifiable protocol (AgentRuntime+Identity.swift): No test coverage for the .custom storage path (class/actor runtimes use .reference, value types use .value, but the new AgentRuntimeIdentifiable path .custom is untested).

---

5. Value-type identity fingerprint is fragile — minor / FYI

Sources/Swarm/Core/AgentRuntime+Identity.swift line 33

fingerprint: String(reflecting: runtime)

String(reflecting:) produces the debug description, which can include memory addresses or internal state that changes between runs. Two structurally identical value-type runtimes would compare as equal (possible false positive), and the same instance could produce different strings if it contains any reference-type properties. This is likely fine for current usage (the call site is areSameRuntime, not a stable cache key), but worth a comment explaining the intent and known limitations.

---

Positives

• completedCalls() hardening is well-considered: the hasPayload check before throwing avoids false positives for empty accumulator entries, and the new SwarmError.invalidToolArguments gives callers actionable context.
• Empty-arguments early return in parseToolArguments correctly handles both truly-empty strings and whitespace-only strings before the UTF-8 conversion.
• context parameter fix in handleToolCalls (was nil, now passes swarmContext) is a real bug fix ensuring tools can read execution context.
• Handoff lookup centralization cleanly removes ~27 lines of duplicated private methods across four files. The Sequence extension is the right home for this.
• precondition in factory methods (MCPResponse.success, MCPResponse.failure) with assert in the private initializer is a clean layered-defense pattern.
• Test infrastructure (ScriptedSwarmToolCallProvider, SwarmRunnerToolCallAgent) is well-structured and reusable.

---

Summary

Severity	Item
Should fix	MCPRequest.init empty-id behavior inconsistency (#1)
Worth discussing	MCPResponse decoder + result: null (#2)
Track/document	SendableValue wire-format limitation (#3)
Minor	Missing tests for HandoffConfiguration+Lookup and .custom identity path (#4)
FYI	Value-type fingerprint fragility note (#5)

Happy to discuss any of these. The core hardening intent is sound — mainly the empty-id inconsistency in MCPRequest is worth a follow-up before merging.

[claude]

Claude finished @christopherkarani's task in 4m 13s —— View job

---

PR #44 Code Review

Tasks

• Gather context
• Read changed files
• Analyze code quality, correctness, security
• Post review findings

---

Summary

This PR makes three independent improvements: (1) JSON-RPC response invariant enforcement in MCPProtocol, (2) centralization of handoff lookup via a new Sequence extension, and (3) hardened tool-call parsing in SwarmRunner. The core direction is sound. I have no blocking issues, but I found a few things worth addressing before merge.

---

Issues

Medium — HandoffConfiguration+Lookup.swift: extension is public but should be internal

// Sources/Swarm/Orchestration/HandoffConfiguration+Lookup.swift
public extension Sequence where Element == AnyHandoffConfiguration {
    func handoffConfiguration(for targetAgent: any AgentRuntime) -> AnyHandoffConfiguration? {

This adds handoffConfiguration(for:) to every [AnyHandoffConfiguration] in the framework's public API surface. External library consumers can call this on their own arrays. Since it's a framework-internal routing helper (not intended for external use), it should be internal. The OrchestrationBuilder and OrchestratorProtocol extensions that call it are already available as the external-facing API.

Fix this →

---

Medium — MCPResponse.success/.failure: docstrings are stale after adding precondition

// Sources/Swarm/MCP/MCPProtocol.swift
/// - Note: This method cannot fail as it guarantees valid inputs.
static func success(id: String, result: SendableValue) -> MCPResponse {
    precondition(!id.isEmpty, "MCPResponse.success requires a non-empty id")

The doc comment says "cannot fail" but an empty id now triggers a precondition crash. These are not the same thing — precondition terminates the process in both debug and release builds. The note should be updated to something like: "Precondition: id must be non-empty. Passing an empty string is a programming error and will crash in all build configurations."

Fix this →

---

Medium — SwarmRunner.completedCalls(): behavioral breaking change not documented

// Sources/Swarm/Orchestration/SwarmRunner.swift
let hasPayload = call.id != nil || call.name != nil || !call.arguments.isEmpty
guard let id = call.id, let name = call.name else {
    if hasPayload {
        throw SwarmError.invalidToolArguments(...)
    }
    continue
}

Previously, incomplete tool call deltas were silently dropped via compactMap. Now they throw SwarmError.invalidToolArguments. This is arguably correct behavior, but it's a breaking change for any caller that was relying on the silent-skip behavior. The change log and/or an inline comment should note this semantic shift, and it warrants a consideration for whether the hasPayload check is sufficient to distinguish corrupted-but-partial deltas from legitimately empty ones (e.g., a streaming chunk that arrives with only an index increment and no other fields).

---

Minor — AgentRuntimeIdentifiable is public but AgentRuntimeIdentity is internal

// Sources/Swarm/Core/AgentRuntime+Identity.swift
public protocol AgentRuntimeIdentifiable: Sendable {
    var runtimeIdentity: String { get }
}

struct AgentRuntimeIdentity: Hashable, Sendable {  // internal

The public protocol allows external types to conform and supply a runtimeIdentity, but external conformers have no visibility into how that identity is consumed (the AgentRuntimeIdentity struct is internal). This is probably intentional — external conformers just supply the string, the framework decides what to do with it. If so, a doc comment on AgentRuntimeIdentifiable clarifying the intent would help users understand why they'd conform.

---

Minor — MCPResponse init validates jsonrpc and id but private init does not

The throwing init(jsonrpc:id:result:error:) and init(from decoder:) both guard against non-"2.0" jsonrpc and empty id. However, the private non-throwing init(id:result:error:) (used by success/failure) hard-codes jsonrpc = "2.0" and relies on the preconditions in the callers. This is consistent but means the invariants are enforced in two different places with two different mechanisms. Consider adding a precondition(!id.isEmpty) directly to the private init for defence-in-depth.

---

What's Good

• Correct DRY consolidation: Removing the four identical private findHandoffConfiguration methods is correct. All affected actors (AgentRouter, SequentialChain, SupervisorAgent) conform to OrchestratorProtocol, so their call sites naturally resolve to the protocol extension. No compilation issues.
• areSameRuntime is accessible: The free function used in the new extension is internal-scoped but accessible within the module. No visibility issue.
• Context propagation fix: Changing context: nil → context: context in SwarmRunner at line 455 is a legitimate bug fix — tool calls were losing the active agent context.
• Empty argument handling: The trimmed.isEmpty → return [:] short-circuit in parseToolArguments correctly handles LLMs that emit tool calls with no arguments, without crashing or throwing spuriously.
• Test coverage: All new behaviors have tests. DurationExtensionsTests tests edge cases including overflow, MCPProtocolTests covers both decode-time invariants, and SwarmRunnerTests covers both the happy path (empty args accepted) and the error path (incomplete call throws).
• @unchecked Sendable in test mock: ScriptedSwarmToolCallProvider uses NSLock correctly to protect mutable state — appropriate for test infrastructure.

---

Not Reviewed

• SwarmToolRegistryTests.swift imports HiveCore — this file was not visible in the branch diff, so I cannot verify MockTool, HiveToolCall, or SwarmToolRegistryError are defined with compatible signatures. Ensure swift build and swift test pass cleanly.

---

[claude]

PR Review: Validate MCP response metadata and tighten runtime identity

Reviewer: Claude Code (automated review)
Build: ❌ Failed (pre-existing Linux/CryptoKit environment issue — not introduced by this PR)
Tests: ❌ Could not run (same root cause)
SwiftLint: ❌ Could not run (libsourcekitdInProc.so unavailable on Linux runner)

---

Summary

This PR delivers three coherent improvements: (1) MCP JSON-RPC 2.0 validation hardening, (2) centralized handoff lookup, and (3) SwarmRunner tool-call parsing robustness. The changes are well-scoped and come with meaningful test coverage. The key bug fix—passing context instead of nil to tool execution—is particularly valuable.

---

✅ Strengths

DRY win — HandoffConfiguration+Lookup.swift
Four private copies of findHandoffConfiguration(for:) are replaced by a single Sequence extension. Clean, correct, and easy to maintain.

Bug fix: context: nil → context: context in SwarmRunner.handleToolCalls
Previously tools never received the session context (AgentContext). This was a silent correctness bug. The fix is minimal and correct.

parseToolArguments whitespace trimming
Trimming before JSON parsing is a sensible defensive measure. Returning [:] for empty/whitespace-only strings correctly handles providers that omit arguments for no-arg tool calls rather than sending {}.

Partial tool-call error surfacing in completedCalls()
The new for loop distinguishes between:

• Completely empty accumulations (no id, no name) → still silently skipped, acceptable for streaming artifacts
• Partial payload (id XOR name) → throws SwarmError.invalidToolArguments, surfacing real protocol violations

This is a meaningful improvement over the previous silent compactMap drop.

New test coverage
DurationExtensionsTests, SwarmToolRegistryTests, and additions to MCPProtocolTests/SwarmRunnerTests are all well-targeted. The ScriptedSwarmToolCallProvider test helper is cleanly scoped.

---

⚠️ Issues & Suggestions

1. MCPResponse.init validation inconsistency with MCPRequest.init [Medium]

MCPRequest.init uses a lenient recovery strategy—it silently substitutes an empty method with "unknown" and an empty id with a fresh UUID. MCPResponse.init now throws MCPError.invalidRequest for the same violations. The asymmetry is not necessarily wrong (responses are usually created internally and requests come from users), but the difference in philosophy should be documented.

Specifically, the jsonrpc parameter still has a default value of "2.0", yet the initializer now throws if any other value is passed. Consider either:

• Removing the jsonrpc parameter entirely (always hardcode "2.0") to make the API impossible to misuse, or
• Documenting the Throws condition clearly in the doc comment.

// Current (misleading default):
public init(jsonrpc: String = "2.0", id: String, ...) throws

// Cleaner alternative:
public init(id: String, ...) throws  // jsonrpc always "2.0"

2. precondition in public factory methods will crash in release builds [Low]

MCPResponse.success(id:result:) and MCPResponse.failure(id:error:) use precondition(!id.isEmpty, ...). Unlike assert, precondition fires in both debug and release builds. These methods are public, so any caller passing an empty string will crash in production.

This is arguably the right choice (empty ID is always a programmer error), but it should be documented explicitly:

/// - Precondition: `id` must be non-empty. Violating this precondition
///   triggers a runtime crash in both debug and release builds.
static func success(id: String, result: SendableValue) -> MCPResponse

3. AgentRuntimeIdentifiable expands the public API surface [Low]

The new public protocol AgentRuntimeIdentifiable: Sendable is a stable API addition. Confirm this is intentional and covered by the versioning strategy. Once public, removing or changing runtimeIdentity: String is a breaking change.

4. No test for the context bug fix [Low]

The context: nil → context: context fix in handleToolCalls is the most impactful correctness fix in this PR, but there is no test verifying that tools actually receive context values. A test asserting that a tool call can read a context value would provide a regression safety net.

5. MCPResponse.init(from:) still uses direct property assignment after validation [Nit]

The decoder init validates the JSON-RPC invariants and then assigns properties directly, bypassing the throwing memberwise init. This is intentional (the comment says so) and correct, but it means the two validation paths (memberwise init vs. decoder) are now partially duplicated. Consider extracting a private non-throwing init that both paths call after validation, to ensure invariants can't diverge.

---

🔧 Build Note

The swift build failure is caused by pre-existing Linux CI environment limitations:

• WaxCore imports CryptoKit (Apple-platform only)
• SwiftTiktoken uses URLSession.shared (not available on Linux)

Neither of these is introduced by this PR. The framework is designed for Apple platforms (iOS 17+, macOS 14+) per CLAUDE.md.

---

Verdict

Approve with minor suggestions. The changes are sound, the motivation is clear, and the new tests are appropriate. The two items worth addressing before merge are:

1. Documenting or removing the misleading jsonrpc default parameter in MCPResponse.init
2. Documenting the precondition crash behavior in the public factory methods

The context bug fix alone makes this PR worth merging.

[christopherkarani]

@claude fix issues and suggestions, then do one final pass review

[claude]

Claude finished @christopherkarani's task in 5m 12s —— View job

---

Fixing Issues & Final Review

Branch: codex/audit-swarm-toolcall-mcp-20260220

Tasks

• Gather context and read relevant files
• Fix: HandoffConfiguration+Lookup.swift — remove public modifier (internal helper)
• Fix: MCPResponse.success/.failure — update stale docstrings to reflect precondition
• Fix: Private MCPResponse init — add defense-in-depth precondition(!id.isEmpty)
• Fix: SwarmRunner.completedCalls() — document behavioral breaking change inline
• Fix: AgentRuntimeIdentifiable — add doc comment clarifying public API intent + fingerprint fragility note
• Add: Test for HandoffConfiguration+Lookup (match found / not found / empty / first-match)
• Add: Test verifying context flows through tool execution
• Commit & push all changes
• Final pass review

---

Changes Made (commit 159a7bd)

6 files changed, +136 / -5

File	Change
HandoffConfiguration+Lookup.swift	Removed public from extension; added doc note that it's an internal routing helper
MCPProtocol.swift	Updated success/failure docstrings (replaced "cannot fail" with precondition crash description); added precondition(!id.isEmpty) to private init for defence-in-depth
SwarmRunner.swift	Added inline comment in completedCalls() explaining the behavioural shift from silent drop to explicit throw
AgentRuntime+Identity.swift	Expanded AgentRuntimeIdentifiable doc comment with conformance guidance and internal-API note; added String(reflecting:) fragility comment at the value fingerprint site
HandoffConfigurationTests.swift	Added HandoffConfigurationLookupTests suite: match found, not found, empty sequence, first-match-wins
SwarmRunnerTests.swift	Added context propagation regression test: initial context survives tool execution and appears in SwarmResponse

---

Final Pass Review

Build / Test

Build and tests cannot be run on this Linux CI runner due to pre-existing environment incompatibilities (WaxCore uses CryptoKit; swift-tiktoken uses URLSession.shared without Linux guards). These failures are on main and unrelated to this PR. All analysis below is manual.

---

What the Fix Commit Resolves

All medium and most low-priority issues from the prior reviews are now addressed:

Issue	Status
HandoffConfiguration+Lookup extension public when it should be internal	✅ Fixed
success/failure docstrings say "cannot fail" despite precondition	✅ Fixed
Private MCPResponse init lacked defence-in-depth precondition	✅ Fixed
completedCalls() behavioural change not documented	✅ Fixed
AgentRuntimeIdentifiable needs clarifying doc comment	✅ Fixed
String(reflecting:) fragility not noted	✅ Fixed
HandoffConfiguration+Lookup has no direct unit tests	✅ Fixed (4 cases)
No regression test for context: nil bug fix	✅ Fixed

---

Remaining Items (accepted trade-offs, not blocking)

MCPRequest.init empty-id inconsistency — the memberwise init now uses precondition(!id.isEmpty) (same as method), so the init and the decoder are now both strict. The old test requestSanitizesEmptyId (which tested the silent UUID fallback) no longer exists in this PR — the PR already upgraded to precondition before this review cycle. No further action needed.

MCPResponse decoder + result: null — decoding {"jsonrpc":"2.0","id":"x","result":null} still throws (both result and error decode to nil, failing the mutual-exclusivity guard). Per JSON-RPC 2.0 spec, a null result is technically valid for success responses. This is a known limitation worth tracking but not blocking for this PR.

SendableValue wire-format limitation — the test comment already documents this. No action needed in this PR.

.custom identity storage path untested — low priority; the path is only exercised when a value-type agent conforms to AgentRuntimeIdentifiable, which currently has no production conformers. Acceptable to defer.

---

Code Quality

The PR is in good shape. With the fix commit:

• All public-API contracts are now accurately documented
• Internal helpers are correctly scoped (internal, not public)
• The precondition invariants are consistent across all three MCPResponse construction paths (public init, factory methods, private init)
• The behavioural change in completedCalls() is discoverable in-code without reading the PR history
• New lookup tests give explicit protection against areSameRuntime regressions
• The context regression test guards the context: nil → context: context fix

Verdict: Ready to merge.
|

[claude]

Code Review: PR #44 — Validate MCP response metadata and tighten runtime identity

Summary

This PR makes four focused changes: (1) adds an AgentRuntimeIdentifiable protocol for value-type agent identity, (2) hardens MCPResponse with JSON-RPC 2.0 version and non-empty ID validation, (3) centralizes the repeated findHandoffConfiguration pattern into a shared extension, and (4) fixes tool call parsing (error surfacing for incomplete calls, empty argument handling, and a context threading bug). Test coverage is added for all new behaviors.

Overall this is solid, well-documented work. A few items worth discussing below.

---

Build / Test / Lint Results

Note: The swift build failure is a pre-existing environment issue — WaxCore (a transitive dependency) imports CryptoKit, which is unavailable on Linux CI. This is unrelated to the changes in this PR. SwiftLint could not run for the same environment reason (SourceKit not available on Linux).

---

Issues and Observations

1. AgentRuntimeIdentifiable priority creates a silent footgun for class/actor types (medium)

The AgentRuntimeIdentity init checks AgentRuntimeIdentifiable conformance before checking for reference types:

if let identifiable = runtime as? any AgentRuntimeIdentifiable {
    storage = .custom(...)
} else if type(of: runtime) is AnyObject.Type {
    storage = .reference(...)
}

This means a class or actor that accidentally (or intentionally) conforms to AgentRuntimeIdentifiable will silently switch from reference identity to string-based identity — a behavioral change that could cause two different instances with the same runtimeIdentity to compare as equal. The docs say "Class- and actor-based runtimes automatically use reference identity and do not need to conform", but there is no compile-time enforcement.

Suggestion: Either document this priority explicitly in AgentRuntimeIdentifiable's doc comment (with a clear warning), or add a runtime assertionFailure when an AnyObject type conforms to AgentRuntimeIdentifiable — since that's always a misuse.

2. precondition on public API — document crash semantics (minor)

MCPResponse.success(id:result:) and MCPResponse.failure(id:error:) now call precondition(!id.isEmpty, ...), which crashes in both debug and release builds. This is documented in the doc comment, which is good. However, it's worth asking whether these public API entry points should throw instead — callers receiving IDs from external sources (e.g., deserializing a request) might prefer to handle an empty ID gracefully rather than crash.

If the intent is "empty ID is always a programming error that should never reach these call sites", precondition is correct. If there's any chance external data could produce an empty ID here, throws is safer.

3. Validation duplication between init(throws:) and init(from decoder:) (minor/intentional)

The JSON-RPC version and ID checks appear in both the designated throwing initializer and the Decodable initializer. This is a defence-in-depth approach but the error types differ (MCPError.invalidRequest vs DecodingError.dataCorrupted). This is reasonable since the two paths serve different callers, but it's worth a comment confirming the duplication is intentional — otherwise a future maintainer might try to deduplicate and break one path.

4. Missing test for AgentRuntimeIdentifiable (minor)

The new AgentRuntimeIdentifiable protocol and the .custom storage case have no dedicated tests. It would be valuable to add:

• Two value-type agents with the same runtimeIdentity → areSameRuntime returns true
• Two value-type agents with different runtimeIdentity → areSameRuntime returns false
• A class-type agent conforming to AgentRuntimeIdentifiable (to document/test current priority behavior)

5. Trailing blank line after removed methods in AgentRouter.swift and SupervisorAgent.swift (nit)

Removing the private findHandoffConfiguration method leaves an extra blank line before the closing } in both files. SwiftFormat would catch this, but it's worth a manual check on the PR branch.

---

Positive Highlights

• HandoffConfiguration+Lookup.swift — Clean DRY refactoring. The Sequence where Element == AnyHandoffConfiguration extension is idiomatic Swift and the centralization eliminates four identical copies of the same lookup logic.

• completedCalls() behavioral change — The transition from silent-drop to explicit error for incomplete tool call deltas is a meaningful correctness improvement, and the inline comment explaining the old behavior is helpful for reviewers.

• Context threading fix (context: nil → context: context in SwarmRunner.swift:460) — The regression test initialContextPreservedThroughToolExecution is a well-scoped guard for this specific bug. Good catch.

• Empty argument trimming — Trimming whitespace before checking for empty arguments is the right approach; some LLM providers emit "{}" or whitespace-padded JSON.

• ScriptedSwarmToolCallProvider — Well-structured test helper. Using NSLock with @unchecked Sendable is the correct pattern when manual synchronization is required.

• DurationExtensionsTests — Tight, focused tests with appropriate epsilon comparison for floating point.

---

Verdict

Request changes on items 1 (potential footgun) and 4 (missing identity tests). Items 2, 3, and 5 are minor and can be addressed at the author's discretion. The core logic is correct and the test coverage for the new behaviors is solid.

---

Reviewed by Claude Code (automated review) — build environment: Linux (CryptoKit/SourceKit unavailable; pre-existing failures unrelated to this PR)

[claude]

PR Review: Validate MCP response metadata and tighten runtime identity

Build status: ❌ swift build fails — pre-existing issue, not introduced by this PR.

error: 'swarm': Source files for target HiveSwarm should be located under 'Sources/HiveSwarm'

The Sources/HiveSwarm directory does not exist on main either; Package.swift declares the target but the directory is missing. This should be addressed separately.

Tests: Could not run — blocked by the build failure above.
SwiftLint: Unavailable in this environment (linker crash loading libsourcekitdInProc.so).

---

Summary

This PR fixes four distinct issues cleanly:

1. MCP JSON-RPC invariant enforcement (version + id)
2. Centralized handoff lookup (DRY refactor)
3. AgentRuntimeIdentifiable protocol for explicit value-type identity
4. Three SwarmRunner bug fixes (context propagation, empty arguments, incomplete tool call detection)

Overall the changes are well-structured and the test coverage is solid. A few observations below.

---

Code Quality

✅ Handoff lookup centralization

Removing the four identical findHandoffConfiguration private methods in favour of a Sequence extension is a clear improvement. The extension site (HandoffConfiguration+Lookup.swift) is appropriately scoped and the file-level comment explains its purpose well.

✅ Context bug fix in SwarmRunner

Passing context: nil to toolEngine.execute was a silent data loss bug. The fix is one-line but impactful, and the accompanying regression test (initialContextPreservedThroughToolExecution) is exactly the right way to guard against regressions.

✅ completedCalls() rewrite

The behavioural shift from silent compactMap drops to explicit error-throwing for incomplete-but-non-empty deltas is the right call for a framework. The inline comment explaining the old vs. new behaviour is appreciated.

⚠️ AgentRuntimeIdentifiable priority ordering

In AgentRuntimeIdentity.init(_:), the AgentRuntimeIdentifiable conformance check runs before the AnyObject check:

if let identifiable = runtime as? any AgentRuntimeIdentifiable {
    storage = .custom(...)
} else if type(of: runtime) is AnyObject.Type {
    storage = .reference(...)
}

This means a class or actor that also conforms to AgentRuntimeIdentifiable will use string-based identity instead of reference identity. Two different instances returning the same runtimeIdentity string would compare as equal, which is almost certainly wrong for a reference type. The doc comment says class/actor runtimes "do not need to conform", but says nothing about what happens when they do. Consider:

• Documenting that class/actor conformance to AgentRuntimeIdentifiable produces undefined/unsupported behaviour, or
• Swapping the priority so AnyObject is checked first and AgentRuntimeIdentifiable only applies to value types.

⚠️ Duplicate precondition in MCPResponse factory methods

success(id:) and failure(id:) each have a precondition(!id.isEmpty, ...), and then the private init(id:result:error:) they delegate to has another precondition(!id.isEmpty, ...). The private-init guard is a reasonable last-resort safety net, but the factory-level checks make the private one unreachable in practice. This is harmless but creates mild confusion. A comment like // Belt-and-suspenders: also guarded at call sites on the private-init precondition would clarify intent.

⚠️ Behavioural change: incomplete tool call deltas now throw

The old behaviour silently dropped calls with a missing id or name. The new behaviour throws SwarmError.invalidToolArguments if any payload is present but either field is absent. This is a breaking behavioural change for callers whose streaming providers sometimes send partial deltas as protocol noise. The test covers the new path well, but consider:

• Is there a way a valid provider emits a non-empty-but-incomplete delta that should still be dropped? (e.g. an index-only delta that arrives before the metadata delta.)
• If the answer is "never happens with compliant providers", the change is fine; just noting the breakage risk.

---

MCP Protocol Validation

✅ JSON-RPC version validation

Checking jsonrpc == "2.0" in both the throwing public init and the Decodable decoder is correct. The version check should happen at decode time so malformed wire data is rejected early.

✅ Empty id rejection

Appropriate guard. The JSON-RPC 2.0 spec disallows empty-string IDs (ids must be string, number, or null — an empty string is technically a string but semantically invalid as a correlation key).

Minor: Validation logic is duplicated between init and init(from:decoder:)

The decoder path does not call self.init(jsonrpc:id:result:error:) — it assigns stored properties directly — so the two validation paths are independent. This is an existing pattern in the file; the PR keeps it consistent. A future refactor could introduce a validate(jsonrpc:id:) helper to deduplicate.

---

Tests

New test	Coverage	Notes
responseRejectsInvalidJSONRPC	Decoder path rejects "1.0"	✅
responseRejectsEmptyId	Decoder path rejects ""	✅
runStreamAcceptsEmptyToolArguments	Empty arguments string → [:]	✅
initialContextPreservedThroughToolExecution	Context regression	✅
runStreamThrowsOnIncompleteToolCall	Incomplete delta throws	✅
HandoffConfigurationLookupTests (4 cases)	Sequence extension	✅
DurationExtensionsTests	timeInterval edge cases	✅
SwarmToolRegistryTests	Registry sort + invoke	✅

Coverage is thorough. The ScriptedSwarmToolCallProvider test helper in SwarmRunnerTests.swift is well-implemented and reusable for future streaming tests.

One nit: SwarmToolRegistryTests imports HiveCore — if Sources/HiveSwarm is never added, these tests will fail to compile. The test target (SwarmTests) does not currently declare a dependency on HiveSwarm/HiveCore, so this may need a Package.swift update or moving the tests to the HiveSwarmTests target.

---

Summary of Action Items

Priority	Item
🔴 Must	Resolve missing Sources/HiveSwarm directory (pre-existing, but blocks CI)
🟡 Should	Document or re-order AgentRuntimeIdentifiable vs. reference-type priority in AgentRuntimeIdentity.init
🟡 Should	Verify SwarmToolRegistryTests compiles under the correct target / dependencies
🟢 Nice to have	Add comment to private-init precondition explaining it is a belt-and-suspenders guard

The core logic changes are sound. Happy to approve once the HiveSwarm directory issue is clarified (or confirmed pre-existing and tracked separately).

---

Review generated by Claude Code

[claude]

PR Review: Validate MCP response metadata and tighten runtime identity

Build Status: ❌ swift build fails — pre-existing issue in swift-tiktoken dependency (URLSession.shared unavailable on Linux). Unrelated to this PR's changes.

Tests: ❌ Cannot run (build blocked by above). Logic reviewed manually below.

---

Summary

This PR tackles three distinct areas: MCP protocol hardening, agent runtime identity, and handoff lookup centralization. The individual changes are well-reasoned, but there's an incomplete refactor and a significant inconsistency introduced in the handoff lookup area.

---

Issues

🔴 Critical: Incomplete centralization creates inconsistent runtime identity matching

The PR centralizes handoff lookup via the new Sequence<AnyHandoffConfiguration>.handoffConfiguration(for:) extension, but only updates two sites (OrchestrationStepContext and OrchestratorProtocol). Three other orchestrators — SequentialChain, AgentRouter, and SupervisorAgent — still have their own private areSameRuntime implementations with different matching logic:

// SequentialChain / AgentRouter / SupervisorAgent (unchanged)
return lhs.name == rhs.name && String(describing: type(of: lhs)) == String(describing: type(of: rhs))

// HandoffConfiguration+Lookup.swift (new, uses AgentRuntimeIdentity)
storage = .value(type: ObjectIdentifier(type(of: runtime)), fingerprint: String(reflecting: runtime))

name + type-string vs String(reflecting:) will give different results for value-type agents with the same name but different stored properties. This inconsistency means handoff resolution behaves differently depending on which orchestrator is used — a subtle correctness bug.

🟡 Dead code: OrchestrationStepContext.areSameRuntime is unreachable

OrchestrationBuilder.swift lines 66–75 contain a private areSameRuntime method that is no longer reachable — findHandoffConfiguration now delegates entirely to the new extension. Should be removed.

🟡 Behavioral change underdocumented in completedCalls()

The change from compactMap (silent drop) to throwing on partial tool calls is correct and the inline comment explains it well. However, this is a breaking change for any code that previously tolerated partial streaming deltas. The PR description mentions this but it would be worth adding a note in CHANGELOG or the method's doc comment so library consumers are not surprised.

🟡 Redundant precondition in MCPResponse.success/.failure

Both factory methods call precondition(!id.isEmpty, ...) and then call the private init which also calls precondition(!id.isEmpty, ...). The outer preconditions are redundant — the private init is the single enforcement point. The outer calls can be removed for clarity.

// Redundant — private init already enforces this
static func success(id: String, result: SendableValue) -> MCPResponse {
    precondition(!id.isEmpty, "MCPResponse.success requires a non-empty id") // ← redundant
    MCPResponse(id: id, result: result, error: nil)
}

🟡 No test for AgentRuntimeIdentifiable

The new public protocol AgentRuntimeIdentifiable is untested. A test should verify that two value-type agent instances with the same runtimeIdentity are considered the same runtime by areSameRuntime(_:_:), and that different identities are not.

🟢 Minor: Misleading comment in init(from decoder:)

// Use private initializer that bypasses the throwing check
// since we've already validated above
self.jsonrpc = jsonrpc   // ← direct assignment, not calling private init

The comment says "Use private initializer" but the code directly assigns self.* properties. The comment should reflect what the code actually does.

---

What's Good

• MCP validation: The jsonrpc == "2.0" and !id.isEmpty guards in both the throwing public init and init(from decoder:) are correct and necessary. The validation is appropriately symmetric.
• Private MCPResponse.init: The clean private initializer with a single precondition is a good design. Moving from try/catch + fatalError to direct precondition in the factory methods removes the misleading "cannot fail" comment and makes intent clearer.
• AgentRuntimeIdentifiable protocol: Well-designed with a clear type + id scoping in .custom storage, preventing cross-type identity collisions. Doc comment is thorough.
• completedCalls() rewrite: The loop-based approach with explicit payload detection is more readable than the old compactMap chain, and the behavior is more correct.
• Empty argument handling: parseToolArguments trimming empty/whitespace arguments to [:] is the right approach for LLMs that emit empty argument strings.
• Test quality: The new tests are well-structured — the context propagation regression test and the scripted provider are particularly useful.

---

Suggested Action

1. Apply the new handoffConfiguration(for:) extension to SequentialChain, AgentRouter, and SupervisorAgent — or explicitly document why those sites intentionally use different matching logic.
2. Remove the dead OrchestrationStepContext.areSameRuntime private method.
3. Add a test for AgentRuntimeIdentifiable round-tripping through areSameRuntime.
4. Fix the misleading comment in MCPResponse.init(from decoder:).

[christopherkarani]

@claude fix all issues

[claude]

Claude finished @christopherkarani's task in 7m 23s —— View job • claude/pr-44-20260225-0619 • Create PR ➔

---

Fixing All Issues from PR #44 Reviews

Branch: claude/pr-44-20260225-0619
Commit: 999bbb6

Tasks

• Gather context and read relevant files
• Reset branch to include PR Validate MCP response metadata and tighten runtime identity #44 merged code
• Fix: AgentRuntimeIdentity.init — check AnyObject before AgentRuntimeIdentifiable to prevent class/actor conformers from bypassing reference identity
• Fix: Remove dead private areSameRuntime from OrchestrationStepContext
• Fix: Remove private findHandoffConfiguration + areSameRuntime from SequentialChain (inconsistent name+type logic)
• Fix: Remove private findHandoffConfiguration + areSameRuntime from AgentRouter (inconsistent name+type logic)
• Fix: Remove private findHandoffConfiguration + areSameRuntime from SupervisorAgent (inconsistent name+type logic)
• Fix: Update misleading comment in MCPResponse.init(from decoder:) ("Use private initializer" → "Assign properties directly")
• Add: Test suite AgentRuntimeIdentifiableTests covering 4 cases
• Commit & push all changes

---

Changes Made

7 files changed, +97 / -68

File	Change
Sources/Swarm/Core/AgentRuntime+Identity.swift	Swapped priority: AnyObject check before AgentRuntimeIdentifiable — reference types now always use pointer identity, even if they conform to AgentRuntimeIdentifiable
Sources/Swarm/Orchestration/AgentRouter.swift	Removed private findHandoffConfiguration and areSameRuntime
Sources/Swarm/Orchestration/SequentialChain.swift	Removed private findHandoffConfiguration and areSameRuntime
Sources/Swarm/Orchestration/SupervisorAgent.swift	Removed private findHandoffConfiguration and areSameRuntime
Sources/Swarm/Orchestration/OrchestrationBuilder.swift	Removed dead private areSameRuntime from OrchestrationStepContext
Sources/Swarm/MCP/MCPProtocol.swift	Fixed misleading comment in MCPResponse.init(from decoder:)
Tests/SwarmTests/Orchestration/HandoffConfigurationTests.swift	Added AgentRuntimeIdentifiableTests suite (same identity → equal, different identity → not equal, reference type priority, lookup integration)

---

Key Design Decisions

Handoff centralization: All three orchestrators (SequentialChain, AgentRouter, SupervisorAgent) now fall through to the OrchestratorProtocol extension's findHandoffConfiguration(for:), which calls handoffs.handoffConfiguration(for:) → module-level areSameRuntime → AgentRuntimeIdentity. The old per-actor implementations used name + type-string matching which was both inconsistent and less accurate than AgentRuntimeIdentity.

AgentRuntimeIdentifiable priority: AnyObject is now checked first. A class/actor that happens to conform to AgentRuntimeIdentifiable will correctly use reference identity — the string identity is silently ignored for reference types, preventing identity collisions between different instances that return the same runtimeIdentity string.

---