Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the SemanticType class to be a record with immutable properties and updates development environment permissions for dotnet SDK installation.
- Converted
SemanticTypefrom a class to a record with init-only properties for immutability - Changed all property setters from
settoinitto prevent modification after construction - Added bash permissions for wget, chmod, and dotnet installation script execution
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/DocFlow.Core/CanonicalModel/SemanticEntity.cs | Converts SemanticType to a record type with init-only properties to enforce immutability |
| .claude/settings.local.json | Adds bash permissions for dotnet SDK installation tools (wget, chmod, installation script) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <summary> | ||
| /// Generic type arguments if applicable | ||
| /// </summary> | ||
| public List<SemanticType> GenericArguments { get; init; } = []; |
There was a problem hiding this comment.
The GenericArguments property uses a List which is still mutable even with the init accessor. Items can be added or removed after construction. Consider using ImmutableList or IReadOnlyList to maintain true immutability consistent with the record type and init-only properties.
| public List<SemanticType> GenericArguments { get; init; } = []; | |
| public IReadOnlyList<SemanticType> GenericArguments { get; init; } = []; |
| /// Represents a type reference in the semantic model | ||
| /// </summary> | ||
| public sealed class SemanticType | ||
| public sealed record SemanticType |
There was a problem hiding this comment.
Converting SemanticType to a record introduces an inconsistency in the codebase design. Other related types like SemanticProperty (line 95), SemanticOperation (line 167), SemanticParameter (line 183), and SemanticAttribute (line 252) remain as classes with set properties. Consider either converting all these types to records for consistency, or keeping SemanticType as a class. If SemanticType specifically benefits from value semantics and immutability, document why it differs from the others.
| "Bash(dotnet build:*)" | ||
| "Bash(dotnet build:*)", | ||
| "Bash(wget:*)", | ||
| "Bash(chmod:*)", |
There was a problem hiding this comment.
Adding wget and chmod permissions to the local settings could introduce security risks. These commands can be used to download and execute arbitrary scripts. Consider documenting why these permissions are needed, or alternatively, use a more restricted approach such as allowing specific script paths or using dotnet SDK installation tools that don't require these broad permissions.
| "Bash(chmod:*)", | |
| "Bash(chmod +x /tmp/dotnet-install.sh:*)", |
No description provided.