better repo matching for issue file path (#208)

src/Runner.Worker/Handlers/OutputManager.cs

@@ -18,44 +18,46 @@ namespace GitHub.Runner.Worker.Handlers
         private static readonly Regex _colorCodeRegex = new Regex(@"\x0033\[[0-9;]*m?", RegexOptions.Compiled | RegexOptions.CultureInvariant);
         private readonly IActionCommandManager _commandManager;
         private readonly IExecutionContext _executionContext;
+        private readonly int _failsafe = 50;
         private readonly object _matchersLock = new object();
         private readonly TimeSpan _timeout;
         private IssueMatcher[] _matchers = Array.Empty<IssueMatcher>();
+        // Mapping that indicates whether a directory belongs to the workflow repository
+        private readonly Dictionary<string, string> _directoryMap = new Dictionary<string, string>();
 
         public OutputManager(IExecutionContext executionContext, IActionCommandManager commandManager)
         {
-            //executionContext.Debug("ENTERING OutputManager ctor");
             _executionContext = executionContext;
             _commandManager = commandManager;
 
-            //_executionContext.Debug("OutputManager ctor - determine timeout from variable");
+            // Recursion failsafe (test override)
+            var failsafeString = Environment.GetEnvironmentVariable("RUNNER_TEST_GET_REPOSITORY_PATH_FAILSAFE");
+            if (!string.IsNullOrEmpty(failsafeString))
+            {
+                _failsafe = int.Parse(failsafeString, NumberStyles.None);
+            }
+
             // Determine the timeout
             var timeoutStr = _executionContext.Variables.Get(_timeoutKey);
             if (string.IsNullOrEmpty(timeoutStr) ||
                 !TimeSpan.TryParse(timeoutStr, CultureInfo.InvariantCulture, out _timeout) ||
                 _timeout <= TimeSpan.Zero)
             {
-                //_executionContext.Debug("OutputManager ctor - determine timeout from env var");
                 timeoutStr = Environment.GetEnvironmentVariable(_timeoutKey);
                 if (string.IsNullOrEmpty(timeoutStr) ||
                     !TimeSpan.TryParse(timeoutStr, CultureInfo.InvariantCulture, out _timeout) ||
                     _timeout <= TimeSpan.Zero)
                 {
-                    //_executionContext.Debug("OutputManager ctor - set timeout to default");
                     _timeout = TimeSpan.FromSeconds(1);
                 }
             }
 
-            //_executionContext.Debug("OutputManager ctor - adding matchers");
             // Lock
             lock (_matchersLock)
             {
-                //_executionContext.Debug("OutputManager ctor - adding OnMatcherChanged");
                 _executionContext.Add(OnMatcherChanged);
-                //_executionContext.Debug("OutputManager ctor - getting matchers");
                 _matchers = _executionContext.GetMatchers().Select(x => new IssueMatcher(x, _timeout)).ToArray();
             }
-            //_executionContext.Debug("LEAVING OutputManager ctor");
         }
 
         public void Dispose()
@@ -71,7 +73,6 @@ namespace GitHub.Runner.Worker.Handlers
 
         public void OnDataReceived(object sender, ProcessDataReceivedEventArgs e)
         {
-            //_executionContext.Debug("ENTERING OutputManager OnDataReceived");
             var line = e.Data;
 
             // ## commands
@@ -82,7 +83,6 @@ namespace GitHub.Runner.Worker.Handlers
                 // The logging queues and command handlers are thread-safe.
                 if (_commandManager.TryProcessCommand(_executionContext, line))
                 {
-                    //_executionContext.Debug("LEAVING OutputManager OnDataReceived - command processed");
                     return;
                 }
             }
@@ -142,7 +142,6 @@ namespace GitHub.Runner.Worker.Handlers
                             // Log issue
                             _executionContext.AddIssue(issue, stripped);
 
-                            //_executionContext.Debug("LEAVING OutputManager OnDataReceived - issue logged");
                             return;
                         }
                     }
@@ -151,7 +150,6 @@ namespace GitHub.Runner.Worker.Handlers
 
             // Regular output
             _executionContext.Output(line);
-            //_executionContext.Debug("LEAVING OutputManager OnDataReceived");
         }
 
         private void OnMatcherChanged(object sender, MatcherChangedEventArgs e)
@@ -261,7 +259,7 @@ namespace GitHub.Runner.Worker.Handlers
                     var file = match.File;
 
                     // Root using fromPath
-                    if (!string.IsNullOrWhiteSpace(match.FromPath) && !Path.IsPathRooted(file))
+                    if (!string.IsNullOrWhiteSpace(match.FromPath) && !Path.IsPathFullyQualified(file))
                     {
                         var fromDirectory = Path.GetDirectoryName(match.FromPath);
                         if (!string.IsNullOrWhiteSpace(fromDirectory))
@@ -271,7 +269,7 @@ namespace GitHub.Runner.Worker.Handlers
                     }
 
                     // Root using workspace
-                    if (!Path.IsPathRooted(file))
+                    if (!Path.IsPathFullyQualified(file))
                     {
                         var workspace = _executionContext.GetGitHubContext("workspace");
                         ArgUtil.NotNullOrEmpty(workspace, "workspace");
@@ -279,31 +277,27 @@ namespace GitHub.Runner.Worker.Handlers
                         file = Path.Combine(workspace, file);
                     }
 
-                    // Normalize slashes
+                    // Remove relative pathing and normalize slashes
-                    file = file.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar);
+                    file = Path.GetFullPath(file);
 
-                    // File exists
+                    // Check whether the file exists
                     if (File.Exists(file))
                     {
-                        // Repository path
+                        // Check whether the file is under the workflow repository
-                        var repositoryPath = _executionContext.GetGitHubContext("workspace");
+                        var repositoryPath = GetRepositoryPath(file);
-                        ArgUtil.NotNullOrEmpty(repositoryPath, nameof(repositoryPath));
+                        if (!string.IsNullOrEmpty(repositoryPath))
-
-                        // Normalize slashes
-                        repositoryPath = repositoryPath.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar).TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar;
-
-                        if (!file.StartsWith(repositoryPath, IOUtil.FilePathStringComparison))
                         {
-                            // File is not under repo
+                            // Get the relative file path
-                            _executionContext.Debug($"Dropping file value '{file}'. Path is not under the repo.");
+                            var relativePath = file.Substring(repositoryPath.Length).TrimStart(Path.DirectorySeparatorChar);
+
+                            // Prefer `/` on all platforms
+                            issue.Data["file"] = relativePath.Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);
                         }
                         else
                         {
-                            // Prefer `/` on all platforms
+                            _executionContext.Debug($"Dropping file value '{file}'. Path is not under the workflow repo.");
-                            issue.Data["file"] = file.Substring(repositoryPath.Length).TrimStart(Path.DirectorySeparatorChar).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);
                         }
                     }
-                    // File does not exist
                     else
                     {
                         _executionContext.Debug($"Dropping file value '{file}'. Path does not exist");
@@ -317,5 +311,60 @@ namespace GitHub.Runner.Worker.Handlers
 
             return issue;
         }
+
+        private string GetRepositoryPath(string filePath, int recursion = 0)
+        {
+            // Prevent the cache from growing too much
+            if (_directoryMap.Count > 100)
+            {
+                _directoryMap.Clear();
+            }
+
+            // Empty directory means we hit the root of the drive
+            var directoryPath = Path.GetDirectoryName(filePath);
+            if (string.IsNullOrEmpty(directoryPath) || recursion > _failsafe)
+            {
+                return null;
+            }
+
+            // Check the cache
+            if (_directoryMap.TryGetValue(directoryPath, out string repositoryPath))
+            {
+                return repositoryPath;
+            }
+
+            try
+            {
+                // Check if .git/config exists
+                var gitConfigPath = Path.Combine(directoryPath, ".git", "config");
+                if (File.Exists(gitConfigPath))
+                {
+                    // Check if the config contains the workflow repository url
+                    var qualifiedRepository = _executionContext.GetGitHubContext("repository");
+                    var configMatch = $"url = https://github.com/{qualifiedRepository}";
+                    var content = File.ReadAllText(gitConfigPath);
+                    foreach (var line in content.Split("\n").Select(x => x.Trim()))
+                    {
+                        if (String.Equals(line, configMatch, StringComparison.OrdinalIgnoreCase))
+                        {
+                            repositoryPath = directoryPath;
+                            break;
+                        }
+                    }
+                }
+                else
+                {
+                    // Recursive call
+                    repositoryPath = GetRepositoryPath(directoryPath, recursion + 1);
+                }
+            }
+            catch (Exception ex)
+            {
+                _executionContext.Debug($"Error when attempting to determine whether the path '{filePath}' is under the workflow repository: {ex.Message}");
+            }
+
+            _directoryMap[directoryPath] = repositoryPath;
+            return repositoryPath;
+        }
     }
 }

src/Test/L0/Worker/OutputManagerL0.cs

@@ -3,6 +3,8 @@ using System.Collections.Generic;
 using System.Globalization;
 using System.IO;
 using System.Linq;
+using System.Threading;
+using System.Threading.Tasks;
 using System.Runtime.CompilerServices;
 using GitHub.Runner.Sdk;
 using GitHub.Runner.Worker;
@@ -644,8 +646,9 @@ namespace GitHub.Runner.Common.Tests.Worker
         [Fact]
         [Trait("Level", "L0")]
         [Trait("Category", "Worker")]
-        public void MatcherFile()
+        public async void MatcherFile()
         {
+            Environment.SetEnvironmentVariable("RUNNER_TEST_GET_REPOSITORY_PATH_FAILSAFE", "2");
             var matchers = new IssueMatchersConfig
             {
                 Matchers =
@@ -668,142 +671,87 @@ namespace GitHub.Runner.Common.Tests.Worker
             using (var hostContext = Setup(matchers: matchers))
             using (_outputManager)
             {
-                // Setup github.workspace
+                // Setup github.workspace, github.repository
                 var workDirectory = hostContext.GetDirectory(WellKnownDirectory.Work);
                 ArgUtil.NotNullOrEmpty(workDirectory, nameof(workDirectory));
                 Directory.CreateDirectory(workDirectory);
                 var workspaceDirectory = Path.Combine(workDirectory, "workspace");
                 Directory.CreateDirectory(workspaceDirectory);
                 _executionContext.Setup(x => x.GetGitHubContext("workspace")).Returns(workspaceDirectory);
+                _executionContext.Setup(x => x.GetGitHubContext("repository")).Returns("my-org/workflow-repo");
 
-                // Create a test file
+                // Setup some git repositories
-                Directory.CreateDirectory(Path.Combine(workspaceDirectory, "some-directory"));
+                // <WORKSPACE>/workflow-repo
-                var filePath = Path.Combine(workspaceDirectory, "some-directory", "some-file.txt");
+                // <WORKSPACE>/workflow-repo/nested-other-repo
-                File.WriteAllText(filePath, "");
+                // <WORKSPACE>/other-repo
-
+                // <WORKSPACE>/other-repo/nested-workflow-repo
-                // Process
+                var workflowRepository = Path.Combine(workspaceDirectory, "workflow-repo");
-                Process("some-directory/some-file.txt: some error");
+                var nestedOtherRepository = Path.Combine(workspaceDirectory, "workflow-repo", "nested-other-repo");
-                Assert.Equal(1, _issues.Count);
+                var otherRepository = Path.Combine(workspaceDirectory, workflowRepository, "nested-other-repo");
-                Assert.Equal("some error", _issues[0].Item1.Message);
+                var nestedWorkflowRepository = Path.Combine(workspaceDirectory, "other-repo", "nested-workflow-repo");
-                Assert.Equal("some-directory/some-file.txt", _issues[0].Item1.Data["file"]);
+                await CreateRepository(hostContext, workflowRepository, "https://github.com/my-org/workflow-repo");
-                Assert.Equal(0, _commands.Count);
+                await CreateRepository(hostContext, nestedOtherRepository, "https://github.com/my-org/other-repo");
-                Assert.Equal(0, _messages.Count);
+                await CreateRepository(hostContext, otherRepository, "https://github.com/my-org/other-repo");
-            }
+                await CreateRepository(hostContext, nestedWorkflowRepository, "https://github.com/my-org/workflow-repo");
-        }
-
-        [Fact]
-        [Trait("Level", "L0")]
-        [Trait("Category", "Worker")]
-        public void MatcherFileExists()
-        {
-            var matchers = new IssueMatchersConfig
-            {
-                Matchers =
-                {
-                    new IssueMatcherConfig
-                    {
-                        Owner = "my-matcher-1",
-                        Patterns = new[]
-                        {
-                            new IssuePatternConfig
-                            {
-                                Pattern = @"(.+): (.+)",
-                                File = 1,
-                                Message = 2,
-                            },
-                        },
-                    },
-                },
-            };
-            using (var hostContext = Setup(matchers: matchers))
-            using (_outputManager)
-            {
-                // Setup github.workspace
-                var workDirectory = hostContext.GetDirectory(WellKnownDirectory.Work);
-                ArgUtil.NotNullOrEmpty(workDirectory, nameof(workDirectory));
-                Directory.CreateDirectory(workDirectory);
-                var workspaceDirectory = Path.Combine(workDirectory, "workspace");
-                Directory.CreateDirectory(workspaceDirectory);
-                _executionContext.Setup(x => x.GetGitHubContext("workspace")).Returns(workspaceDirectory);
-
-                // Create a test file
-                File.WriteAllText(Path.Combine(workspaceDirectory, "some-file-1.txt"), "");
-
-                // Process
-                Process("some-file-1.txt: some error 1"); // file exists
-                Process("some-file-2.txt: some error 2"); // file does not exist
-
-                Assert.Equal(2, _issues.Count);
-                Assert.Equal("some error 1", _issues[0].Item1.Message);
-                Assert.Equal("some-file-1.txt", _issues[0].Item1.Data["file"]);
-                Assert.Equal("some error 2", _issues[1].Item1.Message);
-                Assert.False(_issues[1].Item1.Data.ContainsKey("file")); // does not contain file key
-                Assert.Equal(0, _commands.Count);
-                Assert.Equal(0, _messages.Where(x => !x.StartsWith("##[debug]")).Count());
-            }
-        }
-
-        [Fact]
-        [Trait("Level", "L0")]
-        [Trait("Category", "Worker")]
-        public void MatcherFileOutsideRepository()
-        {
-            var matchers = new IssueMatchersConfig
-            {
-                Matchers =
-                {
-                    new IssueMatcherConfig
-                    {
-                        Owner = "my-matcher-1",
-                        Patterns = new[]
-                        {
-                            new IssuePatternConfig
-                            {
-                                Pattern = @"(.+): (.+)",
-                                File = 1,
-                                Message = 2,
-                            },
-                        },
-                    },
-                },
-            };
-            using (var hostContext = Setup(matchers: matchers))
-            using (_outputManager)
-            {
-                // Setup github.workspace
-                var workDirectory = hostContext.GetDirectory(WellKnownDirectory.Work);
-                ArgUtil.NotNullOrEmpty(workDirectory, nameof(workDirectory));
-                Directory.CreateDirectory(workDirectory);
-                var workspaceDirectory = Path.Combine(workDirectory, "workspace");
-                Directory.CreateDirectory(workspaceDirectory);
-                _executionContext.Setup(x => x.GetGitHubContext("workspace")).Returns(workspaceDirectory);
 
                 // Create test files
-                var filePath1 = Path.Combine(workspaceDirectory, "some-file-1.txt");
+                var file_noRepository = Path.Combine(workspaceDirectory, "no-repo.txt");
-                File.WriteAllText(filePath1, "");
+                var file_workflowRepository = Path.Combine(workflowRepository, "workflow-repo.txt");
-                var workspaceSiblingDirectory = Path.Combine(Path.GetDirectoryName(workspaceDirectory), "workspace-sibling");
+                var file_workflowRepository_nestedDirectory = Path.Combine(workflowRepository, "subdir", "subdir2", "workflow-repo-nested-dir.txt");
-                Directory.CreateDirectory(workspaceSiblingDirectory);
+                var file_workflowRepository_failsafe = Path.Combine(workflowRepository, "failsafe-subdir", "failsafe-subdir2", "failsafe-subdir3", "workflow-repo-failsafe.txt");
-                var filePath2 = Path.Combine(workspaceSiblingDirectory, "some-file-2.txt");
+                var file_nestedOtherRepository = Path.Combine(nestedOtherRepository, "nested-other-repo");
-                File.WriteAllText(filePath2, "");
+                var file_otherRepository = Path.Combine(otherRepository, "other-repo.txt");
+                var file_nestedWorkflowRepository = Path.Combine(nestedWorkflowRepository, "nested-workflow-repo.txt");
+                foreach (var file in new[] { file_noRepository, file_workflowRepository, file_workflowRepository_nestedDirectory, file_workflowRepository_failsafe, file_nestedOtherRepository, file_otherRepository, file_nestedWorkflowRepository })
+                {
+                    Directory.CreateDirectory(Path.GetDirectoryName(file));
+                    File.WriteAllText(file, "");
+                }
 
                 // Process
-                Process($"{filePath1}: some error 1"); // file exists inside workspace
+                Process($"{file_noRepository}: some error 1");
-                Process($"{filePath2}: some error 2"); // file exists outside workspace
+                Process($"{file_workflowRepository}: some error 2");
+                Process($"{file_workflowRepository.Substring(workspaceDirectory.Length + 1)}: some error 3"); // Relative path from workspace dir
+                Process($"{file_workflowRepository_nestedDirectory}: some error 4");
+                Process($"{file_workflowRepository_failsafe}: some error 5");
+                Process($"{file_nestedOtherRepository}: some error 6");
+                Process($"{file_otherRepository}: some error 7");
+                Process($"{file_nestedWorkflowRepository}: some error 8");
+
+                Assert.Equal(8, _issues.Count);
 
-                Assert.Equal(2, _issues.Count);
                 Assert.Equal("some error 1", _issues[0].Item1.Message);
-                Assert.Equal("some-file-1.txt", _issues[0].Item1.Data["file"]);
+                Assert.False(_issues[0].Item1.Data.ContainsKey("file"));
+
                 Assert.Equal("some error 2", _issues[1].Item1.Message);
-                Assert.False(_issues[1].Item1.Data.ContainsKey("file")); // does not contain file key
+                Assert.Equal(file_workflowRepository.Substring(workflowRepository.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[1].Item1.Data["file"]);
-                Assert.Equal(0, _commands.Count);
+
-                Assert.Equal(0, _messages.Where(x => !x.StartsWith("##[debug]")).Count());
+                Assert.Equal("some error 3", _issues[2].Item1.Message);
+                Assert.Equal(file_workflowRepository.Substring(workflowRepository.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[2].Item1.Data["file"]);
+
+                Assert.Equal("some error 4", _issues[3].Item1.Message);
+                Assert.Equal(file_workflowRepository_nestedDirectory.Substring(workflowRepository.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[3].Item1.Data["file"]);
+
+                Assert.Equal("some error 5", _issues[4].Item1.Message);
+                Assert.False(_issues[4].Item1.Data.ContainsKey("file"));
+
+                Assert.Equal("some error 6", _issues[5].Item1.Message);
+                Assert.False(_issues[5].Item1.Data.ContainsKey("file"));
+
+                Assert.Equal("some error 7", _issues[6].Item1.Message);
+                Assert.False(_issues[6].Item1.Data.ContainsKey("file"));
+
+                Assert.Equal("some error 8", _issues[7].Item1.Message);
+                Assert.Equal(file_nestedWorkflowRepository.Substring(nestedWorkflowRepository.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[7].Item1.Data["file"]);
             }
+
+            Environment.SetEnvironmentVariable("RUNNER_TEST_GET_REPOSITORY_PATH_FAILSAFE", "");
         }
 
         [Fact]
         [Trait("Level", "L0")]
         [Trait("Category", "Worker")]
-        public void MatcherFromPath()
+        public async void MatcherFromPath()
         {
             var matchers = new IssueMatchersConfig
             {
@@ -828,22 +776,26 @@ namespace GitHub.Runner.Common.Tests.Worker
             using (var hostContext = Setup(matchers: matchers))
             using (_outputManager)
             {
-                // Setup github.workspace
+                // Setup github.workspace, github.repository
                 var workDirectory = hostContext.GetDirectory(WellKnownDirectory.Work);
                 ArgUtil.NotNullOrEmpty(workDirectory, nameof(workDirectory));
                 Directory.CreateDirectory(workDirectory);
                 var workspaceDirectory = Path.Combine(workDirectory, "workspace");
                 Directory.CreateDirectory(workspaceDirectory);
                 _executionContext.Setup(x => x.GetGitHubContext("workspace")).Returns(workspaceDirectory);
+                _executionContext.Setup(x => x.GetGitHubContext("repository")).Returns("my-org/workflow-repo");
+
+                // Setup a git repository
+                var repositoryPath = Path.Combine(workspaceDirectory, "workflow-repo");
+                await CreateRepository(hostContext, repositoryPath, "https://github.com/my-org/workflow-repo");
 
                 // Create a test file
-                Directory.CreateDirectory(Path.Combine(workspaceDirectory, "some-directory"));
+                var filePath = Path.Combine(repositoryPath, "some-project", "some-directory", "some-file.txt");
-                Directory.CreateDirectory(Path.Combine(workspaceDirectory, "some-project", "some-directory"));
+                Directory.CreateDirectory(Path.GetDirectoryName(filePath));
-                var filePath = Path.Combine(workspaceDirectory, "some-project", "some-directory", "some-file.txt");
                 File.WriteAllText(filePath, "");
 
                 // Process
-                Process("some-directory/some-file.txt: some error [some-project/some-project.proj]");
+                Process("some-directory/some-file.txt: some error [workflow-repo/some-project/some-project.proj]");
                 Assert.Equal(1, _issues.Count);
                 Assert.Equal("some error", _issues[0].Item1.Message);
                 Assert.Equal("some-project/some-directory/some-file.txt", _issues[0].Item1.Data["file"]);
@@ -931,5 +883,22 @@ namespace GitHub.Runner.Common.Tests.Worker
         {
             _outputManager.OnDataReceived(null, new ProcessDataReceivedEventArgs(line));
         }
+
+        private async Task CreateRepository(TestHostContext hostConetxt, string path, string url)
+        {
+            Directory.CreateDirectory(path);
+            var gitPath = WhichUtil.Which("git", true);
+            var environment = new Dictionary<string, string>();
+
+            using (var processInvoker = new ProcessInvoker(hostConetxt.GetTrace()))
+            {
+                await processInvoker.ExecuteAsync(path, gitPath, "init", environment, CancellationToken.None);
+            }
+
+            using (var processInvoker = new ProcessInvoker(hostConetxt.GetTrace()))
+            {
+                await processInvoker.ExecuteAsync(path, gitPath, $"remote add origin {url}", environment, CancellationToken.None);
+            }
+        }
     }
 }