Skip to content

feat: Retry for security analysis #121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dkorecko wants to merge 1 commit into from
Closed

feat: Retry for security analysis #121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 124 additions & 0 deletions PatchPanda.Units/Services/VersionServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Logging;
using Octokit;
using PatchPanda.Units.Helpers;
using PatchPanda.Web;
using PatchPanda.Web.Db;
using PatchPanda.Web.Services;

namespace PatchPanda.Units.Services;

public class VersionServiceTests
{
private class TestableVersionService : VersionService
{
public Mock<IGitHubClient> MockGitHubClient { get; } = new();

public TestableVersionService(
ILogger<VersionService> logger,
IConfiguration configuration,
IDbContextFactory<DataContext> dbContextFactory,
IAiService aiService
) : base(logger, configuration, dbContextFactory, aiService)
{
}

protected override IGitHubClient GetClient()
{
return MockGitHubClient.Object;
}
}

private readonly Mock<ILogger<VersionService>> _logger = new();
private readonly Mock<IConfiguration> _configuration = new();
private readonly Mock<IAiService> _aiService = new();
private readonly IDbContextFactory<DataContext> _dbContextFactory = Helper.CreateInMemoryFactory();

[Fact]
public async Task SecurityCheck_Retries_On_Failure()
{
// Arrange
var service = new TestableVersionService(
_logger.Object,
_configuration.Object,
_dbContextFactory,
_aiService.Object
);

using var db = _dbContextFactory.CreateDbContext();

// Setup App Settings
db.AppSettings.Add(new AppSetting { Key = Constants.SettingsKeys.SECURITY_SCANNING_ENABLED, Value = "true" });
await db.SaveChangesAsync();

var stack = Helper.GetTestStack();
var app = stack.Apps[0];
app.GitHubRepo = new Tuple<string, string>(TestData.GITHUB_OWNER, TestData.GITHUB_REPO);
app.NewerVersions.Clear();

db.Containers.Add(app);
await db.SaveChangesAsync();

// Setup AI Service
_aiService.Setup(x => x.IsInitialized()).Returns(true);

// Fail twice, then succeed
_aiService.SetupSequence(x => x.AnalyzeDiff(It.IsAny<string>()))
.ThrowsAsync(new Exception(TestData.AI_ERROR))
.ThrowsAsync(new Exception(TestData.AI_ERROR))
.ReturnsAsync(new SecurityAnalysisResult { Analysis = TestData.SAFE_ANALYSIS, IsSuspectedMalicious = false });
Comment on lines +64 to +69
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Potential test gap: consider testing when all retries are exhausted.

The test covers the happy path (fail twice, succeed on third). Consider adding a complementary test where all attempts fail, verifying that the result has no SecurityAnalysis set and that the final error is logged.

🤖 Prompt for AI Agents 
In `@PatchPanda.Units/Services/VersionServiceTests.cs` around lines 64 - 69, Add a
new unit test in VersionServiceTests that simulates all AI retries failing:
setup _aiService.SetupSequence(x => x.AnalyzeDiff(It.IsAny<string>())) to
ThrowsAsync(new Exception(TestData.AI_ERROR)) for every retry attempt, call the
same VersionService method under test that triggers AnalyzeDiff (the existing
test's invocation), and assert the returned result has no SecurityAnalysis
(e.g., SecurityAnalysis property is null or default) and that the mocked logger
(the test's logger mock) received an error-level log about the final failure;
use the same TestData.AI_ERROR and SecurityAnalysisResult type references so the
test mirrors existing patterns.


// Setup GitHub Client
var mockRepo = new Mock<IRepositoriesClient>();
var mockRelease = new Mock<IReleasesClient>();
var mockCommit = new Mock<IRepositoryCommitsClient>();

service.MockGitHubClient.Setup(x => x.Repository).Returns(mockRepo.Object);
mockRepo.Setup(x => x.Release).Returns(mockRelease.Object);
mockRepo.Setup(x => x.Commit).Returns(mockCommit.Object);

var releaseNew = new Release(
TestData.DUMMY_URL, TestData.DUMMY_URL, TestData.DUMMY_URL, TestData.DUMMY_URL, 1, "nodeId", TestData.NEW_VERSION,
"master", TestData.NEW_VERSION, TestData.BODY, false, false, DateTimeOffset.Now,
DateTimeOffset.Now, new Author(), TestData.DUMMY_URL, TestData.DUMMY_URL, null
);

var releaseCurrent = new Release(
TestData.DUMMY_URL, TestData.DUMMY_URL, TestData.DUMMY_URL, TestData.DUMMY_URL, 2, "nodeId", TestData.VERSION,
"master", TestData.VERSION, TestData.BODY, false, false, DateTimeOffset.Now,
DateTimeOffset.Now, new Author(), TestData.DUMMY_URL, TestData.DUMMY_URL, null
);

mockRelease.Setup(x => x.GetAll(TestData.GITHUB_OWNER, TestData.GITHUB_REPO, It.IsAny<ApiOptions>()))
.ReturnsAsync(new List<Release> { releaseNew, releaseCurrent });

// Construct GitHubCommitFile with necessary patch content
// GitHubCommitFile(filename, additions, deletions, changes, status, blobUrl, contentsUrl, rawUrl, sha, patch, previousFileName)
var commitFile = new GitHubCommitFile("filename", 0, 0, 0, "status", TestData.DUMMY_URL, TestData.DUMMY_URL, TestData.DUMMY_URL, "sha", TestData.PATCH_CONTENT, null);

// Construct CompareResult
// CompareResult(url, htmlUrl, permalinkUrl, diffUrl, patchUrl, baseCommit, mergeBaseCommit, status, aheadBy, behindBy, totalCommits, commits, files)
var compareResult = new CompareResult(
TestData.DUMMY_URL, TestData.DUMMY_URL, TestData.DUMMY_URL, TestData.DUMMY_URL, TestData.DUMMY_URL,
null, // baseCommit
null, // mergeBaseCommit
"ahead", // status
1, // aheadBy
0, // behindBy
1, // totalCommits
new List<GitHubCommit>(), // commits
new List<GitHubCommitFile> { commitFile } // files
);

mockCommit.Setup(x => x.Compare(TestData.GITHUB_OWNER, TestData.GITHUB_REPO, It.IsAny<string>(), It.IsAny<string>()))
.ReturnsAsync(compareResult);

// Act
var result = await service.GetNewerVersions(app, []);

// Assert
_aiService.Verify(x => x.AnalyzeDiff(It.IsAny<string>()), Times.AtLeast(3));
Assert.Single(result);
Assert.Equal(TestData.SAFE_ANALYSIS, result.First().SecurityAnalysis);
}
}
6 changes: 6 additions & 0 deletions PatchPanda.Units/TestData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,10 @@ public static class TestData
public const string RELEASE_TAG_A = "v1.0.0";
public const string RELEASE_TAG_B = "v2.0.0";
public const string ALPINE_IMAGE = "alpine:3.16";

public const string SAFE_ANALYSIS = "Safe";
public const string AI_ERROR = "AI Error";
public const string PATCH_CONTENT = "patch_content";
public const string DUMMY_URL = "http://dummy.url";
public const string BODY = "body";
}
114 changes: 71 additions & 43 deletions PatchPanda.Web/Services/VersionService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ IAiService aiService
_aiService = aiService;
}

private GitHubClient GetClient()
protected virtual IGitHubClient GetClient()
{
var client = new GitHubClient(new ProductHeaderValue("PatchPanda"));

Expand Down Expand Up @@ -174,58 +174,86 @@ await db.AppSettings.FirstOrDefaultAsync(x =>

if (securityScanningEnabled && _aiService.IsInitialized())
{
try
{
var client = GetClient();
// Resolve the correct tag for the current version to ensure comparison works
// We extract the semantic version portion from the app version and use it to find the source tag
var adjustedRegex = app.GitHubVersionRegex.TrimStart('^', 'v').TrimEnd('$');
var versionMatch = Regex.Match(app.Version, adjustedRegex);
Comment on lines +177 to +180
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

TrimStart('^', 'v') may over-trim regexes starting with v-words.

TrimStart removes all leading characters present in the char set, not just the first occurrence. A regex like "^version\\d+" would become "ersion\\d+" because both '^' and 'v' are stripped greedily. If such patterns are possible, consider using explicit prefix removal instead.

Safer prefix removal 
-                var adjustedRegex = app.GitHubVersionRegex.TrimStart('^', 'v').TrimEnd('$');
+                var adjustedRegex = app.GitHubVersionRegex;
+                if (adjustedRegex.StartsWith('^'))
+                    adjustedRegex = adjustedRegex[1..];
+                if (adjustedRegex.StartsWith('v'))
+                    adjustedRegex = adjustedRegex[1..];
+                if (adjustedRegex.EndsWith('$'))
+                    adjustedRegex = adjustedRegex[..^1];
📝 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.

Suggested change
// Resolve the correct tag for the current version to ensure comparison works
// We extract the semantic version portion from the app version and use it to find the source tag
var adjustedRegex = app.GitHubVersionRegex.TrimStart('^', 'v').TrimEnd('$');
var versionMatch = Regex.Match(app.Version, adjustedRegex);
// Resolve the correct tag for the current version to ensure comparison works
// We extract the semantic version portion from the app version and use it to find the source tag
var adjustedRegex = app.GitHubVersionRegex;
if (adjustedRegex.StartsWith('^'))
adjustedRegex = adjustedRegex[1..];
if (adjustedRegex.StartsWith('v'))
adjustedRegex = adjustedRegex[1..];
if (adjustedRegex.EndsWith('$'))
adjustedRegex = adjustedRegex[..^1];
var versionMatch = Regex.Match(app.Version, adjustedRegex);
🤖 Prompt for AI Agents 
In `@PatchPanda.Web/Services/VersionService.cs` around lines 177 - 180, The
current logic uses app.GitHubVersionRegex.TrimStart('^','v').TrimEnd('$') which
can remove multiple leading characters (e.g., an initial "v" inside a word) and
over-trim; update the transformation that builds adjustedRegex (used by
Regex.Match) to remove only a single leading '^' if present, then only a single
leading 'v' if present, and only a single trailing '$' if present (use explicit
StartsWith/EndsWith checks or equivalent string slicing) so the semantic pattern
inside app.GitHubVersionRegex is preserved.


// Resolve the correct tag for the current version to ensure comparison works
// We extract the semantic version portion from the app version and use it to find the source tag
var adjustedRegex = app.GitHubVersionRegex.TrimStart('^', 'v').TrimEnd('$');
var versionMatch = Regex.Match(app.Version, adjustedRegex);
if (versionMatch.Success)
{
var currentRelease = allReleases.FirstOrDefault(r =>
r.TagName is not null
&& Regex.IsMatch(r.TagName, Regex.Escape(versionMatch.Value))
);
Comment on lines +184 to +187
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Bug: Substring match can select the wrong release.

Regex.IsMatch(r.TagName, Regex.Escape(versionMatch.Value)) performs a contains match. For example, if versionMatch.Value is "1.0.0", a tag like "v21.0.0" would also match because "21.0.0" contains the substring "1.0.0". This could silently pick the wrong base tag for the security diff.

Anchor the pattern or use simple string matching:

Proposed fix: anchor the escaped pattern 
                     var currentRelease = allReleases.FirstOrDefault(r =>
                         r.TagName is not null
-                        && Regex.IsMatch(r.TagName, Regex.Escape(versionMatch.Value))
+                        && Regex.IsMatch(r.TagName, $@"(^|[^0-9]){Regex.Escape(versionMatch.Value)}($|[^0-9])")
                     );

Alternatively, a simpler non-regex approach:

                     var currentRelease = allReleases.FirstOrDefault(r =>
-                        r.TagName is not null
-                        && Regex.IsMatch(r.TagName, Regex.Escape(versionMatch.Value))
+                        r.TagName is not null
+                        && r.TagName.TrimStart('v') == versionMatch.Value
                     );
📝 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.

Suggested change
var currentRelease = allReleases.FirstOrDefault(r =>
r.TagName is not null
&& Regex.IsMatch(r.TagName, Regex.Escape(versionMatch.Value))
);
var currentRelease = allReleases.FirstOrDefault(r =>
r.TagName is not null
&& Regex.IsMatch(r.TagName, $@"(^|[^0-9]){Regex.Escape(versionMatch.Value)}($|[^0-9])")
);
Suggested change
var currentRelease = allReleases.FirstOrDefault(r =>
r.TagName is not null
&& Regex.IsMatch(r.TagName, Regex.Escape(versionMatch.Value))
);
var currentRelease = allReleases.FirstOrDefault(r =>
r.TagName is not null
&& r.TagName.TrimStart('v') == versionMatch.Value
);
🤖 Prompt for AI Agents 
In `@PatchPanda.Web/Services/VersionService.cs` around lines 184 - 187, The
substring regex allows false positives (e.g., "v21.0.0" matching "1.0.0");
update the selection so it matches the whole tag instead of containing the
version. Replace the predicate used to compute currentRelease (the lambda over
allReleases and r.TagName) to either compare strings exactly (r.TagName ==
versionMatch.Value or string.Equals) or anchor the escaped regex like
Regex.IsMatch(r.TagName, "^" + Regex.Escape(versionMatch.Value) + "$") so only
exact tag matches are selected.


if (versionMatch.Success)
if (currentRelease != null)
{
var currentRelease = allReleases.FirstOrDefault(r =>
r.TagName is not null
&& Regex.IsMatch(r.TagName, Regex.Escape(versionMatch.Value))
);
var baseTag = currentRelease.TagName;

if (currentRelease != null)
for (int i = 1; i <= Constants.Limits.MAX_OLLAMA_ATTEMPTS; i++)
{
var baseTag = currentRelease.TagName;

// Get the difference between the current version and the new version
var diff = await client.Repository.Commit.Compare(
repo.Item1,
repo.Item2,
baseTag,
notSeenNewVersion.VersionNumber
);

var textToAnalyze = string.Concat(
diff.Files.Select(f => f.Patch ?? "")
);

var analysis = await _aiService.AnalyzeDiff(textToAnalyze);

if (analysis is not null)
try
{
var client = GetClient();

// Get the difference between the current version and the new version
var diff = await client.Repository.Commit.Compare(
repo.Item1,
repo.Item2,
baseTag,
notSeenNewVersion.VersionNumber
);

var textToAnalyze = string.Concat(
diff.Files.Select(f => f.Patch ?? "")
);
Comment on lines +193 to +209
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

GitHub API call is needlessly inside the retry loop, wasting rate limit.

The Compare call to GitHub only needs to succeed once. If the AI analysis (line 211) is the flaky part, the diff retrieval should be hoisted above the loop so it is only called once. Each redundant Compare call consumes a GitHub API request, bringing you closer to rate limits.

Proposed restructuring 
                     var baseTag = currentRelease.TagName;
+                    var client = GetClient();
+                    var diff = await client.Repository.Commit.Compare(
+                        repo.Item1,
+                        repo.Item2,
+                        baseTag,
+                        notSeenNewVersion.VersionNumber
+                    );
+                    var textToAnalyze = string.Concat(
+                        diff.Files.Select(f => f.Patch ?? "")
+                    );

                     for (int i = 1; i <= Constants.Limits.MAX_OLLAMA_ATTEMPTS; i++)
                     {
                         try
                         {
-                            var client = GetClient();
-
-                            var diff = await client.Repository.Commit.Compare(
-                                repo.Item1,
-                                repo.Item2,
-                                baseTag,
-                                notSeenNewVersion.VersionNumber
-                            );
-
-                            var textToAnalyze = string.Concat(
-                                diff.Files.Select(f => f.Patch ?? "")
-                            );
-
                             var analysis = await _aiService.AnalyzeDiff(textToAnalyze);
🤖 Prompt for AI Agents 
In `@PatchPanda.Web/Services/VersionService.cs` around lines 193 - 209, The GitHub
Compare call (client.Repository.Commit.Compare) is being invoked on every retry
inside the loop (for i = 1..Constants.Limits.MAX_OLLAMA_ATTEMPTS) which wastes
API rate limits; move the call that computes diff (the call to
client.Repository.Commit.Compare and the construction of textToAnalyze from
diff.Files.Select(...)) out of the retry loop so it runs once before the retry
attempts, then inside the loop only retry the flaky AI analysis using
textToAnalyze and not make additional Compare calls; locate GetClient(),
client.Repository.Commit.Compare, textToAnalyze, MAX_OLLAMA_ATTEMPTS and
notSeenNewVersion to implement this change.


var analysis = await _aiService.AnalyzeDiff(textToAnalyze);

if (analysis is not null)
{
notSeenNewVersion.SecurityAnalysis = analysis.Analysis;
notSeenNewVersion.IsSuspectedMalicious =
analysis.IsSuspectedMalicious;
break;
}
else
{
_logger.LogWarning(
"Attempting to perform security scan for {Repo} version {Version}, attempt {Count} out of {Max} resulted in null analysis.",
$"{repo.Item1}/{repo.Item2}",
notSeenNewVersion.VersionNumber,
i,
Constants.Limits.MAX_OLLAMA_ATTEMPTS
);
}
}
catch (Exception ex)
{
notSeenNewVersion.SecurityAnalysis = analysis.Analysis;
notSeenNewVersion.IsSuspectedMalicious =
analysis.IsSuspectedMalicious;
if (i == Constants.Limits.MAX_OLLAMA_ATTEMPTS)
{
_logger.LogError(
ex,
"Failed to perform security scan for {Repo} version {Version}",
$"{repo.Item1}/{repo.Item2}",
notSeenNewVersion.VersionNumber
);
}
else
{
_logger.LogWarning(
ex,
"Attempting to perform security scan for {Repo} version {Version}, attempt {Count} out of {Max} failed.",
$"{repo.Item1}/{repo.Item2}",
notSeenNewVersion.VersionNumber,
i,
Constants.Limits.MAX_OLLAMA_ATTEMPTS
);
}
}
}
Comment on lines +193 to 254
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

No delay between retry attempts.

Immediate retries against the AI service are likely to hit the same transient failure. Adding a small delay (e.g., exponential backoff or a fixed pause) would improve reliability.

Example: add a short delay on failure 
                         catch (Exception ex)
                         {
                             if (i == Constants.Limits.MAX_OLLAMA_ATTEMPTS)
                             {
                                 _logger.LogError(
                                     ex,
                                     "Failed to perform security scan for {Repo} version {Version}",
                                     $"{repo.Item1}/{repo.Item2}",
                                     notSeenNewVersion.VersionNumber
                                 );
                             }
                             else
                             {
                                 _logger.LogWarning(
                                     ex,
                                     "Attempting to perform security scan for {Repo} version {Version}, attempt {Count} out of {Max} failed.",
                                     $"{repo.Item1}/{repo.Item2}",
                                     notSeenNewVersion.VersionNumber,
                                     i,
                                     Constants.Limits.MAX_OLLAMA_ATTEMPTS
                                 );
+                                await Task.Delay(TimeSpan.FromSeconds(i * 2));
                             }
                         }
🤖 Prompt for AI Agents 
In `@PatchPanda.Web/Services/VersionService.cs` around lines 193 - 254, The retry
loop in VersionService (the for loop that calls GetClient() and
_aiService.AnalyzeDiff for notSeenNewVersion) has no pause between attempts; add
a small delay between retries (e.g., Task.Delay with exponential backoff or a
fixed delay based on the attempt index i) inside the catch and/or after a null
analysis before the next iteration so transient failures are less likely to
repeat immediately—use the existing loop variable i to compute backoff (for
example delay = TimeSpan.FromSeconds(Math.Pow(2, i)) or a fixed ms) and await
the delay before continuing to the next attempt.

}
}
catch (Exception ex)
{
_logger.LogError(
ex,
"Failed to perform security scan for {Repo} version {Version}",
$"{repo.Item1}/{repo.Item2}",
notSeenNewVersion.VersionNumber
);
}
}

if (_aiService.IsInitialized())
Expand Down