Fix CodeQL and SonarCloud findings; exclude generated files from CodeQL

.github/codeql-config.yml

@@ -4,8 +4,18 @@
 
 name: "Agent Control CodeQL Config"
 
+# Globally ignore build output and generated files
+paths-ignore:
+  - '**/obj/**'
+  - '**/bin/**'
+
 # Query filters to disable specific queries for certain paths
 query-filters:
+  # Suppress warnings in all generated build output under obj/ (belt-and-suspenders
+  # alongside the global paths-ignore above)
+  - exclude:
+      id: cs/missed-ternary-operator
+    paths: ['**/obj/**/*.cs']
   - exclude:
       id: cs/catch-of-all-exceptions
     paths:

docs/design/agent-control/repo-sync/package-zip-extractor.md

@@ -8,10 +8,13 @@
 four managed agent folders (`.github/agents`, `.github/standards`, `.github/templates`,
 `.github/skills`) from a package zip file. Per architecture.md's "Blind delete of the four
 known folders" decision (see `docs/design/agent-control/repo-sync.md`), it performs: (1) open/
-validate the zip; (2) delete the four known folders if present; (3) extract only those same
-four folders from the zip — root-level files such as `release-notes.md` are never extracted to
-disk. Rewriting the `.agentcontrol.json` pin is the caller's responsibility (the `RepoConfig`
-subsystem), so this class stays focused on file operations alone.
+validate the zip; (2) validate every entry's destination path up front, before anything is
+deleted; (3) delete the four known folders if present; (4) extract only the already-validated
+entries that fall within those same four folders — root-level files such as `release-notes.md`
+are never extracted to disk. Validating every entry before the delete step means an invalid or
+path-traversing entry rejects the whole upgrade without ever touching the existing managed
+folders. Rewriting the `.agentcontrol.json` pin is the caller's responsibility (the
+`RepoConfig` subsystem), so this class stays focused on file operations alone.
 
 #### Data Model
 
@@ -23,15 +26,17 @@ consulted by both `Extract` and `AllManagedFoldersExist`.
 
 #### Key Methods
 
-**Extract**: Opens/validates the zip, deletes the four managed folders if present, and
-extracts the same four folders from the zip into the repo.
+**Extract**: Opens/validates the zip, validates every entry's destination path, deletes the
+four managed folders if present, and extracts the already-validated entries into the repo.
 
 - *Parameters*: `string zipPath`, `string repoRoot`.
 - *Returns*: `void`.
 - *Preconditions*: Neither parameter `null`.
 - *Postconditions*: The four managed folders under `repoRoot` match the zip's contents exactly
-  (`AgentControl-PackageZipExtractor-Extract`). There is intentionally no rollback on failure —
-  a partially-applied change is possible and must be resolved manually by the caller, per
+  (`AgentControl-PackageZipExtractor-Extract`). An entry with an invalid destination path is
+  rejected before any managed folder is deleted, so that failure mode never leaves the repo
+  partially upgraded. A delete or extraction I/O failure can still do so — there is
+  intentionally no rollback for that case, and it must be resolved manually by the caller, per
   architecture.md's "Upgrade sequence and failure handling" decision.
 
 **AllManagedFoldersExist**: Determines whether all four managed folders currently exist under
@@ -55,11 +60,13 @@ entry without extracting it to disk.
 #### Error Handling
 
 `Extract` throws `ArgumentNullException` for a null `zipPath`/`repoRoot`, and
-`InvalidOperationException` when the zip cannot be opened/is not a valid archive, a managed
-folder cannot be deleted, or extraction fails partway through — wrapping the underlying
-`IOException`/`UnauthorizedAccessException`/`InvalidDataException` with a message naming the
-zip path and repo root. `ReadReleaseNotes` throws the same `InvalidOperationException` pattern
-for a zip that cannot be opened or whose release-notes entry cannot be read.
+`InvalidOperationException` when the zip cannot be opened/is not a valid archive, an entry's
+destination path is invalid (including resolving outside `repoRoot`), a managed folder cannot
+be deleted, or extraction fails partway through — wrapping the underlying
+`IOException`/`UnauthorizedAccessException`/`InvalidDataException`/`ArgumentException`/
+`NotSupportedException` with a message naming the zip path and repo root (or the offending
+entry). `ReadReleaseNotes` throws the same `InvalidOperationException` pattern for a zip that
+cannot be opened or whose release-notes entry cannot be read.
 
 #### Dependencies
 

docs/verification/agent-control/repo-sync/package-zip-extractor.md

@@ -16,6 +16,9 @@ N/A - standard test environment.
 - All unit tests pass with zero failures.
 - Extraction fully replaces managed folder contents and creates them when absent.
 - An invalid zip file is rejected with a clear exception rather than a partial extraction.
+- A zip entry that escapes the repo root, or resolves inside the repo root but outside every
+  managed folder, is rejected/skipped without ever writing outside a managed folder, and
+  without leaving pre-existing managed folders blind-deleted ahead of a rejected extraction.
 - Folder-presence and release-notes checks reflect genuine filesystem/archive state.
 
 #### Test Scenarios
@@ -29,6 +32,16 @@ throws `InvalidOperationException`. This scenario is tested by
 `PackageZipExtractor_Extract_InvalidZipFile_ThrowsInvalidOperationException`, covering
 `AgentControl-PackageZipExtractor-Extract`.
 
+**PackageZipExtractor_Extract_RejectsPathTraversalWithoutPartialUpgrade**: A zip entry that
+textually starts with a managed-folder prefix but uses ".." to resolve outside every managed
+folder (while staying under the repo root) is silently skipped rather than extracted. A zip
+entry that escapes the repo root entirely is rejected with `InvalidOperationException` before
+any managed folder is deleted, leaving a pre-existing managed folder's contents untouched.
+This scenario is tested by
+`PackageZipExtractor_Extract_TraversalEntryWithinRepoRoot_DoesNotEscapeManagedFolders` and
+`PackageZipExtractor_Extract_EntryEscapesRepoRoot_ThrowsBeforeDeletingManagedFolders`, covering
+`AgentControl-PackageZipExtractor-Extract`.
+
 **PackageZipExtractor_AllManagedFoldersExist_ReflectsActualPresence**: The check returns true
 when all four managed folders are present, and false when some or none are present. This
 scenario is tested by

src/DemaConsulting.AgentControl/AgentPackageManagement/PackageSource.cs

@@ -161,10 +161,10 @@ public static IReadOnlyList<string> EnumeratePackageNames(string sourceDirectory
 
         var names = new HashSet<string>(StringComparer.Ordinal);
 
-        foreach (var filePath in Directory.EnumerateFiles(sourceDirectory, "*" + ZipExtension))
+        foreach (var fileName in Directory.EnumerateFiles(sourceDirectory, "*" + ZipExtension)
+                     .Select(Path.GetFileNameWithoutExtension))
         {
-            var fileName = Path.GetFileNameWithoutExtension(filePath);
-            var name = TrySplitNameAndVersion(fileName);
+            var name = TrySplitNameAndVersion(fileName!);
             if (name is not null)
             {
                 names.Add(name);

src/DemaConsulting.AgentControl/AgentPackageManagement/PackageVersionCache.cs

@@ -46,20 +46,20 @@ internal sealed class PackageVersionCache
     ///     name. A cached <see langword="null"/> value (no package found) is a valid, distinct
     ///     cache entry from "not yet queried".
     /// </summary>
-    private readonly Dictionary<(string SourceDirectory, string PackageName), DiscoveredPackage?> _cache = new();
+    private readonly Dictionary<(string SourceDirectory, string PackageName), DiscoveredPackage?> _cache = [];
 
     /// <summary>
     ///     The cached distinct package base names discoverable at a source directory, keyed by
     ///     source directory.
     /// </summary>
-    private readonly Dictionary<string, IReadOnlyList<string>> _packageNamesCache = new();
+    private readonly Dictionary<string, IReadOnlyList<string>> _packageNamesCache = [];
 
     /// <summary>
     ///     The cached descending-by-version package list for a given <c>(sourceDirectory,
     ///     packageName)</c> pair.
     /// </summary>
     private readonly Dictionary<(string SourceDirectory, string PackageName), IReadOnlyList<DiscoveredPackage>>
-        _versionsDescendingCache = new();
+        _versionsDescendingCache = [];
 
     /// <summary>
     ///     Enumerates the distinct package base names discoverable at a source directory,

src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs

@@ -139,11 +139,18 @@ public static Process Launch(ProcessStartInfo startInfo, ILogger? logger = null)
         ArgumentNullException.ThrowIfNull(startInfo);
 
         var effectiveLogger = logger ?? AppLogging.Factory.CreateLogger(LoggerCategoryName);
-        var argumentsText = string.Join(' ', startInfo.ArgumentList);
 
-        effectiveLogger.LogDebug(
-            "Starting shell process '{FileName}' with arguments '{Arguments}' in working directory '{WorkingDirectory}'",
-            startInfo.FileName, argumentsText, startInfo.WorkingDirectory);
+        // Lazily computed so the join is performed at most once per call, and only if some
+        // enabled log statement (or the failure path) actually needs it - avoids formatting the
+        // same argument list twice when both Debug and Information logging are enabled.
+        var argumentsText = new Lazy<string>(() => string.Join(' ', startInfo.ArgumentList));
+
+        if (effectiveLogger.IsEnabled(LogLevel.Debug))
+        {
+            effectiveLogger.LogDebug(
+                "Starting shell process '{FileName}' with arguments '{Arguments}' in working directory '{WorkingDirectory}'",
+                startInfo.FileName, argumentsText.Value, startInfo.WorkingDirectory);
+        }
 
         var stopwatch = Stopwatch.StartNew();
         try
@@ -153,9 +160,12 @@ public static Process Launch(ProcessStartInfo startInfo, ILogger? logger = null)
                                $"Failed to start shell process '{startInfo.FileName}'.");
 
             stopwatch.Stop();
-            effectiveLogger.LogInformation(
-                "Started shell process '{FileName} {Arguments}' as PID {ProcessId} after {ElapsedMilliseconds}ms",
-                startInfo.FileName, argumentsText, process.Id, stopwatch.ElapsedMilliseconds);
+            if (effectiveLogger.IsEnabled(LogLevel.Information))
+            {
+                effectiveLogger.LogInformation(
+                    "Started shell process '{FileName} {Arguments}' as PID {ProcessId} after {ElapsedMilliseconds}ms",
+                    startInfo.FileName, argumentsText.Value, process.Id, stopwatch.ElapsedMilliseconds);
+            }
 
             return process;
         }
@@ -166,13 +176,16 @@ public static Process Launch(ProcessStartInfo startInfo, ILogger? logger = null)
             // NativeErrorCode is the actual OS error code behind a Win32Exception - the same
             // critical diagnostic data highlighted as missing for GitClient's intermittent
             // failures; captured here too since this is another real-process-spawning path.
-            effectiveLogger.LogError(
-                ex,
-                "Failed to start shell process '{FileName} {Arguments}' after {ElapsedMilliseconds}ms (Win32 NativeErrorCode={NativeErrorCode})",
-                startInfo.FileName, argumentsText, stopwatch.ElapsedMilliseconds, ex.NativeErrorCode);
+            if (effectiveLogger.IsEnabled(LogLevel.Error))
+            {
+                effectiveLogger.LogError(
+                    ex,
+                    "Failed to start shell process '{FileName} {Arguments}' after {ElapsedMilliseconds}ms (Win32 NativeErrorCode={NativeErrorCode})",
+                    startInfo.FileName, argumentsText.Value, stopwatch.ElapsedMilliseconds, ex.NativeErrorCode);
+            }
 
             throw new InvalidOperationException(
-                $"Failed to start shell process '{startInfo.FileName} {argumentsText}': {ex.Message}", ex);
+                $"Failed to start shell process '{startInfo.FileName} {argumentsText.Value}': {ex.Message}", ex);
         }
     }
 }

src/DemaConsulting.AgentControl/GitIntegration/CommittedAgentFilesCache.cs

@@ -40,7 +40,7 @@ internal sealed class CommittedAgentFilesCache
     ///     The cached results, keyed by a normalized repo path and its <c>HEAD</c> commit hash at
     ///     the time the result was recorded.
     /// </summary>
-    private readonly Dictionary<(string RepoPath, string HeadHash), bool> _cache = new();
+    private readonly Dictionary<(string RepoPath, string HeadHash), bool> _cache = [];
 
     /// <summary>
     ///     Attempts to retrieve a cached result for a repo at a specific <c>HEAD</c> commit hash.

src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs

@@ -362,11 +362,15 @@ private GitCommandResult RunGit(string repositoryPath, params string[] arguments
 
         // Logged before the process starts so a crash/hang mid-invocation still leaves a record
         // of exactly what was about to run and where - the key data point missing from prior
-        // occurrences of the intermittent exit-code -1 failure.
-        var argumentsText = string.Join(' ', arguments);
-        _logger.LogDebug(
-            "Starting git process '{GitExecutable}' with arguments '{Arguments}' in working directory '{WorkingDirectory}'",
-            _gitExecutablePath, argumentsText, repositoryPath);
+        // occurrences of the intermittent exit-code -1 failure. Lazily computed so the join is
+        // only ever performed once, and only if some enabled log statement actually needs it.
+        var argumentsText = new Lazy<string>(() => string.Join(' ', arguments));
+        if (_logger.IsEnabled(LogLevel.Debug))
+        {
+            _logger.LogDebug(
+                "Starting git process '{GitExecutable}' with arguments '{Arguments}' in working directory '{WorkingDirectory}'",
+                _gitExecutablePath, argumentsText.Value, repositoryPath);
+        }
 
         var stopwatch = Stopwatch.StartNew();
         try
@@ -383,25 +387,28 @@ private GitCommandResult RunGit(string repositoryPath, params string[] arguments
 
             // A long WaitForExit points at a hung/slow process rather than a fast failure -
             // exactly the ambiguity the flaky-test investigation could not previously resolve.
-            if (stopwatch.Elapsed > SlowExitWarningThreshold)
+            if (stopwatch.Elapsed > SlowExitWarningThreshold && _logger.IsEnabled(LogLevel.Warning))
             {
                 _logger.LogWarning(
                     "git process '{GitExecutable} {Arguments}' took an unexpectedly long {ElapsedMilliseconds}ms to exit",
-                    _gitExecutablePath, argumentsText, stopwatch.ElapsedMilliseconds);
+                    _gitExecutablePath, argumentsText.Value, stopwatch.ElapsedMilliseconds);
             }
 
             var stdout = stdoutTask.GetAwaiter().GetResult();
             var stderr = stderrTask.GetAwaiter().GetResult();
 
-            _logger.LogInformation(
-                "git process '{GitExecutable} {Arguments}' exited with code {ExitCode} after {ElapsedMilliseconds}ms",
-                _gitExecutablePath, argumentsText, process.ExitCode, stopwatch.ElapsedMilliseconds);
+            if (_logger.IsEnabled(LogLevel.Information))
+            {
+                _logger.LogInformation(
+                    "git process '{GitExecutable} {Arguments}' exited with code {ExitCode} after {ElapsedMilliseconds}ms",
+                    _gitExecutablePath, argumentsText.Value, process.ExitCode, stopwatch.ElapsedMilliseconds);
+            }
 
-            if (process.ExitCode != 0)
+            if (process.ExitCode != 0 && _logger.IsEnabled(LogLevel.Warning))
             {
                 _logger.LogWarning(
                     "git process '{GitExecutable} {Arguments}' failed with exit code {ExitCode}; stderr: {StandardError}",
-                    _gitExecutablePath, argumentsText, process.ExitCode, stderr);
+                    _gitExecutablePath, argumentsText.Value, process.ExitCode, stderr);
             }
 
             return new GitCommandResult(process.ExitCode, stdout, stderr);
@@ -417,13 +424,16 @@ private GitCommandResult RunGit(string repositoryPath, params string[] arguments
                 ? win32Exception.NativeErrorCode
                 : (int?)null;
 
-            _logger.LogError(
-                ex,
-                "Failed to run '{GitExecutable} {Arguments}' after {ElapsedMilliseconds}ms (Win32 NativeErrorCode={NativeErrorCode})",
-                _gitExecutablePath, argumentsText, stopwatch.ElapsedMilliseconds, nativeErrorCode);
+            if (_logger.IsEnabled(LogLevel.Error))
+            {
+                _logger.LogError(
+                    ex,
+                    "Failed to run '{GitExecutable} {Arguments}' after {ElapsedMilliseconds}ms (Win32 NativeErrorCode={NativeErrorCode})",
+                    _gitExecutablePath, argumentsText.Value, stopwatch.ElapsedMilliseconds, nativeErrorCode);
+            }
 
             throw new InvalidOperationException(
-                $"Failed to run '{_gitExecutablePath} {argumentsText}': {ex.Message}", ex);
+                $"Failed to run '{_gitExecutablePath} {argumentsText.Value}': {ex.Message}", ex);
         }
     }
 }

src/DemaConsulting.AgentControl/LauncherUI/MainWindowViewModel.cs

@@ -90,9 +90,8 @@ public MainWindowViewModel(AppSettings settings, StartupOptions? startupOptions
 
         RepoCards = [];
         DisplayedRepoCards = [];
-        foreach (var recentRepo in settings.RecentRepos)
+        foreach (var card in settings.RecentRepos.Select(CreateCard))
         {
-            var card = CreateCard(recentRepo);
             AttachCardEvents(card);
             RepoCards.Add(card);
         }

src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs

@@ -586,9 +586,10 @@ private void Launch()
     ///     <see langword="true"/> if no sync action was needed or attempted (the repo has
     ///     committed agent files, has no pin, or the managed folders were already present), or a
     ///     missing set was silently re-extracted successfully; <see langword="false"/> if a sync
-    ///     attempt was made and failed, in which case <see cref="ErrorOccurred"/> has already been
-    ///     raised as a non-blocking warning explaining why. Either way, <see cref="Launch"/>
-    ///     proceeds to spawn the agent tool regardless of this return value.
+    ///     attempt was made and failed for an ordinary reason (source unreachable, pinned version
+    ///     missing, an I/O failure), in which case <see cref="ErrorOccurred"/> has already been
+    ///     raised as a non-blocking warning explaining why and <see cref="Launch"/> proceeds to
+    ///     spawn the agent tool regardless of this return value.
     /// </returns>
     /// <remarks>
     ///     <para>
@@ -621,8 +622,7 @@ private void Launch()
     ///     through <see cref="ApplyPackageAndShowReleaseNotes"/> - architecture.md's
     ///     ensure-synced-before-launch bullet never mentions showing release notes, unlike its
     ///     Select-Package bullet, so a silent background repair must not pop a release-notes
-    ///     window on every launch. If this re-extraction attempt fails for any reason (source
-    ///     unreachable, pinned version missing, extraction I/O failure), <see cref="ErrorOccurred"/>
+    ///     window on every launch. If this re-extraction attempt fails, <see cref="ErrorOccurred"/>
     ///     is raised as a non-blocking warning and this returns <see langword="false"/> - but the
     ///     launch still proceeds regardless.
     ///     </para>

src/DemaConsulting.AgentControl/LauncherUI/SelectPackageWindow.axaml.cs

@@ -60,7 +60,7 @@ public SelectPackageWindow(SelectPackageWindowViewModel viewModel) : this()
     {
         ArgumentNullException.ThrowIfNull(viewModel);
         DataContext = viewModel;
-        viewModel.Confirmed += (_, result) => Close(((string Name, string Version)?)result);
+        viewModel.Confirmed += (_, result) => Close(result);
     }
 
     /// <summary>
@@ -70,6 +70,6 @@ public SelectPackageWindow(SelectPackageWindowViewModel viewModel) : this()
     /// <param name="e">Routed event arguments (unused).</param>
     private void CancelButton_Click(object? sender, RoutedEventArgs e)
     {
-        Close(((string Name, string Version)?)null);
+        Close(null);
     }
 }