Fix several small bugs that an AI agent detected#2303
Open
mjcheetham wants to merge 16 commits intogit-ecosystem:mainfrom
Open
Fix several small bugs that an AI agent detected#2303mjcheetham wants to merge 16 commits intogit-ecosystem:mainfrom
mjcheetham wants to merge 16 commits intogit-ecosystem:mainfrom
Conversation
When using a custom credential UI for either GitHub or GitLab the commands are mixing up the optional URL and user name args. This is because the positional args of the `ExecuteAsync` methods was not the same as the `SetHandler`. Swap the args to fix this. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
The multi-dictionary writer normalises value lists to honor reset semantics (empty values clear prior entries). However, when only one normalised value remains, it writes the first element from the original list instead of the normalised list. If the list contains an empty reset marker followed by a single valid value, the output will incorrectly emit the pre-reset value (or an empty string), violating the protocol and potentially leaking stale data that should have been cleared. Fix the issue and extend the unit tests to cover this shape. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
If the WWW-Authenticate header from GitHub is missing a domain or enterprise hint we'd be hitting a null-reference exception when calculating a hash code for the `GitHubAuthChallenge`. Fix this. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Outside of GitHub.com we should not filter accounts. The `FilterAccounts` method was detecting and logging this, but didn't actually stop filtering! Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
The network diagnostic failed to correctly await the test HTTP requests, and also did not return and await a `Task` (to capture exceptions). Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Add the missing die function. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
When suppressStreams is true, Git's stderr is redirected to a pipe but then we only read stdout before waiting for exit. If Ggit writes lots to stderr (for example when tracing is enabled), the stderr pipe can fill, causing the Git process to block and GCM to hang while checking repository state. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
If SymbolOutput is not set then there's a bug whereby we try and trim the end '/' and '\' characters on a null value. Guard against this. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
In the OAuth device-code flow, the in-proc UI path the calling logic creates a `CancellationTokenSource` and later calls `Cancel()` on that CTS to close the dialog once the token is obtained (or the user cancels). However, `ShowDeviceCodeViaUiAsync` disregards the provided cancellation token and passes `CancellationToken.None` into `AvaloniaUi.ShowViewAsync`. Pass the cancellation token correctly. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
The `Thread::ManagedTheadId` always starts with the entry thread as `1` and not `0`. https://github.com/dotnet/runtime/blob/790e8a525a0f76b8ad755c12e95b7f8770195d67/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/ManagedThreadId.cs#L181 Fix this in Trace2. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
BuildTimeSpan assumes an 11-character span and adjusts padding when the
formatted elapsed time overflows. For values with 5+ digits before the
decimal (>= 10000 seconds), the size difference exceeds the available
padding budget, driving BeginPadding below zero. This causes
`new string(' ', BeginPadding)` to throw an ArgumentOutOfRangeException.
Since Trace2FileWriter does not catch exceptions, this crashes the
credential helper when TRACE2 performance output is enabled.
Fix the overflow check from `==` to `>=` so that values exceeding the
full span (data + padding) zero out all padding rather than producing a
negative value.
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
We had been incorrectly looking for the `sslAutoClientCert` Git config option under the `credential` section, rather than `http`. Note: this is a Git for Windows only option. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
ReleaseManagedResources iterates forward by index while removing elements from the same list. Each removal shifts remaining elements left, but the loop increments i, causing the next element to be skipped. As a result, only about half of the writers are disposed and removed, leaving file handles or buffers open. Fix by iterating in reverse so that removals do not shift any unvisited indices, and use RemoveAt(i) to avoid a redundant linear search. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
ProcessManager.CreateProcess sets RedirectStandardError=false for all processes to avoid TRACE2 deadlocks. However, GetRemotes and CreateGitException unconditionally read StandardError, which throws InvalidOperationException when stderr is not redirected. Fix GetRemotes by explicitly redirecting stderr before starting the process, since it needs to check for 'not a git repository' errors. Guard CreateGitException defensively, as it is called from various contexts where stderr may or may not be redirected. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
In f1a1ae5 (environment: manually scan $PATH on POSIX systems, 2022-05-31) the `which`-based lookup was replaced with a manual PATH scan that only checks FileExists, without verifying execute permissions. Unlike `which`, this means a non-executable file earlier in PATH can shadow a valid executable, causing process creation to fail when GCM later tries to run the located path. Add an IsExecutable check that verifies at least one execute bit is set on POSIX systems, matching the behaviour of `which`. On Windows, any existing file is considered executable. Guard the POSIX-specific File.GetUnixFileMode call with #if !NETFRAMEWORK for net472 compatibility. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
The powershell.exe invocation in the Installer.Windows.csproj Exec task uses Unicode en dash characters (U+2013) instead of ASCII hyphens for the -NonInteractive and -ExecutionPolicy parameters. Windows PowerShell 5.1 does not recognise en dashes as parameter prefixes, so these flags are not applied correctly, which can cause the layout step to fail or run with an unexpected execution policy. Replace the en dash characters with ASCII hyphens. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
This comment was marked as spam.
This comment was marked as spam.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A collection of small bug fixes found with the help of an AI agent scanning the codebase. None of these are known to be actively exploited or causing user-reported issues, but each is a latent defect that could cause crashes, hangs, or incorrect behaviour under the right conditions.
Fixes
Credential UI —
github/gitlab: use correct param orderStream extensions —
streamextensions: fix a bug in multi-var reset handlingGitHub auth —
github: handle empty domain or enterprise hintsGitHub account filtering —
github: do not filter accounts outside of dotcomNetwork diagnostics —
diagnose: fix network diag to await HTTP requestsmacOS notarize script —
macos: add die function to notarize.sh scriptdiefunction.Git stderr handling —
git: drain stderr on IsInsideRepositoryWindows layout script —
windows: fix layout.ps1 if symboloutput is not setSymbolOutputwhen the variable is unset.OAuth device code UI —
oauth: pass cancellation token to in-proc device code UITRACE2 thread ID —
trace2: fix main thread identificationTRACE2 perf format —
trace2: fix crash in perf format for large elapsed timesArgumentOutOfRangeExceptioncrash when elapsed time exceeds 9999 seconds.HTTP config —
http: use correct http.sslAutoClientCert setting nameTRACE2 writer cleanup —
trace2: fix incomplete disposal of writers on cleanupGit stderr redirect —
git: fix crash when reading stderr from non-redirected processesInvalidOperationExceptionwhenGetRemotes/CreateGitExceptionread non-redirected stderr.Executable lookup —
environment: check execute permission in TryLocateExecutablewhichit replaced.Installer Exec command —
windows: fix en-dash characters in installer Exec command