Align core data serializers and repository interface with Python SDK#9
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a dictionary-based Serializer runtime and specific model serializers; annotates JsonSchema JSON names; expands IConcurrentToolRepository with async getters and removal and implements them in InMemToolRepository; moves call-template registrations to static constructors across templates; extends some templates; adds unit tests and a test project reference. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant S as Serializer<T>
participant R as Runtime
participant P as PolymorphicRegistry
rect rgb(240,248,255)
note over C,S: ToDictionary flow
C->>S: ToDictionary(obj)
S->>R: ToDictionary(obj)
R-->>S: Dictionary<string, object?>
S-->>C: Dictionary<string, object?>
end
rect rgb(248,240,255)
note over C,S: ValidateDictionary flow (polymorphic)
C->>S: ValidateDictionary(dict)
alt Requires discriminator
S->>P: Resolve type from discriminator
P-->>S: Concrete Type
S->>R: Deserialize(Type, dict)
else Concrete type
S->>R: Deserialize<T>(dict)
end
R-->>S: Instance
S-->>C: T
end
sequenceDiagram
autonumber
participant U as User
participant Repo as InMemToolRepository
participant L as writeLock
participant T as toolsByName
participant M as manualsByName
U->>Repo: RemoveToolAsync(toolName)
Repo->>L: Enter
Repo->>T: TryRemove(toolName)
alt Tool found
Repo->>M: ForEach(manual) remove tool by name
M-->>Repo: Updated manuals (where changed)
Repo-->>U: true
else Tool missing
Repo-->>U: false
end
Repo->>L: Exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (23)
tests/Utcp.Core.Tests/Utcp.Core.Tests.csproj (1)
13-18: Mark test-only packages as PrivateAssets to avoid leaking to consumers.Prevents transitive flow of test deps.
Apply:
- <PackageReference Include="coverlet.collector" Version="6.0.0" /> - <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.8.0" /> - <PackageReference Include="xunit" Version="2.5.3" /> - <PackageReference Include="xunit.runner.visualstudio" Version="2.5.3" /> - <PackageReference Include="FluentAssertions" Version="6.12.0" /> + <PackageReference Include="coverlet.collector" Version="6.0.0" PrivateAssets="all" /> + <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.8.0" PrivateAssets="all" /> + <PackageReference Include="xunit" Version="2.5.3" PrivateAssets="all" /> + <PackageReference Include="xunit.runner.visualstudio" Version="2.5.3"> + <PrivateAssets>all</PrivateAssets> + <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> + </PackageReference> + <PackageReference Include="FluentAssertions" Version="6.12.0" PrivateAssets="all" />src/Utcp.Cli/CliCallTemplate.cs (1)
11-14: Same static registration concern as others—ensure types are initialized before deserialization.Either add a bootstrap or module initializer, or guarantee tests/apps touch these types early.
src/Utcp.Gql/GraphQlCallTemplate.cs (1)
11-14: Registration timing + Uri handling in serializer.
- Ensure GraphQlCallTemplate is initialized before “gql” deserialization occurs.
- Confirm the serializer round-trips System.Uri losslessly (absolute vs relative, normalization). If not, serialize as string explicitly.
src/Utcp.Text/TextCallTemplate.cs (2)
11-14: Static registration timing—same note as other templates.Ensure deterministic init or provide bootstrap.
19-19: New EnsureUnderRoot = true: clarify semantics and enforce consistently.
- If this is a path traversal guard, ensure all text I/O honors it.
- Consider a clearer name (RestrictToRoot/EnforceRoot) and document root resolution rules.
Do tests cover both true/false cases and path traversal edge cases (.., symlinks)?
src/Utcp.Socket/UdpCallTemplate.cs (2)
11-14: Static registration timing—same bootstrap consideration.
16-17: Validate UDP port range.Add validation (1–65535) to avoid invalid templates at the edge.
Apply:
- public int Port { get; init; } + [System.ComponentModel.DataAnnotations.Range(1, 65535)] + public int Port { get; init; }src/Utcp.Http/HttpCallTemplate.cs (3)
12-15: Static registration timing—ensure deterministic init or bootstrap.Same guidance as other templates.
6-6: Unused import.System.Net.Http.Headers isn’t used here; remove to keep file tidy.
-using System.Net.Http.Headers;
17-22: Headers case-insensitivity and Body with GET.
- Consider documenting that header keys should be treated case-insensitively; recommend callers pass Dictionary<string,string>(StringComparer.OrdinalIgnoreCase).
- Body with default GET is unusual; ensure callers/tests expect this.
src/Utcp.Core/Models/Tool.cs (1)
19-41: JsonPropertyName attributes: STJ already imported; add JsonExtensionData to preserve unknown schema members.
- System.Text.Json.Serialization is already globally imported (src/Utcp.Core/GlobalUsings.cs:12) — no local using needed.
- Add a JsonExtensionData property to JsonSchema (src/Utcp.Core/Models/Tool.cs) to avoid losing unknown JSON members during round-trips; e.g.:
[JsonExtensionData] public Dictionary<string, JsonElement> Extra { get; init; } = new();src/Utcp.Mcp/McpCallTemplate.cs (1)
11-14: Make call‑template registration deterministic; registry is already concurrent
- PolymorphicRegistry already uses ConcurrentDictionary — no Dictionary race. (src/Utcp.Core/Serialization/PolymorphicRegistry.cs)
- Static self‑registration is still non‑deterministic: per‑type static ctors (e.g. src/Utcp.Mcp/McpCallTemplate.cs and other *CallTemplate.cs files) run only on first type access; deserializing a discriminator like "mcp" before the type is touched can miss the mapping. Fix by centralizing registration (PolymorphicRegistry static ctor), adding per‑assembly [ModuleInitializer]s, or performing a startup bootstrap (assembly scan or explicit type loads).
- Optional: harden RegisterCallTemplateDerivedType to detect/throw on conflicting discriminator registrations instead of silently overwriting.
tests/Utcp.Core.Tests/InMemToolRepositoryTests.cs (1)
11-43: Unit test logic looks good; build currently blocked by required-member error from HttpCallTemplate.Root cause is missing [SetsRequiredMembers] on the template’s ctor (see separate comments). As a temporary workaround (not preferred), you could set CallTemplateType explicitly in the initializer:
var manualTemplate = new HttpCallTemplate { Name = "manual", Url = new Uri("https://api.example.com/utcp"), + CallTemplateType = "http", };src/Utcp.Http/StreamableHttpCallTemplate.cs (1)
19-21: New properties are reasonable; minor naming nit.For SSE, “text/event-stream” is typically sent as Accept rather than Content-Type on requests. Consider renaming to Accept or clarifying semantics in docs.
tests/Utcp.Core.Tests/SerializationParityTests.cs (2)
53-74: Numeric assertion type-safety: prefer 1L to match dictionary number shape.Runtime converts integers to long (Int64). Using 1L avoids brittle boxing comparisons.
- dict.Should().ContainKey("minLength").WhoseValue.Should().Be(1); + dict.Should().ContainKey("minLength").WhoseValue.Should().Be(1L);
106-134: Same CS9035 consideration for HttpCallTemplate here.Prefer ctor attribute fix; fallback is to set the property explicitly:
var callTemplate = new HttpCallTemplate { Name = "manual", Url = new Uri("https://api.example.com/tool"), + CallTemplateType = "http", };src/Utcp.Core/Serialization/Serializer.cs (1)
97-110: Minor: numeric conversion policy.Current order yields Int64 or Double, else Decimal. Document this in serializer remarks to set expectations for dictionary value types and avoid brittle tests.
src/Utcp.Core/Interfaces/IConcurrentToolRepository.cs (2)
19-19: Return empty collections instead of null for GetToolsByManualAsync.Returning null forces callers into null-checks; an empty list communicates “no tools” unambiguously and aligns with other getters that never return null.
If you can still change the interface, prefer:
-Task<IReadOnlyList<Tool>?> GetToolsByManualAsync(string manualName, CancellationToken cancellationToken = default); +Task<IReadOnlyList<Tool>> GetToolsByManualAsync(string manualName, CancellationToken cancellationToken = default);
14-14: Parameter name is misleading.
GetManualCallTemplateAsync(string manualCallTemplateName)appears to index by the manual’s name (see repository). Rename the parameter tomanualNamefor clarity and consistency.src/Utcp.Core/Repositories/InMemToolRepository.cs (4)
81-93: Avoid double lookup; operate on snapshot to reduce churn.You lookup each manual twice (
Keys.ToList()thenTryGetValue). Iterate overToArray()of KeyValuePairs to avoid the second lookup.- foreach (var manualName in this.manualsByName.Keys.ToList()) - { - if (!this.manualsByName.TryGetValue(manualName, out var manual)) - { - continue; - } + foreach (var kv in this.manualsByName.ToArray()) + { + var manualName = kv.Key; + var manual = kv.Value;
109-114: Naming: parameter likely represents the manual name.If
manualTemplatesByNameis keyed by manual name (set in SaveManualAsync), the parameter should bemanualNameto match usage.
139-147: Prefer returning empty list over null for GetToolsByManualAsync.This avoids null checks and is consistent with other getters.
- public Task<IReadOnlyList<Tool>?> GetToolsByManualAsync(string manualName, CancellationToken cancellationToken = default) + public Task<IReadOnlyList<Tool>> GetToolsByManualAsync(string manualName, CancellationToken cancellationToken = default) { - if (this.manualsByName.TryGetValue(manualName, out var manual)) - { - return Task.FromResult<IReadOnlyList<Tool>?>(manual.Tools.ToList()); - } - - return Task.FromResult<IReadOnlyList<Tool>?>(null); + return Task.FromResult( + this.manualsByName.TryGetValue(manualName, out var manual) + ? (IReadOnlyList<Tool>)manual.Tools.ToList() + : Array.Empty<Tool>()); }Note: requires corresponding interface change.
17-44: Assumption: tool names are globally unique.Saving a manual inserts each tool into a global
toolsByNameby tool name; saving another manual with a tool of the same name will overwrite. If uniqueness isn’t guaranteed across manuals, keying should include the manual scope.If not globally unique, consider composite keys (
$"{manualName}:{tool.Name}") or a per-manual index.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/Utcp.Cli/CliCallTemplate.cs(1 hunks)src/Utcp.Core/Interfaces/IConcurrentToolRepository.cs(1 hunks)src/Utcp.Core/Models/Serialization/ModelSerializers.cs(1 hunks)src/Utcp.Core/Models/Tool.cs(2 hunks)src/Utcp.Core/Repositories/InMemToolRepository.cs(1 hunks)src/Utcp.Core/Serialization/Serializer.cs(1 hunks)src/Utcp.Gql/GraphQlCallTemplate.cs(1 hunks)src/Utcp.Http/HttpCallTemplate.cs(1 hunks)src/Utcp.Http/StreamableHttpCallTemplate.cs(1 hunks)src/Utcp.Mcp/McpCallTemplate.cs(1 hunks)src/Utcp.Socket/TcpCallTemplate.cs(1 hunks)src/Utcp.Socket/UdpCallTemplate.cs(1 hunks)src/Utcp.Text/TextCallTemplate.cs(1 hunks)tests/Utcp.Core.Tests/InMemToolRepositoryTests.cs(1 hunks)tests/Utcp.Core.Tests/SerializationParityTests.cs(1 hunks)tests/Utcp.Core.Tests/Utcp.Core.Tests.csproj(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
src/Utcp.Socket/UdpCallTemplate.cs (1)
src/Utcp.Core/Serialization/PolymorphicRegistry.cs (1)
RegisterCallTemplateDerivedType(22-25)
src/Utcp.Core/Models/Serialization/ModelSerializers.cs (2)
src/Utcp.Core/Serialization/Serializer.cs (7)
Serializer(10-112)Runtime(21-111)Runtime(25-34)Deserialize(64-68)Dictionary(14-14)Dictionary(36-50)Dictionary(86-95)src/Utcp.Core/Serialization/PolymorphicRegistry.cs (2)
TryGetAuthType(37-40)TryGetCallTemplateType(32-35)
tests/Utcp.Core.Tests/InMemToolRepositoryTests.cs (2)
src/Utcp.Core/Interfaces/IConcurrentToolRepository.cs (11)
Task(10-10)Task(11-11)Task(12-12)Task(13-13)Task(14-14)Task(15-15)Task(16-16)Task(17-17)Task(18-18)Task(19-19)Task(20-20)src/Utcp.Core/Repositories/InMemToolRepository.cs (6)
Task(17-44)Task(46-69)Task(71-101)Task(103-107)Task(109-113)InMemToolRepository(10-154)
tests/Utcp.Core.Tests/SerializationParityTests.cs (3)
src/Utcp.Core/Models/Serialization/ModelSerializers.cs (13)
AuthSerializer(9-35)CallTemplateSerializer(37-63)Dictionary(31-34)Dictionary(59-62)Dictionary(72-75)Dictionary(85-88)Dictionary(98-101)JsonSchemaSerializer(65-76)JsonSchema(67-70)Tool(80-83)ToolSerializer(78-89)UtcpManual(93-96)UtcpManualSerializer(91-102)src/Utcp.Http/HttpCallTemplate.cs (2)
HttpCallTemplate(12-15)HttpCallTemplate(23-26)src/Utcp.Core/Serialization/Serializer.cs (3)
Dictionary(14-14)Dictionary(36-50)Dictionary(86-95)
src/Utcp.Mcp/McpCallTemplate.cs (1)
src/Utcp.Core/Serialization/PolymorphicRegistry.cs (1)
RegisterCallTemplateDerivedType(22-25)
src/Utcp.Text/TextCallTemplate.cs (1)
src/Utcp.Core/Serialization/PolymorphicRegistry.cs (1)
RegisterCallTemplateDerivedType(22-25)
src/Utcp.Socket/TcpCallTemplate.cs (1)
src/Utcp.Core/Serialization/PolymorphicRegistry.cs (1)
RegisterCallTemplateDerivedType(22-25)
src/Utcp.Gql/GraphQlCallTemplate.cs (1)
src/Utcp.Core/Serialization/PolymorphicRegistry.cs (1)
RegisterCallTemplateDerivedType(22-25)
src/Utcp.Core/Repositories/InMemToolRepository.cs (1)
src/Utcp.Core/Interfaces/IConcurrentToolRepository.cs (11)
Task(10-10)Task(11-11)Task(12-12)Task(13-13)Task(14-14)Task(15-15)Task(16-16)Task(17-17)Task(18-18)Task(19-19)Task(20-20)
src/Utcp.Core/Interfaces/IConcurrentToolRepository.cs (1)
src/Utcp.Core/Models/Serialization/ModelSerializers.cs (3)
CallTemplate(39-57)UtcpManual(93-96)Tool(80-83)
src/Utcp.Http/HttpCallTemplate.cs (1)
src/Utcp.Core/Serialization/PolymorphicRegistry.cs (1)
RegisterCallTemplateDerivedType(22-25)
src/Utcp.Cli/CliCallTemplate.cs (1)
src/Utcp.Core/Serialization/PolymorphicRegistry.cs (1)
RegisterCallTemplateDerivedType(22-25)
src/Utcp.Core/Serialization/Serializer.cs (2)
src/Utcp.Core/Models/Serialization/ModelSerializers.cs (5)
Dictionary(31-34)Dictionary(59-62)Dictionary(72-75)Dictionary(85-88)Dictionary(98-101)src/Utcp.Core/Models/Serialization/CustomConverters.cs (2)
CallTemplateJsonConverter(11-35)AuthJsonConverter(37-61)
src/Utcp.Http/StreamableHttpCallTemplate.cs (1)
src/Utcp.Core/Serialization/PolymorphicRegistry.cs (1)
RegisterCallTemplateDerivedType(22-25)
🪛 GitHub Check: build-test-pack (macos-latest)
tests/Utcp.Core.Tests/InMemToolRepositoryTests.cs
[failure] 49-49:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 15-15:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 49-49:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 15-15:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
tests/Utcp.Core.Tests/SerializationParityTests.cs
[failure] 109-109:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 79-79:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 36-36:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 109-109:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 79-79:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 36-36:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
🪛 GitHub Check: build-test-pack (ubuntu-latest)
tests/Utcp.Core.Tests/InMemToolRepositoryTests.cs
[failure] 49-49:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 15-15:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 49-49:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 15-15:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
tests/Utcp.Core.Tests/SerializationParityTests.cs
[failure] 109-109:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 79-79:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 36-36:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 109-109:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 79-79:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
[failure] 36-36:
Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
🪛 GitHub Actions: build
tests/Utcp.Core.Tests/InMemToolRepositoryTests.cs
[error] 15-15: dotnet build Utcp.sln -c Release --no-restore: CS9035: Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
tests/Utcp.Core.Tests/SerializationParityTests.cs
[error] 36-36: dotnet build Utcp.sln -c Release --no-restore: CS9035: Required member 'CallTemplate.CallTemplateType' must be set in the object initializer or attribute constructor.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (18)
tests/Utcp.Core.Tests/Utcp.Core.Tests.csproj (1)
24-27: Good: add Utcp.Http reference to enable Http-related tests.Matches the new serializer + call-template coverage.
src/Utcp.Socket/TcpCallTemplate.cs (2)
11-14: Static type registration moved to a static ctor — good change.This avoids repeated per-instance registration and eliminates races during first use.
19-22: Mark parameterless CallTemplate ctors with [SetsRequiredMembers] to fix CS9035Annotate the parameterless constructor so the compiler recognizes that the required base member (CallTemplateType) is set.
+using System.Diagnostics.CodeAnalysis; using Utcp.Core.Models; using Utcp.Core.Serialization; public sealed record TcpCallTemplate : CallTemplate { @@ - public TcpCallTemplate() + [SetsRequiredMembers] + public TcpCallTemplate() { CallTemplateType = "tcp"; } }Apply the same attribute to parameterless ctors for: HttpCallTemplate, StreamableHttpCallTemplate, UdpCallTemplate, TextCallTemplate, CliCallTemplate, GraphQlCallTemplate, McpCallTemplate.
tests/Utcp.Core.Tests/InMemToolRepositoryTests.cs (3)
45-64: LGTM: positive path for GetManualCallTemplateAsync.Covers storing and retrieving the manual’s template.
66-72: LGTM: null contract for unknown manual.This matches the interface’s nullable return type.
74-80: LGTM: RemoveToolAsync negative path covered.Good guard ensuring no false positives on missing tool.
src/Utcp.Http/StreamableHttpCallTemplate.cs (2)
11-14: Static registration via type initializer — good.Consistent with the new pattern and avoids redundant work.
23-26: Do not add [SetsRequiredMembers] to these ctors — they don't initialize all required members (Name).CallTemplate (src/Utcp.Core/Models/CallTemplate.cs) declares required CallTemplateType and Name; the parameterless constructors only set CallTemplateType. Marking them [SetsRequiredMembers] would suppress the compiler check for Name and could hide missing initializations. Either initialize all required members in the ctor and then add [SetsRequiredMembers], or leave the ctors unchanged and keep callers supplying Name via object initializers. (learn.microsoft.com)
Likely an incorrect or invalid review comment.
tests/Utcp.Core.Tests/SerializationParityTests.cs (3)
12-30: LGTM: Auth round‑trip and discriminator assertion.Covers both ToDictionary and ValidateDictionary for a built‑in type.
76-104: LGTM: nested Tool round‑trip including embedded HttpCallTemplate and JsonSchema.Covers key presence and deep equivalence; comparing JsonSchema by members is appropriate.
33-50: No change required — ctor already sets the required memberHttpCallTemplate's parameterless constructor assigns CallTemplateType = "http" (src/Utcp.Http/HttpCallTemplate.cs), so the CS9035 concern is incorrect; do not add [SetsRequiredMembers] or modify tests.
Likely an incorrect or invalid review comment.
src/Utcp.Core/Serialization/Serializer.cs (2)
21-34: Good: central, one‑time JsonSerializerOptions with custom converters.SnakeCaseLower and the polymorphic converters match the serializer expectations.
36-50: Defensive checks and object-only guarantee are appropriate.ToDictionary throws for non-object roots; ConvertElement handles all kinds.
src/Utcp.Core/Interfaces/IConcurrentToolRepository.cs (1)
12-12: Specify case-sensitivity in contract (tool/manual name lookups).Define whether lookups are case-sensitive. The current repo implementation mixes sensitivities, which can surprise callers.
Add XML doc comments clarifying the expectation and ensure implementation matches.
Also applies to: 18-18
src/Utcp.Core/Repositories/InMemToolRepository.cs (1)
121-126: Confirm immutability or defensive copy for GetManualAsync.If
UtcpManual.Toolsis mutable, returning the stored instance exposes internal state. EnsureUtcpManualis effectively immutable or return a copy.src/Utcp.Core/Models/Serialization/ModelSerializers.cs (3)
9-35: AuthSerializer: solid discriminator validation.The flow (discriminator → registry → deserialize) is correct and failures are explicit.
37-63: CallTemplateSerializer: LGTM.Mirrors AuthSerializer with clear errors; aligns with PolymorphicRegistry.
65-102: JsonSchema/Tool/UtcpManual serializers: simple and correct passthroughs.Directly delegating to Runtime keeps behavior consistent with global options (snake_case, converters).
| public async Task<bool> RemoveToolAsync(string toolName, CancellationToken cancellationToken = default) | ||
| { | ||
| await this.writeLock.WaitAsync(cancellationToken).ConfigureAwait(false); | ||
| try | ||
| { | ||
| if (!this.toolsByName.TryRemove(toolName, out _)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| foreach (var manualName in this.manualsByName.Keys.ToList()) | ||
| { | ||
| if (!this.manualsByName.TryGetValue(manualName, out var manual)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| var remaining = manual.Tools.Where(t => !string.Equals(t.Name, toolName, StringComparison.OrdinalIgnoreCase)).ToList(); | ||
| if (remaining.Count != manual.Tools.Count) | ||
| { | ||
| this.manualsByName[manualName] = manual with { Tools = remaining }; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
| finally | ||
| { | ||
| this.writeLock.Release(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent case handling can make RemoveToolAsync fail unexpectedly.
toolsByName.TryRemove(toolName, out _) is case-sensitive, but the subsequent manual update compares names with OrdinalIgnoreCase. If callers pass a different casing, removal returns false and manuals aren’t updated.
Two fixes; prefer A:
A) Make dictionaries case-insensitive (applies outside this hunk):
- private readonly ConcurrentDictionary<string, Tool> toolsByName = new();
- private readonly ConcurrentDictionary<string, UtcpManual> manualsByName = new();
- private readonly ConcurrentDictionary<string, CallTemplate> manualTemplatesByName = new();
+ private readonly ConcurrentDictionary<string, Tool> toolsByName = new(StringComparer.OrdinalIgnoreCase);
+ private readonly ConcurrentDictionary<string, UtcpManual> manualsByName = new(StringComparer.OrdinalIgnoreCase);
+ private readonly ConcurrentDictionary<string, CallTemplate> manualTemplatesByName = new(StringComparer.OrdinalIgnoreCase);B) Or, perform a case-insensitive key resolution before removing:
- if (!this.toolsByName.TryRemove(toolName, out _))
+ var key = this.toolsByName.Keys.FirstOrDefault(k => string.Equals(k, toolName, StringComparison.OrdinalIgnoreCase));
+ if (key is null || !this.toolsByName.TryRemove(key, out _))
{
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async Task<bool> RemoveToolAsync(string toolName, CancellationToken cancellationToken = default) | |
| { | |
| await this.writeLock.WaitAsync(cancellationToken).ConfigureAwait(false); | |
| try | |
| { | |
| if (!this.toolsByName.TryRemove(toolName, out _)) | |
| { | |
| return false; | |
| } | |
| foreach (var manualName in this.manualsByName.Keys.ToList()) | |
| { | |
| if (!this.manualsByName.TryGetValue(manualName, out var manual)) | |
| { | |
| continue; | |
| } | |
| var remaining = manual.Tools.Where(t => !string.Equals(t.Name, toolName, StringComparison.OrdinalIgnoreCase)).ToList(); | |
| if (remaining.Count != manual.Tools.Count) | |
| { | |
| this.manualsByName[manualName] = manual with { Tools = remaining }; | |
| } | |
| } | |
| return true; | |
| } | |
| finally | |
| { | |
| this.writeLock.Release(); | |
| } | |
| } | |
| public async Task<bool> RemoveToolAsync(string toolName, CancellationToken cancellationToken = default) | |
| { | |
| await this.writeLock.WaitAsync(cancellationToken).ConfigureAwait(false); | |
| try | |
| { | |
| var key = this.toolsByName.Keys.FirstOrDefault(k => string.Equals(k, toolName, StringComparison.OrdinalIgnoreCase)); | |
| if (key is null || !this.toolsByName.TryRemove(key, out _)) | |
| { | |
| return false; | |
| } | |
| foreach (var manualName in this.manualsByName.Keys.ToList()) | |
| { | |
| if (!this.manualsByName.TryGetValue(manualName, out var manual)) | |
| { | |
| continue; | |
| } | |
| var remaining = manual.Tools.Where(t => !string.Equals(t.Name, toolName, StringComparison.OrdinalIgnoreCase)).ToList(); | |
| if (remaining.Count != manual.Tools.Count) | |
| { | |
| this.manualsByName[manualName] = manual with { Tools = remaining }; | |
| } | |
| } | |
| return true; | |
| } | |
| finally | |
| { | |
| this.writeLock.Release(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Utcp.Core/Repositories/InMemToolRepository.cs around lines 71 to 101,
RemoveToolAsync uses toolsByName.TryRemove(toolName, out _) which is
case-sensitive while later comparisons use StringComparison.OrdinalIgnoreCase,
causing removals to fail for differently-cased input; fix by making the relevant
dictionaries (toolsByName and manualsByName) use a case-insensitive comparer
(e.g., StringComparer.OrdinalIgnoreCase) at construction so keys are normalized
and TryRemove behaves consistently, or if you prefer the alternate approach,
resolve the actual stored key by searching toolsByName.Keys with an
ordinal-ignore-case match and remove that concrete key before updating manuals.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68c85f8aafa8832291a5cbaa72f01c88
Summary by CodeRabbit
New Features
Improvements
Tests