Refactor/simplify credentials and improve data model#26
Conversation
- Remove credential fields from AnalysisRequest (BREAKING)
- Credentials now only configured at application level
- Removed AccessToken and AzureDevOpsOrgUrl from request model
- Removed ICredentialResolver interface and implementation
- Migrate AnalysisRequest to EF Core owned entity (BREAKING)
- Changed from JsonDocument to strongly-typed owned entity
- Improved query performance with direct property access
- Removed SanitizedAnalysisRequest class
- Add DELETE endpoint for analysis history
- New endpoint: DELETE /api/analysis/history?jobId={guid}
- Allows deletion of analysis jobs by ID
- Improve exception handling
- Enhanced CustomExceptionHandler to handle all exceptions
- Added proper error responses with trace IDs
- Update Claude analyzer defaults
- MaxTokens increased to 40960
- Temperature set to 0.5
- Remove custom instructions from prompt templates
- Removed {{customInstructions}} placeholder from all templates
- Simplify MCP service resolver
- Removed GetAvailableMcpServices() method
- Update documentation
- Updated GIT_CREDENTIALS_CONFIGURATION.md
- Updated README.md with correct configuration structure
- Updated CHANGELOG.md with all changes
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
DawnDevelop
deleted the
refactor/simplify-credentials-and-improve-data-model
branch
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a significant refactoring to simplify credential management and improve the data model architecture. The changes remove per-request credential handling in favor of application-level configuration, and migrate from JsonDocument storage to EF Core owned entities for better type safety.
Key Changes
- Credential Model Refactoring: Removed
AccessTokenandAzureDevOpsOrgUrlfromAnalysisRequest, centralizing credential management at the application level via configuration or environment variables - Data Model Improvement: Replaced
JsonDocumentstorage with EF Core owned entities (OwnsOne/OwnsMany) forAnalysisRequest, improving type safety and queryability - Code Cleanup: Removed
ICredentialResolver,CredentialResolver,SanitizedAnalysisRequest, and related infrastructure; renamedAnalysisPromptBuildertoPromptBuilder
Reviewed Changes
Copilot reviewed 55 out of 55 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
src/Lintellect.Shared/Models/AnalysisRequest.cs |
Removed credential fields (AccessToken, AzureDevOpsOrgUrl) from data model |
src/Lintellect.Api/Domain/Entities/AnalysisJob.cs |
Changed from JsonDocument to AnalysisRequest typed property; added deep copy method |
src/Lintellect.Api/Infrastructure/Persistence/Configurations/AnalysisJobConfiguration.cs |
Migrated from JSON conversion to owned entity configuration |
src/Lintellect.Api/Infrastructure/Services/Git/GitClientFactory.cs |
Refactored to use IOptionsMonitor instead of ICredentialResolver |
src/Lintellect.Api/Infrastructure/Services/Git/CredentialResolver.cs |
Removed entire file - no longer needed |
src/Lintellect.Api/ConfigureServices.cs |
Updated to configure credentials from environment variables via PostConfigure |
src/Lintellect.Api/Application/Messages/Queries/GetAnalysisHistoryQuery.cs |
Simplified queries using strongly-typed properties instead of JSON navigation |
src/Lintellect.Api/Application/Messages/Commands/Analysis/DeleteAnalysisHistoryCommand.cs |
New command for deleting analysis history |
src/Lintellect.Api/Apis/Infrastructure/CustomExceptionHandler.cs |
Enhanced to handle all unhandled exceptions with proper logging |
src/Lintellect.Api/Infrastructure/Services/AI/Prompts/PromptBuilder.cs |
Renamed from AnalysisPromptBuilder; improved prompt generation with file prioritization |
src/Lintellect.Api/Infrastructure/Services/AI/ClaudeAnalyzerService.cs |
Updated to use PromptBuilder; added CreateMessageParameters helper method |
tests/Lintellect.Api.UnitTests/Domain/AnalysisJobTests.cs |
Added test for deep copy snapshot functionality |
tests/Lintellect.Api.UnitTests/Builders/AnalysisRequestBuilder.cs |
Removed credential-related builder methods |
docs/GIT_CREDENTIALS_CONFIGURATION.md |
Updated documentation to reflect application-level credential configuration |
Directory.Packages.props |
Updated multiple package versions including Aspire, FluentValidation, and test frameworks |
| Multiple prompt templates | Improved formatting and structure; removed custom instructions placeholders |
Comments suppressed due to low confidence (3)
src/Lintellect.Api/Infrastructure/Services/AI/Prompts/PromptBuilder.cs:324
- In the
BuildStaticAnalyzerFindingsSectionmethod, line 324 may throw anArgumentOutOfRangeExceptioniffinding.Messageis less than 150 characters. The code should check the message length before attempting to truncate it.
Change line 324 from:
builder.AppendLine($" Message: {finding.Message[..150]}");to:
var message = finding.Message.Length > 150 ? finding.Message[..150] + "..." : finding.Message;
builder.AppendLine($" Message: {message}");tests/Lintellect.Api.FunctionalTests/Queries/GetAnalysisHistoryQueryTests.cs:95
- This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
src/Lintellect.Api/Infrastructure/Services/AI/ClaudeAnalyzerService.cs:217 - This assignment to tokenCount is useless, since its value is never read.
var tokenCount = await _client.Messages.CountMessageTokensAsync(new()
{
Messages = idDescriptionSummary.MessageParameters.Messages,
Tools = idDescriptionSummary.MessageParameters.Tools,
Model = idDescriptionSummary.MessageParameters.Model,
System = idDescriptionSummary.MessageParameters.System
});
| ## Your Task: | ||
| Generate inline code suggestions as structured JSON that can be posted as PR comments. | ||
| Each suggestion must include the file path, line number, explanation, and corrected TypeScript code. |
There was a problem hiding this comment.
In the Java inline suggestions prompt template, there's a duplicate "Your Task" section (lines 12-15 and 27-29). Additionally, line 29 incorrectly mentions "corrected TypeScript code" instead of "corrected Java code" for this Java-specific template.
| ## Your Task: | |
| Generate inline code suggestions as structured JSON that can be posted as PR comments. | |
| Each suggestion must include the file path, line number, explanation, and corrected TypeScript code. |
| } | ||
|
|
||
| private async Task PostInlineSuggestionsAsync( | ||
| private static async Task PostInlineSuggestionsAsync( |
There was a problem hiding this comment.
[nitpick] The method signature on line 249 is changed from instance to static, but the method calls prService which is passed as a parameter. While this works, the change to static seems unnecessary since the method doesn't access any instance state anyway. However, if this is intentional for consistency, it's acceptable.
More importantly, consider adding a null check for the suggestions parameter or documenting that it should never be null.
| var query = context.AnalysisJobs.AsQueryable(); | ||
| if (request.JobId != Guid.Empty) | ||
| { | ||
| query = query.Where(x => x.Id == request.JobId); | ||
| } | ||
|
|
||
| await query.ExecuteDeleteAsync(cancellationToken); |
There was a problem hiding this comment.
The DeleteAnalysisHistoryCommand allows deleting all analysis jobs when JobId is Guid.Empty. This could be a dangerous operation if called accidentally. Consider:
- Requiring a non-empty GUID, or
- Adding a separate command/endpoint for bulk deletion with explicit confirmation, or
- At minimum, adding a warning log when deleting all records
This prevents accidental data loss from a misconfigured or buggy client call.
| <PackageVersion Include="NUnit.Analyzers" Version="4.11.0" /> | ||
| <PackageVersion Include="NUnit3TestAdapter" Version="6.0.0-beta.1" /> | ||
| <PackageVersion Include="NUnit.Analyzers" Version="4.11.2" /> | ||
| <PackageVersion Include="NUnit3TestAdapter" Version="5.2.0" /> |
There was a problem hiding this comment.
[nitpick] The NUnit3TestAdapter version has been downgraded from 6.0.0-beta.1 to 5.2.0. While this moves from a beta to a stable version (which is generally good), ensure this downgrade doesn't remove any features that were being used from the beta version.
| ## Your Task: | ||
| Generate inline code suggestions as structured JSON that can be posted as PR comments. | ||
| Each suggestion must include the file path, line number, explanation, and corrected JavaScript code. | ||
| Each suggestion must include the file path, line number, explanation, and corrected TypeScript code. |
There was a problem hiding this comment.
In the JavaScript inline suggestions prompt template, the task description incorrectly mentions "TypeScript code" instead of "JavaScript code". This should be corrected to maintain consistency with the JavaScript-specific nature of this template.
Change line 14 from:
Each suggestion must include the file path, line number, explanation, and corrected TypeScript code.
to:
Each suggestion must include the file path, line number, explanation, and corrected JavaScript code.
| .NotNull() | ||
| .NotEmpty() | ||
| .MaximumLength(100) | ||
| .WithMessage("Analyzer used must be specified and not exceed 100 characters."); |
There was a problem hiding this comment.
[nitpick] The validation rules for Summary, DetailedAnalysis, and InlineSuggestions have been removed. While the PR description mentions simplification, removing validation for required fields could lead to invalid data being persisted.
Consider whether these fields should still be validated, especially if they're required by the domain logic. If they're now optional, this change is acceptable, but it should be clearly documented.
| .WithMessage("Analyzer used must be specified and not exceed 100 characters."); | |
| .WithMessage("Analyzer used must be specified and not exceed 100 characters."); | |
| RuleFor(x => x.Summary) | |
| .NotNull() | |
| .NotEmpty() | |
| .MaximumLength(2000) | |
| .WithMessage("Summary is required and must not exceed 2000 characters."); | |
| RuleFor(x => x.DetailedAnalysis) | |
| .NotNull() | |
| .NotEmpty() | |
| .MaximumLength(10000) | |
| .WithMessage("Detailed analysis is required and must not exceed 10000 characters."); | |
| RuleFor(x => x.InlineSuggestions) | |
| .NotNull() | |
| .WithMessage("Inline suggestions are required."); |
| await HandleUnknownException(httpContext, exception); | ||
| return true; |
There was a problem hiding this comment.
[nitpick] The exception handler now handles all exceptions and returns true, which means the default ASP.NET Core exception handling pipeline will never run. While this provides consistent error responses, it may hide exceptions that should be handled differently (e.g., task cancellation).
Consider checking if the response has started before attempting to write, and potentially allowing certain exception types (like OperationCanceledException) to pass through to the default handler.
| return new AnalysisRequest | ||
| { | ||
| Language = request.Language, | ||
| Findings = request.Findings? | ||
| .Select(finding => new AnalyzerFindings | ||
| { | ||
| RuleId = finding.RuleId, | ||
| Message = finding.Message, | ||
| FilePath = finding.FilePath, | ||
| Line = finding.Line, | ||
| Severity = finding.Severity | ||
| }) | ||
| .ToArray() ?? Array.Empty<AnalyzerFindings>(), | ||
| GitInfo = request.GitInfo is null | ||
| ? null | ||
| : new GitInfo( | ||
| request.GitInfo.PullRequestId, | ||
| request.GitInfo.CommitId, | ||
| request.GitInfo.RepositoryName, | ||
| request.GitInfo.Type, | ||
| request.GitInfo.ProjectName), | ||
| GitProvider = request.GitProvider, | ||
| FileExclusions = request.FileExclusions is null ? [] : [.. request.FileExclusions], | ||
| EnableSummaryComment = request.EnableSummaryComment, | ||
| EnableInlineSuggestions = request.EnableInlineSuggestions, | ||
| EnableDescriptionSummary = request.EnableDescriptionSummary, | ||
| EnableAzureDevopsCodeOwners = request.EnableAzureDevopsCodeOwners, | ||
| McpServer = request.McpServer is null ? [] : [.. request.McpServer], | ||
| AIAnalyzer = request.AIAnalyzer | ||
| }; |
There was a problem hiding this comment.
[nitpick] The CloneAnalysisRequest method performs a manual deep copy of the AnalysisRequest object. However, this approach is brittle and requires updates whenever new properties are added to the AnalysisRequest class.
Consider using a more maintainable approach:
- Implement
ICloneableonAnalysisRequest, or - Use a serialization-based cloning mechanism, or
- Add unit tests to ensure all properties are copied (which exists in the test file)
The current test on lines 23-42 in AnalysisJobTests.cs helps catch missing properties, but a more maintainable solution would reduce the chance of errors.
| return new AnalysisRequest | |
| { | |
| Language = request.Language, | |
| Findings = request.Findings? | |
| .Select(finding => new AnalyzerFindings | |
| { | |
| RuleId = finding.RuleId, | |
| Message = finding.Message, | |
| FilePath = finding.FilePath, | |
| Line = finding.Line, | |
| Severity = finding.Severity | |
| }) | |
| .ToArray() ?? Array.Empty<AnalyzerFindings>(), | |
| GitInfo = request.GitInfo is null | |
| ? null | |
| : new GitInfo( | |
| request.GitInfo.PullRequestId, | |
| request.GitInfo.CommitId, | |
| request.GitInfo.RepositoryName, | |
| request.GitInfo.Type, | |
| request.GitInfo.ProjectName), | |
| GitProvider = request.GitProvider, | |
| FileExclusions = request.FileExclusions is null ? [] : [.. request.FileExclusions], | |
| EnableSummaryComment = request.EnableSummaryComment, | |
| EnableInlineSuggestions = request.EnableInlineSuggestions, | |
| EnableDescriptionSummary = request.EnableDescriptionSummary, | |
| EnableAzureDevopsCodeOwners = request.EnableAzureDevopsCodeOwners, | |
| McpServer = request.McpServer is null ? [] : [.. request.McpServer], | |
| AIAnalyzer = request.AIAnalyzer | |
| }; | |
| // Use System.Text.Json for deep cloning | |
| var json = System.Text.Json.JsonSerializer.Serialize(request); | |
| return System.Text.Json.JsonSerializer.Deserialize<AnalysisRequest>(json) | |
| ?? throw new InvalidOperationException("Failed to clone AnalysisRequest."); |
| messageParams.MCPServers ??= []; | ||
| foreach (var mcp in mcpServers) | ||
| { | ||
| messageParams.MCPServers.Add(new() | ||
| { | ||
| Name = mcp.Name, | ||
| Url = mcp.Url, | ||
| AuthorizationToken = mcp.AuthorizationToken, | ||
| ToolConfiguration = new MCPToolConfiguration() | ||
| { | ||
| Enabled = true, | ||
| AllowedTools = [$"{mcp.Name}"] | ||
| } | ||
| }); |
There was a problem hiding this comment.
The CreateMessageParameters method is called with mcpServers parameter but then unconditionally initializes messageParams.MCPServers ??= [] on line 330 and iterates over the parameter. However, if mcpServers is already an empty list (from the ??= [] on line 314), the code still checks for empty lists in the comment on line 329.
The comment on line 329 states "Only set MCPServers if list is not empty (Claude API rejects empty lists)" but the code doesn't actually check if the list is empty before setting it. Consider adding a check:
// Only set MCPServers if list is not empty (Claude API rejects empty lists)
if (mcpServers.Count > 0)
{
messageParams.MCPServers ??= [];
foreach (var mcp in mcpServers)
{
// ... existing code
}
}| messageParams.MCPServers ??= []; | |
| foreach (var mcp in mcpServers) | |
| { | |
| messageParams.MCPServers.Add(new() | |
| { | |
| Name = mcp.Name, | |
| Url = mcp.Url, | |
| AuthorizationToken = mcp.AuthorizationToken, | |
| ToolConfiguration = new MCPToolConfiguration() | |
| { | |
| Enabled = true, | |
| AllowedTools = [$"{mcp.Name}"] | |
| } | |
| }); | |
| if (mcpServers.Count > 0) | |
| { | |
| messageParams.MCPServers ??= []; | |
| foreach (var mcp in mcpServers) | |
| { | |
| messageParams.MCPServers.Add(new() | |
| { | |
| Name = mcp.Name, | |
| Url = mcp.Url, | |
| AuthorizationToken = mcp.AuthorizationToken, | |
| ToolConfiguration = new MCPToolConfiguration() | |
| { | |
| Enabled = true, | |
| AllowedTools = [$"{mcp.Name}"] | |
| } | |
| }); | |
| } |
| var tokenCount = await _client.Messages.CountMessageTokensAsync(new() | ||
| { | ||
| Messages = idDescriptionSummary.MessageParameters.Messages, | ||
| Tools = idDescriptionSummary.MessageParameters.Tools, | ||
| Model = idDescriptionSummary.MessageParameters.Model, | ||
| System = idDescriptionSummary.MessageParameters.System | ||
| }); | ||
|
|
There was a problem hiding this comment.
The token counting code on lines 211-217 is added but the result tokenCount is never used. This appears to be debug code that should either be utilized (e.g., logged) or removed.
If this is intentional for future use, consider adding a TODO comment explaining its purpose. Otherwise, remove the unused code to avoid confusion.
| var tokenCount = await _client.Messages.CountMessageTokensAsync(new() | |
| { | |
| Messages = idDescriptionSummary.MessageParameters.Messages, | |
| Tools = idDescriptionSummary.MessageParameters.Tools, | |
| Model = idDescriptionSummary.MessageParameters.Model, | |
| System = idDescriptionSummary.MessageParameters.System | |
| }); |
Pull Request
Description
Brief description of changes made in this PR.
Type of Change
Testing
Checklist
Related Issues