Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,71 @@ await File.WriteAllTextAsync(
Assert.Equal(nestedManifestId, manifest.Key);
}

/// <summary>
/// Tests that unavailable descendants do not prevent discovery in accessible sibling directories.
/// </summary>
/// <returns>A <see cref="Task"/> representing the asynchronous test operation.</returns>
[Fact]
public async Task DiscoverManifestsAsync_WithUnavailableDescendants_ContinuesDiscoveringAccessibleManifests()
{
// Arrange
const string accessibleManifestId = "1.0.genhub.mod.accessible";
var accessibleDirectory = Directory.CreateDirectory(
Path.Combine(_tempDirectory, "accessible")).FullName;
var inaccessibleDirectory = Directory.CreateDirectory(
Path.Combine(_tempDirectory, "inaccessible")).FullName;
var removedDirectory = Directory.CreateDirectory(
Path.Combine(_tempDirectory, "removed")).FullName;
var unlistableDirectory = Directory.CreateDirectory(
Path.Combine(_tempDirectory, "unlistable")).FullName;
var unreachableDirectory = Directory.CreateDirectory(
Path.Combine(unlistableDirectory, "unreachable")).FullName;
await File.WriteAllTextAsync(
Path.Combine(accessibleDirectory, "accessible.json"),
SerializeManifest(accessibleManifestId));
await File.WriteAllTextAsync(
Path.Combine(unreachableDirectory, "unreachable.json"),
SerializeManifest("1.0.genhub.mod.unreachable"));

IEnumerable<string> EnumerateFiles(string directory, string pattern)
{
if (directory == inaccessibleDirectory)
{
throw new UnauthorizedAccessException("Injected inaccessible directory.");
}

if (directory == removedDirectory)
{
throw new DirectoryNotFoundException("Injected concurrently removed directory.");
}

return Directory.EnumerateFiles(directory, pattern, SearchOption.TopDirectoryOnly);
}

IEnumerable<string> EnumerateDirectories(string directory)
{
if (directory == unlistableDirectory)
{
throw new UnauthorizedAccessException("Injected unlistable directory.");
}

return Directory.EnumerateDirectories(directory, "*", SearchOption.TopDirectoryOnly);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SUGGESTION: Test delegate should match production implementation

The test uses the old pattern with hardcoded "*" and SearchOption.TopDirectoryOnly, but the production code uses the parameterless overload Directory.EnumerateDirectories(directory) to avoid hardcoding the directory wildcard pattern. For consistency and accuracy, the test should match the production implementation.

Suggested change
return Directory.EnumerateDirectories(directory, "*", SearchOption.TopDirectoryOnly);
return Directory.EnumerateDirectories(directory);

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

}

var discoveryService = new ManifestDiscoveryService(
_loggerMock.Object,
_cacheMock.Object,
EnumerateFiles,
EnumerateDirectories);

// Act
var manifests = await discoveryService.DiscoverManifestsAsync([_tempDirectory]);

// Assert
var manifest = Assert.Single(manifests);
Assert.Equal(accessibleManifestId, manifest.Key);
}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
/// <summary>
/// Tests that ValidateDependencies returns false when a required dependency is missing.
/// </summary>
Expand Down
96 changes: 91 additions & 5 deletions GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,29 @@ namespace GenHub.Features.Manifest;
public class ManifestDiscoveryService(ILogger<ManifestDiscoveryService> logger, IManifestCache manifestCache)
{
private static readonly JsonSerializerOptions JsonOptions = new() { PropertyNameCaseInsensitive = true };
private readonly Func<string, string, IEnumerable<string>> _enumerateFiles =
(directory, pattern) => Directory.EnumerateFiles(directory, pattern, SearchOption.TopDirectoryOnly);

private readonly Func<string, IEnumerable<string>> _enumerateDirectories =
directory => Directory.EnumerateDirectories(directory);

/// <summary>
/// Initializes a new instance of the <see cref="ManifestDiscoveryService"/> class with filesystem test seams.
/// </summary>
/// <param name="logger">The logger.</param>
/// <param name="manifestCache">The manifest cache.</param>
/// <param name="enumerateFiles">The top-level file enumerator.</param>
/// <param name="enumerateDirectories">The top-level directory enumerator.</param>
internal ManifestDiscoveryService(
ILogger<ManifestDiscoveryService> logger,
IManifestCache manifestCache,
Func<string, string, IEnumerable<string>> enumerateFiles,
Func<string, IEnumerable<string>> enumerateDirectories)
: this(logger, manifestCache)
{
_enumerateFiles = enumerateFiles;
_enumerateDirectories = enumerateDirectories;
}

/// <summary>
/// Gets manifests by content type.
Expand Down Expand Up @@ -61,7 +84,10 @@ public async Task<Dictionary<string, ContentManifest>> DiscoverManifestsAsync(
foreach (var directory in searchDirectories.Where(Directory.Exists))
{
logger.LogInformation("Scanning directory for manifests: {Directory}", directory);
var manifestFiles = Directory.EnumerateFiles(directory, FileTypes.JsonFilePattern, SearchOption.AllDirectories);
var manifestFiles = EnumerateFilesSafely(
directory,
FileTypes.JsonFilePattern,
cancellationToken);
foreach (var manifestFile in manifestFiles)
{
try
Expand Down Expand Up @@ -157,6 +183,11 @@ public bool ValidateDependencies(
return true;
}

private static bool IsSkippableEnumerationException(Exception exception)
{
return exception is UnauthorizedAccessException or IOException;
}

private static bool IsVersionCompatible(string actualVersion, string minVersion, string maxVersion)
{
if (!string.IsNullOrEmpty(minVersion) && string.Compare(actualVersion, minVersion, StringComparison.OrdinalIgnoreCase) < 0)
Expand Down Expand Up @@ -184,16 +215,71 @@ private static bool IsVersionCompatible(string actualVersion, string minVersion,
return null;
}

private IEnumerable<string> EnumerateFilesSafely(
string rootDirectory,
string searchPattern,
CancellationToken cancellationToken)
{
var pendingDirectories = new Stack<string>();
pendingDirectories.Push(rootDirectory);

while (pendingDirectories.Count > 0)
{
cancellationToken.ThrowIfCancellationRequested();
var currentDirectory = pendingDirectories.Pop();

string[] files;
try
{
files = _enumerateFiles(currentDirectory, searchPattern).ToArray();
}
catch (Exception ex) when (IsSkippableEnumerationException(ex))
{
logger.LogWarning(
ex,
"Skipping files in inaccessible or unavailable manifest directory: {Directory}",
currentDirectory);
files = [];
}

foreach (var file in files)
{
cancellationToken.ThrowIfCancellationRequested();
yield return file;
}

string[] childDirectories;
try
{
childDirectories = _enumerateDirectories(currentDirectory).ToArray();
}
catch (Exception ex) when (IsSkippableEnumerationException(ex))
{
logger.LogWarning(
ex,
"Skipping inaccessible or unavailable manifest directory: {Directory}",
currentDirectory);
childDirectories = [];
}

for (var index = childDirectories.Length - 1; index >= 0; index--)
{
pendingDirectories.Push(childDirectories[index]);
}
}
}

private async Task DiscoverFileSystemManifestsAsync(IEnumerable<string> searchDirectories, CancellationToken cancellationToken)
{
foreach (var directory in searchDirectories.Where(Directory.Exists))
{
logger.LogInformation("Scanning directory for manifests: {Directory}", directory);

// Look for both .json and .manifest.json files to avoid conflicts with stored manifests
var manifestFiles = Directory.EnumerateFiles(directory, FileTypes.ManifestFilePattern, SearchOption.AllDirectories)
.Concat(Directory.EnumerateFiles(directory, "*.json", SearchOption.AllDirectories)
.Where(f => !f.EndsWith(FileTypes.ManifestFileExtension)));
// The JSON pattern includes both .json and .manifest.json files.
var manifestFiles = EnumerateFilesSafely(
directory,
FileTypes.JsonFilePattern,
cancellationToken);

foreach (var manifestFile in manifestFiles)
{
Expand Down
Loading